[ovs-dev] [PATCH v5 ovn 3/4] ofctrl.c: Simplify active desired flow selection.

Dumitru Ceara dceara at redhat.com
Tue Oct 13 07:16:05 UTC 2020


On 10/13/20 9:03 AM, Han Zhou wrote:
> 
> 
> On Sun, Oct 11, 2020 at 5:06 AM Dumitru Ceara <dceara at redhat.com
> <mailto:dceara at redhat.com>> wrote:
>>
>> Always consider the first "desired flow" in the list of flows that
> refer an
>> "installed flow" as the active flow.  This simplifies the logic of the
> flow
>> update code and is used in a further commit to implement a partial
> ordering
>> of desired flows within installed flows.
>>
>> Signed-off-by: Dumitru Ceara <dceara at redhat.com
> <mailto:dceara at redhat.com>>
>> ---
>>  controller/ofctrl.c |   92
> +++++++++++++++++++++------------------------------
>>  1 file changed, 37 insertions(+), 55 deletions(-)
>>
>> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
>> index 20cf3ac..74f98e3 100644
>> --- a/controller/ofctrl.c
>> +++ b/controller/ofctrl.c
>> @@ -188,6 +188,8 @@ struct sb_flow_ref {
>>   * relationship is 1 to N. A link is added when a flow addition is
> processed.
>>   * A link is removed when a flow deletion is processed, the desired flow
>>   * table is cleared, or the installed flow table is cleared.
>> + * The first desired_flow in the list is the active one, the one that is
>> + * actually installed.
>>   */
>>  struct installed_flow {
>>      struct ovn_flow flow;
>> @@ -199,11 +201,6 @@ struct installed_flow {
>>       * installed flow, e.g. when there are conflict/duplicated ACLs that
>>       * generates same match conditions). */
>>      struct ovs_list desired_refs;
>> -
>> -    /* The corresponding flow in desired table. It must be one of the
> flows in
>> -     * desired_refs list.  If there are more than one flows in
> references list,
>> -     * this is the one that is actually installed. */
>> -    struct desired_flow *desired_flow;
>>  };
>>
>>  typedef bool
>> @@ -231,6 +228,8 @@ static struct installed_flow *installed_flow_lookup(
>>      const struct ovn_flow *target);
>>  static void installed_flow_destroy(struct installed_flow *);
>>  static struct installed_flow *installed_flow_dup(struct desired_flow *);
>> +static struct desired_flow *installed_flow_get_active(
>> +    struct installed_flow *f);
>>
>>  static uint32_t ovn_flow_match_hash(const struct ovn_flow *);
>>  static char *ovn_flow_to_string(const struct ovn_flow *);
>> @@ -796,24 +795,6 @@ ofctrl_recv(const struct ofp_header *oh, enum
> ofptype type)
>>          log_openflow_rl(&rl, VLL_DBG, oh, "OpenFlow packet ignored");
>>      }
>>  }
>> -
>> -/* Returns true if a desired flow is active (the one currently installed
>> - * among the list of desired flows that are linked to the same installed
>> - * flow). */
>> -static inline bool
>> -desired_flow_is_active(struct desired_flow *d)
>> -{
>> -    return (d->installed_flow && d->installed_flow->desired_flow == d);
>> -}
>> -
>> -/* Set a desired flow as the active one among the list of desired flows
>> - * that are linked to the same installed flow. */
>> -static inline void
>> -desired_flow_set_active(struct desired_flow *d)
>> -{
>> -    ovs_assert(d->installed_flow);
>> -    d->installed_flow->desired_flow = d;
>> -}
>>
>>  static bool
>>  flow_action_has_conj(const struct ovn_flow *f)
>> @@ -831,27 +812,22 @@ flow_action_has_conj(const struct ovn_flow *f)
>>  /* Adds the desired flow to the list of desired flows that have same
> match
>>   * conditions as the installed flow.
>>   *
>> - * If the newly added desired flow is the first one in the list, it
> is also set
>> - * as the active one.
>> - *
>>   * It is caller's responsibility to make sure the link between the
> pair didn't
>> - * exist before. */
>> -static void
>> + * exist before.
>> + *
>> + * Returns true if the newly added desired flow is selected to be the
> active
>> + * one.
>> + */
>> +static bool
>>  link_installed_to_desired(struct installed_flow *i, struct
> desired_flow *d)
>>  {
>> -    ovs_assert(i->desired_flow != d);
>> -    if (ovs_list_is_empty(&i->desired_refs)) {
>> -        ovs_assert(!i->desired_flow);
>> -        i->desired_flow = d;
>> -    }
>> -    ovs_list_insert(&i->desired_refs, &d->installed_ref_list_node);
>>      d->installed_flow = i;
>> +    ovs_list_push_back(&i->desired_refs, &d->installed_ref_list_node);
>> +    return installed_flow_get_active(i) == d;
>>  }
>>
>>  /* Replaces 'old_desired' with 'new_desired' in the list of desired flows
>>   * that have same match conditions as the installed flow.
>> - *
>> - * If 'old_desired' was the active flow, 'new_desired' becomes the
> active one.
>>   */
>>  static void
>>  replace_installed_to_desired(struct installed_flow *i,
>> @@ -863,24 +839,22 @@ replace_installed_to_desired(struct
> installed_flow *i,
>>                       &old_desired->installed_ref_list_node);
>>      old_desired->installed_flow = NULL;
>>      new_desired->installed_flow = i;
>> -    if (i->desired_flow == old_desired) {
>> -        i->desired_flow = new_desired;
>> -    }
>>  }
>>
>> -static void
>> +/* Removes the desired flow from the list of desired flows that have
> the same
>> + * match conditions as the installed flow.
>> + *
>> + * Returns true if the desired flow was the previously active flow.
>> + */
>> +static bool
>>  unlink_installed_to_desired(struct installed_flow *i, struct
> desired_flow *d)
>>  {
>> -    ovs_assert(i && i->desired_flow &&
> !ovs_list_is_empty(&i->desired_refs));
>> +    struct desired_flow *old_active = installed_flow_get_active(i);
>> +
>>      ovs_assert(d && d->installed_flow == i);
>>      ovs_list_remove(&d->installed_ref_list_node);
>>      d->installed_flow = NULL;
>> -    if (i->desired_flow == d) {
>> -        i->desired_flow = ovs_list_is_empty(&i->desired_refs) ? NULL :
>> -            CONTAINER_OF(ovs_list_front(&i->desired_refs),
>> -                         struct desired_flow,
>> -                         installed_ref_list_node);
>> -    }
>> +    return old_active == d;
>>  }
>>
>>  static void
>> @@ -1280,7 +1254,6 @@ installed_flow_dup(struct desired_flow *src)
>>  {
>>      struct installed_flow *dst = xmalloc(sizeof *dst);
>>      ovs_list_init(&dst->desired_refs);
>> -    dst->desired_flow = NULL;
>>      dst->flow.table_id = src->flow.table_id;
>>      dst->flow.priority = src->flow.priority;
>>      minimatch_clone(&dst->flow.match, &src->flow.match);
>> @@ -1292,6 +1265,17 @@ installed_flow_dup(struct desired_flow *src)
>>  }
>>
>>  static struct desired_flow *
>> +installed_flow_get_active(struct installed_flow *f)
>> +{
>> +    if (!ovs_list_is_empty(&f->desired_refs)) {
>> +        return CONTAINER_OF(ovs_list_front(&f->desired_refs),
>> +                            struct desired_flow,
>> +                            installed_ref_list_node);
>> +    }
>> +    return NULL;
>> +}
>> +
>> +static struct desired_flow *
>>  desired_flow_lookup__(struct ovn_desired_flow_table *flow_table,
>>                        const struct ovn_flow *target,
>>                        desired_flow_match_cb match_cb,
>> @@ -1439,8 +1423,7 @@ static void
>>  installed_flow_destroy(struct installed_flow *f)
>>  {
>>      if (f) {
>> -        ovs_assert(ovs_list_is_empty(&f->desired_refs));
>> -        ovs_assert(!f->desired_flow);
>> +        ovs_assert(!installed_flow_get_active(f));
>>          ovn_flow_uninit(&f->flow);
>>          free(f);
>>      }
>> @@ -1898,10 +1881,10 @@ update_installed_flows_by_track(struct
> ovn_desired_flow_table *flow_table,
>>              /* The desired flow was deleted */
>>              if (f->installed_flow) {
>>                  struct installed_flow *i = f->installed_flow;
>> -                bool was_active = desired_flow_is_active(f);
>> -                unlink_installed_to_desired(i, f);
>> +                bool was_active = unlink_installed_to_desired(i, f);
>> +                struct desired_flow *d = installed_flow_get_active(i);
>>
>> -                if (!i->desired_flow) {
>> +                if (!d) {
>>                      installed_flow_del(&i->flow, msgs);
>>                      ovn_flow_log(&i->flow, "removing installed
> (tracked)");
>>
>> @@ -1912,7 +1895,6 @@ update_installed_flows_by_track(struct
> ovn_desired_flow_table *flow_table,
>>                       * installed flow, so update the OVS flow for the new
>>                       * active flow (at least the cookie will be
> different,
>>                       * even if the actions are the same). */
>> -                    struct desired_flow *d = i->desired_flow;
>>                      ovn_flow_log(&i->flow, "updating installed
> (tracked)");
>>                      installed_flow_mod(&i->flow, &d->flow, msgs);
>>                  }
>> @@ -1931,7 +1913,7 @@ update_installed_flows_by_track(struct
> ovn_desired_flow_table *flow_table,
>>                  hmap_insert(&installed_flows, &new_node->match_hmap_node,
>>                              new_node->flow.hash);
>>                  link_installed_to_desired(new_node, f);
>> -            } else if (desired_flow_is_active(f)) {
>> +            } else if (installed_flow_get_active(i) == f) {
>>                  /* The installed flow is installed for f, but f has
> change
>>                   * tracked, so it must have been modified. */
>>                  ovn_flow_log(&i->flow, "updating installed (tracked)");
>>
> 
> Thanks Dumitru. I applied patch 1 - 3 of the series to master, with a
> tiny change to patch 3 just to follow the coding style:
> 
> ............................... 8><
> ................................................................><8
> ....................................
> index 74f98e36b..ba0c61c80 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -228,8 +228,7 @@ static struct installed_flow *installed_flow_lookup(
>      const struct ovn_flow *target);
>  static void installed_flow_destroy(struct installed_flow *);
>  static struct installed_flow *installed_flow_dup(struct desired_flow *);
> -static struct desired_flow *installed_flow_get_active(
> -    struct installed_flow *f);
> +static struct desired_flow *installed_flow_get_active(struct
> installed_flow *);
>  
>  static uint32_t ovn_flow_match_hash(const struct ovn_flow *);
>  static char *ovn_flow_to_string(const struct ovn_flow *);

Ack, looks good to me.

Thanks,
Dumitru



More information about the dev mailing list