[ovs-dev] [PATCH ovn v2] northd: Support flow offloading for logical switches with no ACLs.
Numan Siddique
numans at ovn.org
Tue May 11 11:50:54 UTC 2021
On Mon, May 10, 2021 at 11:43 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> On 5/10/21 5:39 PM, Dumitru Ceara wrote:
> > On 5/7/21 4:42 PM, numans at ovn.org wrote:
> >> From: Numan Siddique <numans at ovn.org>
> >>
> >> Some smart NICs can't offload datapath flows matching on conntrack
> >> fields. If a deployment desires to make use of such smart NICs
> >> then it cannot configure ACLs on the logical switches. If suppose
> >> a logical switch S1 has no ACLs configured and a logical switch S2
> >> has ACLs configured, then the CMS would expect the datapath flows
> >> belonging to S1 logical ports are offloaded since it has no ACLs.
> >> But this is not working as expected (even if S1 and S2 are
> >> not connected via a logical router).
> >>
> >> ovn-northd generates the below logical flows in ls_in_acl_hint
> >> and ls_in_acl stages for S1
> >>
> >> table=8 (ls_in_acl_hint ), priority=0 , match=(1), action=(next;)
> >> table=9 (ls_in_acl ), priority=0 , match=(1), action=(next;)
> >>
> >> And the below for S2
> >>
> >> table=8 (ls_in_acl_hint ), priority=7 , match=(ct.new && !ct.est), action=(reg0[7] = 1; reg0[9] = 1; next;)
> >> table=8 (ls_in_acl_hint ), priority=6 , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 1), action=(reg0[7] = 1; reg0[9] = 1; next;)
> >> table=8 (ls_in_acl_hint ), priority=5 , match=(!ct.trk), action=(reg0[8] = 1; reg0[9] = 1; next;)
> >> table=8 (ls_in_acl_hint ), priority=4 , match=(!ct.new && ct.est && !ct.rpl && ct_label.blocked == 0), action=(reg0[8] = 1; reg0[10] = 1; next;)
> >> table=8 (ls_in_acl_hint ), priority=3 , match=(!ct.est), action=(reg0[9] = 1; next;)
> >> table=8 (ls_in_acl_hint ), priority=2 , match=(ct.est && ct_label.blocked == 1), action=(reg0[9] = 1; next;)
> >> table=8 (ls_in_acl_hint ), priority=1 , match=(ct.est && ct_label.blocked == 0), action=(reg0[10] = 1; next;)
> >> table=8 (ls_in_acl_hint ), priority=0 , match=(1), action=(next;)
> >> table=9 (ls_in_acl ), priority=65535, match=(!ct.est && ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
> >> table=9 (ls_in_acl ), priority=65535, match=(ct.est && !ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0), action=(next;)
> >> table=9 (ls_in_acl ), priority=65535, match=(ct.inv || (ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
> >> table=9 (ls_in_acl ), priority=65535, match=(nd || nd_ra || nd_rs || mldv1 || mldv2), action=(next;)
> >> table=9 (ls_in_acl ), priority=34000, match=(eth.dst == $svc_monitor_mac), action=(next;)
> >> table=9 (ls_in_acl ), priority=1 , match=(ip && (!ct.est || (ct.est && ct_label.blocked == 1))), action=(reg0[1] = 1; next;)
> >> table=9 (ls_in_acl ), priority=0 , match=(1), action=(next;)
> >>
> >> Because there are higher priority flows in 'ls_in_acl_hint' and
> >> 'ls_in_acl' with the match on conntrack fields,
> >> ovs-vswitchd will generate a datapath flow with the match on ct_state fields as -
> >> 'ct_state(-new-est-rel-rpl-inv-trk)' for the packet from S1, even though
> >> the S1 pipeline doesn't have logical flows which match on conntrack
> >> fields. [1] has more information.
> >>
> >> Modifying the below flows if a logical switch has no ACLs solves this
> >> problem.
> >>
> >> table=8 (ls_in_acl_hint ), priority=65535 , match=(1), action=(next;)
> >> table=9 (ls_in_acl ), priority=65535 , match=(1), action=(next;)
> >>
> >> With the above flows with higher priority, ovs-vswitchd will not
> >> consider other flows in the same table during translation.
> >>
> >> This patch addresses this issue by using higher prioriy flows (for both
> >> ls_in_acl* and ls_out_acl* stages).
> >>
> >> [1] - https://bugzilla.redhat.com/show_bug.cgi?id=1955191#c8
> >>
> >> Signed-off-by: Numan Siddique <numans at ovn.org>
> >> ---
> >
> > Hi Numan,
> >
> > A couple of tests are failing after rebase:
> >
> > 789: ovn -- ct.inv usage -- ovn-northd -- dp-groups=yes FAILED
> > (ovn-northd.at:3147)
> > 790: ovn -- ct.inv usage -- ovn-northd FAILED
> > (ovn-northd.at:3147)
> >
> >> v1 -> v2
> >> ----
> >> * Rebased to resolve conflicts.
> >> * Addressed review comment from Dumitru. Combined ls_has_stateful_acl()
> >> and ls_has_acl() into one single function - od_ls_update_acls_flags().
> >
> > Nit: There's no other function in ovn_northd that's prefixed with
> > od_ls_.*(). Maybe it makes sense to rename this to ls_get_acl_flags()
> > to be inline with ls_has_lb_vip()?
> >
>
> I assume the test failure is just due to the rebase and can be easily
> fixed. The function rename can be done at apply time then:
>
> Acked-by: Dumitru Ceara <dceara at redhat.com>
Thanks Dumitru.
I applied this patch to the main branch with the below changes.
*****
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index b137cba6b6..97cdfa7d69 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -4760,7 +4760,7 @@ ovn_ls_port_group_destroy(struct hmap *nb_pgs)
}
static void
-od_ls_update_acls_flags(struct ovn_datapath *od)
+ls_get_acl_flags(struct ovn_datapath *od)
{
od->has_acls = false;
od->has_stateful_acl = false;
@@ -6818,7 +6818,7 @@ build_lswitch_lflows_pre_acl_and_acl(struct
ovn_datapath *od,
{
if (od->nbs) {
od->has_lb_vip = ls_has_lb_vip(od);
- od_ls_update_acls_flags(od);
+ ls_get_acl_flags(od);
build_pre_acls(od, lflows);
build_pre_lb(od, lflows, meter_groups, lbs);
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index bedd556d53..ed3c6106fb 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -3156,18 +3156,18 @@ check ovn-nbctl --wait=sb acl-add sw0 to-lport
1002 ip allow-related
ovn-sbctl dump-flows sw0 > sw0flows
AT_CAPTURE_FILE([sw0flows])
-AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 65535 | sort], [0], [dnl
- table=9 (ls_in_acl ), priority=65535, match=(!ct.est &&
ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
- table=9 (ls_in_acl ), priority=65535, match=(ct.est &&
!ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0),
action=(next;)
- table=9 (ls_in_acl ), priority=65535, match=(ct.inv ||
(ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
- table=9 (ls_in_acl ), priority=65535, match=(nd || nd_ra
|| nd_rs || mldv1 || mldv2), action=(next;)
+AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 6553 | sort], [0], [dnl
+ table=9 (ls_in_acl ), priority=65532, match=(!ct.est &&
ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
+ table=9 (ls_in_acl ), priority=65532, match=(ct.est &&
!ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0),
action=(next;)
+ table=9 (ls_in_acl ), priority=65532, match=(ct.inv ||
(ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
+ table=9 (ls_in_acl ), priority=65532, match=(nd || nd_ra
|| nd_rs || mldv1 || mldv2), action=(next;)
])
-AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 65535 | sort], [0], [dnl
- table=4 (ls_out_acl ), priority=65535, match=(!ct.est &&
ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
- table=4 (ls_out_acl ), priority=65535, match=(ct.est &&
!ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0),
action=(next;)
- table=4 (ls_out_acl ), priority=65535, match=(ct.inv ||
(ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
- table=4 (ls_out_acl ), priority=65535, match=(nd || nd_ra
|| nd_rs || mldv1 || mldv2), action=(next;)
+AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 6553 | sort], [0], [dnl
+ table=4 (ls_out_acl ), priority=65532, match=(!ct.est &&
ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
+ table=4 (ls_out_acl ), priority=65532, match=(ct.est &&
!ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0),
action=(next;)
+ table=4 (ls_out_acl ), priority=65532, match=(ct.inv ||
(ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
+ table=4 (ls_out_acl ), priority=65532, match=(nd || nd_ra
|| nd_rs || mldv1 || mldv2), action=(next;)
])
# Disable ct.inv usage.
@@ -3176,18 +3176,18 @@ check ovn-nbctl --wait=sb set NB_Global .
options:use_ct_inv_match=false
ovn-sbctl dump-flows sw0 > sw0flows
AT_CAPTURE_FILE([sw0flows])
-AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 65535 | sort], [0], [dnl
- table=9 (ls_in_acl ), priority=65535, match=(!ct.est &&
ct.rel && !ct.new && ct_label.blocked == 0), action=(next;)
- table=9 (ls_in_acl ), priority=65535, match=((ct.est &&
ct.rpl && ct_label.blocked == 1)), action=(drop;)
- table=9 (ls_in_acl ), priority=65535, match=(ct.est &&
!ct.rel && !ct.new && ct.rpl && ct_label.blocked == 0), action=(next;)
- table=9 (ls_in_acl ), priority=65535, match=(nd || nd_ra
|| nd_rs || mldv1 || mldv2), action=(next;)
+AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 6553 | sort], [0], [dnl
+ table=9 (ls_in_acl ), priority=65532, match=(!ct.est &&
ct.rel && !ct.new && ct_label.blocked == 0), action=(next;)
+ table=9 (ls_in_acl ), priority=65532, match=((ct.est &&
ct.rpl && ct_label.blocked == 1)), action=(drop;)
+ table=9 (ls_in_acl ), priority=65532, match=(ct.est &&
!ct.rel && !ct.new && ct.rpl && ct_label.blocked == 0), action=(next;)
+ table=9 (ls_in_acl ), priority=65532, match=(nd || nd_ra
|| nd_rs || mldv1 || mldv2), action=(next;)
])
-AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 65535 | sort], [0], [dnl
- table=4 (ls_out_acl ), priority=65535, match=(!ct.est &&
ct.rel && !ct.new && ct_label.blocked == 0), action=(next;)
- table=4 (ls_out_acl ), priority=65535, match=((ct.est &&
ct.rpl && ct_label.blocked == 1)), action=(drop;)
- table=4 (ls_out_acl ), priority=65535, match=(ct.est &&
!ct.rel && !ct.new && ct.rpl && ct_label.blocked == 0), action=(next;)
- table=4 (ls_out_acl ), priority=65535, match=(nd || nd_ra
|| nd_rs || mldv1 || mldv2), action=(next;)
+AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 6553 | sort], [0], [dnl
+ table=4 (ls_out_acl ), priority=65532, match=(!ct.est &&
ct.rel && !ct.new && ct_label.blocked == 0), action=(next;)
+ table=4 (ls_out_acl ), priority=65532, match=((ct.est &&
ct.rpl && ct_label.blocked == 1)), action=(drop;)
+ table=4 (ls_out_acl ), priority=65532, match=(ct.est &&
!ct.rel && !ct.new && ct.rpl && ct_label.blocked == 0), action=(next;)
+ table=4 (ls_out_acl ), priority=65532, match=(nd || nd_ra
|| nd_rs || mldv1 || mldv2), action=(next;)
])
AT_CHECK([grep -c "ct.inv" sw0flows], [1], [dnl
@@ -3200,18 +3200,18 @@ check ovn-nbctl --wait=sb set NB_Global .
options:use_ct_inv_match=true
ovn-sbctl dump-flows sw0 > sw0flows
AT_CAPTURE_FILE([sw0flows])
-AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 65535 | sort], [0], [dnl
- table=9 (ls_in_acl ), priority=65535, match=(!ct.est &&
ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
- table=9 (ls_in_acl ), priority=65535, match=(ct.est &&
!ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0),
action=(next;)
- table=9 (ls_in_acl ), priority=65535, match=(ct.inv ||
(ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
- table=9 (ls_in_acl ), priority=65535, match=(nd || nd_ra
|| nd_rs || mldv1 || mldv2), action=(next;)
+AT_CHECK([grep -w "ls_in_acl" sw0flows | grep 6553 | sort], [0], [dnl
+ table=9 (ls_in_acl ), priority=65532, match=(!ct.est &&
ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
+ table=9 (ls_in_acl ), priority=65532, match=(ct.est &&
!ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0),
action=(next;)
+ table=9 (ls_in_acl ), priority=65532, match=(ct.inv ||
(ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
+ table=9 (ls_in_acl ), priority=65532, match=(nd || nd_ra
|| nd_rs || mldv1 || mldv2), action=(next;)
])
-AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 65535 | sort], [0], [dnl
- table=4 (ls_out_acl ), priority=65535, match=(!ct.est &&
ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
- table=4 (ls_out_acl ), priority=65535, match=(ct.est &&
!ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0),
action=(next;)
- table=4 (ls_out_acl ), priority=65535, match=(ct.inv ||
(ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
- table=4 (ls_out_acl ), priority=65535, match=(nd || nd_ra
|| nd_rs || mldv1 || mldv2), action=(next;)
+AT_CHECK([grep -w "ls_out_acl" sw0flows | grep 6553 | sort], [0], [dnl
+ table=4 (ls_out_acl ), priority=65532, match=(!ct.est &&
ct.rel && !ct.new && !ct.inv && ct_label.blocked == 0), action=(next;)
+ table=4 (ls_out_acl ), priority=65532, match=(ct.est &&
!ct.rel && !ct.new && !ct.inv && ct.rpl && ct_label.blocked == 0),
action=(next;)
+ table=4 (ls_out_acl ), priority=65532, match=(ct.inv ||
(ct.est && ct.rpl && ct_label.blocked == 1)), action=(drop;)
+ table=4 (ls_out_acl ), priority=65532, match=(nd || nd_ra
|| nd_rs || mldv1 || mldv2), action=(next;)
])
AT_CHECK([grep -c "ct.inv" sw0flows], [0], [dnl
*****
Numan
>
> Regards,
> Dumitru
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list