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

Dumitru Ceara dceara at redhat.com
Wed Jan 27 10:07:14 UTC 2021


On 1/25/21 8:08 PM, 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.")
> Suggested-by: Dumitru Ceara <dceara at redhat.com>
> Signed-off-by: Numan Siddique <numans at 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 <dceara at redhat.com>

Thanks,
Dumitru



More information about the dev mailing list