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

Han Zhou hzhou at ovn.org
Tue Oct 13 07:03:16 UTC 2020


On Sun, Oct 11, 2020 at 5:06 AM Dumitru Ceara <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>
> ---
>  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 *);


More information about the dev mailing list