[ovs-dev] [PATCH v2 ovn] Save the addr set and port groups in lflow expr cache

Numan Siddique numans at ovn.org
Wed Feb 19 00:58:33 UTC 2020


On Wed, Feb 19, 2020, 3:01 AM Mark Michelson <mmichels at redhat.com> wrote:

> Hi, Numan. Would it be possible to add a test case that exercises the fix?
>

Hi Mark,

The modified test case in this patch fails without the fix.

Numan


> On 2/18/20 2:53 PM, numans at ovn.org wrote:
> > From: Numan Siddique <numans at ovn.org>
> >
> > After the patch [1], which added caching of lflow expr, the
> lflow_resource_ref
> > is not rebuilt properly when lflow_run() is called. If a lflow is
> already cached
> > in lflow expr cache, then the lflow_resource_ref is not updated.
> > But flow_output_run() clears the lflow_resource_ref cache before calling
> lflow_run().
> >
> > This results in incorrect OF flows present in the switch even if the
> > address set/port group references have changed.
> >
> > This patch fixes this issue by saving the addr set and port group sets in
> > the lfow expr cache and updating the lflow_resource_ref.
> >
> > [1] - 8795bec737b9("ovn-controller: Cache logical flow expr tree for
> each lflow.")
> > Fixes: 8795bec737b9("ovn-controller: Cache logical flow expr tree for
> each lflow.")
> >
> > Signed-off-by: Numan Siddique <numans at ovn.org>
> > ---
> >   controller/lflow.c | 63 ++++++++++++++++++++++++++++++++++------------
> >   tests/ovn.at       |  4 +++
> >   2 files changed, 51 insertions(+), 16 deletions(-)
> >
> > diff --git a/controller/lflow.c b/controller/lflow.c
> > index 79d89131a..110809df1 100644
> > --- a/controller/lflow.c
> > +++ b/controller/lflow.c
> > @@ -268,16 +268,21 @@ struct lflow_expr {
> >       struct hmap_node node;
> >       struct uuid lflow_uuid; /* key */
> >       struct expr *expr;
> > +    struct sset addr_sets_ref;
> > +    struct sset port_groups_ref;
> >   };
> >
> >   static void
> >   lflow_expr_add(struct hmap *lflow_expr_cache,
> >                  const struct sbrec_logical_flow *lflow,
> > -               struct expr *lflow_expr)
> > +               struct expr *lflow_expr, struct sset *addr_sets_ref,
> > +               struct sset *port_groups_ref)
> >   {
> >       struct lflow_expr *le = xmalloc(sizeof *le);
> >       le->lflow_uuid = lflow->header_.uuid;
> >       le->expr = lflow_expr;
> > +    sset_clone(&le->addr_sets_ref, addr_sets_ref);
> > +    sset_clone(&le->port_groups_ref, port_groups_ref);
> >       hmap_insert(lflow_expr_cache, &le->node,
> uuid_hash(&le->lflow_uuid));
> >   }
> >
> > @@ -301,6 +306,8 @@ lflow_expr_delete(struct hmap *lflow_expr_cache,
> struct lflow_expr *le)
> >   {
> >       hmap_remove(lflow_expr_cache, &le->node);
> >       expr_destroy(le->expr);
> > +    sset_destroy(&le->addr_sets_ref);
> > +    sset_destroy(&le->port_groups_ref);
> >       free(le);
> >   }
> >
> > @@ -310,6 +317,8 @@ lflow_expr_destroy(struct hmap *lflow_expr_cache)
> >       struct lflow_expr *le;
> >       HMAP_FOR_EACH_POP (le, node, lflow_expr_cache) {
> >           expr_destroy(le->expr);
> > +        sset_destroy(&le->addr_sets_ref);
> > +        sset_destroy(&le->port_groups_ref);
> >           free(le);
> >       }
> >
> > @@ -548,6 +557,25 @@ update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t
> n_conjs)
> >       return true;
> >   }
> >
> > +static void
> > +lflow_update_resource_refs(const struct sbrec_logical_flow *lflow,
> > +                           struct sset *addr_sets_ref,
> > +                           struct sset *port_groups_ref,
> > +                           struct lflow_resource_ref *lfrr)
> > +{
> > +    const char *addr_set_name;
> > +    SSET_FOR_EACH (addr_set_name, addr_sets_ref) {
> > +        lflow_resource_add(lfrr, REF_TYPE_ADDRSET, addr_set_name,
> > +                           &lflow->header_.uuid);
> > +    }
> > +
> > +    const char *port_group_name;
> > +    SSET_FOR_EACH (port_group_name, port_groups_ref) {
> > +        lflow_resource_add(lfrr, REF_TYPE_PORTGROUP, port_group_name,
> > +                           &lflow->header_.uuid);
> > +    }
> > +}
> > +
> >   static bool
> >   consider_logical_flow(const struct sbrec_logical_flow *lflow,
> >                         struct hmap *dhcp_opts, struct hmap *dhcpv6_opts,
> > @@ -615,33 +643,27 @@ consider_logical_flow(const struct
> sbrec_logical_flow *lflow,
> >       struct hmap matches;
> >       struct expr *expr = NULL;
> >
> > -    struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
> > -    struct sset port_groups_ref = SSET_INITIALIZER(&port_groups_ref);
> >       struct lflow_expr *le =
> lflow_expr_get(l_ctx_out->lflow_expr_cache, lflow);
> >       if (le) {
> >           if (delete_expr_from_cache) {
> >               lflow_expr_delete(l_ctx_out->lflow_expr_cache, le);
> > +            le = NULL;
> >           } else {
> >               expr = le->expr;
> >           }
> >       }
> >
> >       if (!expr) {
> > +        struct sset addr_sets_ref = SSET_INITIALIZER(&addr_sets_ref);
> > +        struct sset port_groups_ref =
> SSET_INITIALIZER(&port_groups_ref);
> > +
> >           expr = expr_parse_string(lflow->match, &symtab,
> l_ctx_in->addr_sets,
> >                                    l_ctx_in->port_groups, &addr_sets_ref,
> >                                    &port_groups_ref, &error);
> > -        const char *addr_set_name;
> > -        SSET_FOR_EACH (addr_set_name, &addr_sets_ref) {
> > -            lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_ADDRSET,
> > -                               addr_set_name, &lflow->header_.uuid);
> > -        }
> > -        const char *port_group_name;
> > -        SSET_FOR_EACH (port_group_name, &port_groups_ref) {
> > -            lflow_resource_add(l_ctx_out->lfrr, REF_TYPE_PORTGROUP,
> > -                               port_group_name, &lflow->header_.uuid);
> > -        }
> > -        sset_destroy(&addr_sets_ref);
> > -        sset_destroy(&port_groups_ref);
> > +        /* Add the address set and port groups (if any) to the lflow
> > +         * references. */
> > +        lflow_update_resource_refs(lflow, &addr_sets_ref,
> &port_groups_ref,
> > +                                   l_ctx_out->lfrr);
> >
> >           if (!error) {
> >               if (prereqs) {
> > @@ -658,15 +680,24 @@ consider_logical_flow(const struct
> sbrec_logical_flow *lflow,
> >               free(error);
> >               ovnacts_free(ovnacts.data, ovnacts.size);
> >               ofpbuf_uninit(&ovnacts);
> > +            sset_destroy(&addr_sets_ref);
> > +            sset_destroy(&port_groups_ref);
> >               return true;
> >           }
> >
> >           expr = expr_simplify(expr);
> >           expr = expr_normalize(expr);
> >
> > -        lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr);
> > +        lflow_expr_add(l_ctx_out->lflow_expr_cache, lflow, expr,
> > +                       &addr_sets_ref, &port_groups_ref);
> > +        sset_destroy(&addr_sets_ref);
> > +        sset_destroy(&port_groups_ref);
> >       } else {
> >           expr_destroy(prereqs);
> > +        /* Add the cached address set and port groups (if any) to the
> lflow
> > +         * references. */
> > +        lflow_update_resource_refs(lflow, &le->addr_sets_ref,
> > +                                   &le->port_groups_ref,
> l_ctx_out->lfrr);
> >       }
> >
> >       struct condition_aux cond_aux = {
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index ea6760e1f..254645a3a 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -13778,6 +13778,10 @@ for i in 1 2 3; do
> >       n_flows_after=`ovs-ofctl dump-flows br-int | grep "10.1.2.10" | wc
> -l`
> >       AT_CHECK([test $(expr $n_flows_before \* 2) = $n_flows_after],
> [0], [ignore])
> >
> > +    # Trigger full recompute. Creating a chassis would trigger full
> recompute.
> > +    ovn-sbctl chassis-add tst geneve 127.0.0.4
> > +    ovn-sbctl chassis-del tst
> > +
> >       # Remove an ACL
> >       ovn-nbctl --wait=hv acl-del ls1 to-lport 200 \
> >               'outport=="lp2" && ip4 && ip4.src == $as1'
> >
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>


More information about the dev mailing list