[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 11:29:37 UTC 2020
On Wed, Feb 19, 2020 at 1:10 PM Han Zhou <hzhou at ovn.org> wrote:
>
> On Tue, Feb 18, 2020 at 11:54 AM <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'
> > --
> > 2.24.1
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> Acked-by: Han Zhou <hzhou at ovn.org>
Thanks for the review. I applied this patch to master and branch-20.03.
Thanks
Numan
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list