[ovs-dev] [PATCH ovn] ovn-controller: Fix wrong conj_id match flows when caching is enabled.

Numan Siddique numans at ovn.org
Mon Jan 25 19:13:19 UTC 2021


On Mon, Jan 25, 2021 at 7:46 PM Dumitru Ceara <dceara at redhat.com> wrote:
>
> On 1/22/21 9:33 AM, numans at ovn.org wrote:
> > From: Numan Siddique <numans at ovn.org>
> >
> > When the below ACL is added -
> > ovn-nbctl acl-add ls1 to-lport 3
> >    '(ip4.src==10.0.0.1 || ip4.src==10.0.0.2) &&
> >     (ip4.dst == 10.0.0.3 || ip4.dst == 10.0.0.4)' allow
> >
> > ovn-controller installs the below OF flows
> >
> > table=45, priority=1003,ip,metadata=0x1,nw_dst=10.0.0.4 actions=conjunction(2,1/2)
> > table=45, priority=1003,ip,metadata=0x1,nw_dst=10.0.0.3 actions=conjunction(2,1/2)
> > table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.2 actions=conjunction(2,2/2)
> > table=45, priority=1003,ip,metadata=0x1,nw_src=10.0.0.1 actions=conjunction(2,2/2)
> > table=45, priority=1003,conj_id=2,ip,metadata=0x1 actions=resubmit(,46)
> >
> > When a full recompute is triggered, ovn-controller deletes the last
> > OF flow with the match conj_id=2 and adds the below OF flow
> >
> > table=45, priority=1003,conj_id=3,ip,metadata=0x1 actions=resubmit(,46)
> >
> > For subsequent recomputes, the conj_id keeps increasing by 1.
> >
> > This disrupts the traffic which matches on conjuction action flows.
> >
> > This patch fixes this issue.
> >
> > Fixes: 1213bc8270("ovn-controller: Cache logical flow expr matches.")
> > Signed-off-by: Numan Siddique <numans at ovn.org>
> > ---
>
> Hi Numan,
>
> Good catch!
>
> >   controller/lflow.c |  7 +++++++
> >   tests/ovn.at       | 28 ++++++++++++++++++++++++++++
> >   2 files changed, 35 insertions(+)
> >
> > diff --git a/controller/lflow.c b/controller/lflow.c
> > index c02585b1e..a9420a7c4 100644
> > --- a/controller/lflow.c
> > +++ b/controller/lflow.c
> > @@ -754,6 +754,13 @@ add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
> >                                         &m->match, &conj, &lflow->header_.uuid);
> >               ofpbuf_uninit(&conj);
> >           }
> > +
> > +        if (m->match.wc.masks.conj_id) {
> > +            /* Reset the conj_id back to relative conj id. If caching is
> > +             * enabled, then processing of the expr match next time (due to
> > +             * full recompute) will result in the wrong conj_id match flow. */
> > +            m->match.flow.conj_id -= conj_id_ofs;
> > +        }
>
> While this works I'm not sure this is the best way to fix the issue.
> We're now making the add_matches_to_flow_table() aware of the lflow cache.


Thanks for the comments.

If you see, In the lflow_expr cache we do store the conj_ids offset(s)
 associated with the logical flow. So I thought , storing the original
expr match
in the cache would make sense. And hence the patch reverts the changes
done to the expr match conj_id in the add_matches_to_flow_table().

But I do think your suggestion would make sense. So I submitted v2 as per
your suggestion. Please take a look.

Thanks
Numan


>
> In my opinion the problem is that the matches we cache when storing
> entries of type LCACHE_T_MATCHES are not fully processed.
>
> One option to fix that is to pass the current conj_ids_ofs to
> expr_to_matches() to make sure the generated match objects always use
> the "final" conj_id.
>
> This is quite intrusive I guess.
>
> An alternative is to add a new step, something like:
>
> prepare_matches(matches, conj_id_ofs)
> {
>      for_each m in matches:
>          // adjust m conj_id
>          if (m->match.wc.masks.conj_id)
>              m->match.flow.conj_id += conj_id_ofs;
>          for_each conj in m->conjunctions:
>              conj->id += conj_id_ofs;
> }
>
> We'd also have to remove the parts that update the conj_ids in
> add_matches_to_flow_table():
>
> https://github.com/ovn-org/ovn/blob/a2d043d2ee473da8e4a835730d86f16a47fb096b/controller/lflow.c#L712
>
> and
>
> https://github.com/ovn-org/ovn/blob/master/controller/lflow.c#L747
>
> Then we could call prepare_matches() before add_matches_to_flow_table() if:
> 1. flow cache disabled.
> 2. if flow cache is enabled and we're caching lflow matches.
>
> Later, when cached matches are used (from step 2 above) we don't need to
> change them in any way and we can just call add_matches_to_flow_table()
> without having to worry about offsets.
>
> What do you think?




>
>
> Thanks,
> Dumitru
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list