On Wed, Jan 27, 2021 at 3:37 PM Dumitru Ceara <dce...@redhat.com> wrote:
>
> On 1/25/21 8:08 PM, num...@ovn.org wrote:
> > From: Numan Siddique <num...@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.")
> > Suggested-by: Dumitru Ceara <dce...@redhat.com>
> > Signed-off-by: Numan Siddique <num...@ovn.org>
> > ---
> > controller/lflow.c | 46 ++++++++++++++++++++++++++++++----------------
> > tests/ovn.at | 28 ++++++++++++++++++++++++++++
> > 2 files changed, 58 insertions(+), 16 deletions(-)
> >
> > diff --git a/controller/lflow.c b/controller/lflow.c
> > index c02585b1e..7bb3cdaeb 100644
> > --- a/controller/lflow.c
> > +++ b/controller/lflow.c
> > @@ -668,9 +668,8 @@ update_conj_id_ofs(uint32_t *conj_id_ofs, uint32_t
> > n_conjs)
> > static void
> > add_matches_to_flow_table(const struct sbrec_logical_flow *lflow,
> > const struct sbrec_datapath_binding *dp,
> > - struct hmap *matches, size_t conj_id_ofs,
> > - uint8_t ptable, uint8_t output_ptable,
> > - struct ofpbuf *ovnacts,
> > + struct hmap *matches, uint8_t ptable,
> > + uint8_t output_ptable, struct ofpbuf *ovnacts,
> > bool ingress, struct lflow_ctx_in *l_ctx_in,
> > struct lflow_ctx_out *l_ctx_out)
> > {
> > @@ -708,9 +707,6 @@ add_matches_to_flow_table(const struct
> > sbrec_logical_flow *lflow,
> > struct expr_match *m;
> > HMAP_FOR_EACH (m, hmap_node, matches) {
> > match_set_metadata(&m->match, htonll(dp->tunnel_key));
> > - if (m->match.wc.masks.conj_id) {
> > - m->match.flow.conj_id += conj_id_ofs;
> > - }
> > if (datapath_is_switch(dp)) {
> > unsigned int reg_index
> > = (ingress ? MFF_LOG_INPORT : MFF_LOG_OUTPORT) - MFF_REG0;
> > @@ -744,7 +740,7 @@ add_matches_to_flow_table(const struct
> > sbrec_logical_flow *lflow,
> > struct ofpact_conjunction *dst;
> >
> > dst = ofpact_put_CONJUNCTION(&conj);
> > - dst->id = src->id + conj_id_ofs;
> > + dst->id = src->id;
> > dst->clause = src->clause;
> > dst->n_clauses = src->n_clauses;
> > }
> > @@ -815,6 +811,22 @@ convert_match_to_expr(const struct sbrec_logical_flow
> > *lflow,
> > return expr_simplify(e);
> > }
> >
> > +static void
> > +prepare_expr_matches(struct hmap *matches, uint32_t conj_id_ofs)
>
> I think this fits better as a function in expr.c because it only updates
> internals of the expression matches, maybe renamed to
> expr_matches_prepare().
>
> > +{
> > + struct expr_match *m;
> > + HMAP_FOR_EACH (m, hmap_node, matches) {
> > + if (m->match.wc.masks.conj_id) {
> > + m->match.flow.conj_id += conj_id_ofs;
> > + }
> > +
> > + for (int i = 0; i < m->n; i++) {
>
> Nit: s/int i/size_t i/
>
> With these small issues addressed:
>
> Acked-by: Dumitru Ceara <dce...@redhat.com>
Thanks for the review and comments. I addressed them and pushed the
patch to master, branch-20.12 and branch-20.09.
Thanks
Numan
>
> Thanks,
> Dumitru
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
