[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