[ovs-dev] [PATCH ovn] northd: Support flow offloading for logical switches with no ACLs.
Numan Siddique
numans at ovn.org
Mon May 10 09:58:34 UTC 2021
On Fri, May 7, 2021 at 11:26 AM Dumitru Ceara <dceara at redhat.com> wrote:
>
> On 5/7/21 4:35 PM, Numan Siddique wrote:
> > On Fri, May 7, 2021 at 5:50 AM Dumitru Ceara <dceara at redhat.com> wrote:
> >>
> >> On 5/6/21 3:52 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,
> >>
> >> This needs a rebase after the recent commits to master.
> >
> > Thanks for the review. Sure I'll rebase and submit v2.
> >
> >>
> >>> northd/lswitch.dl | 12 ++++
> >>> northd/ovn-northd.8.xml | 32 ++++++----
> >>> northd/ovn-northd.c | 62 ++++++++++++++-----
> >>> northd/ovn_northd.dl | 62 +++++++++++--------
> >>> tests/ovn-northd.at | 51 ++++++++++++----
> >>> tests/system-ovn.at | 130 ++++++++++++++++++++++++++++++++++++++++
> >>> 6 files changed, 284 insertions(+), 65 deletions(-)
> >>>
> >>> diff --git a/northd/lswitch.dl b/northd/lswitch.dl
> >>> index 47c497e0cf..9bcfe9c321 100644
> >>> --- a/northd/lswitch.dl
> >>> +++ b/northd/lswitch.dl
> >>> @@ -125,6 +125,15 @@ LogicalSwitchHasStatefulACL(ls, false) :-
> >>> nb::Logical_Switch(._uuid = ls),
> >>> not LogicalSwitchStatefulACL(ls, _).
> >>>
> >>> +relation LogicalSwitchHasACLs(ls: uuid, has_acls: bool)
> >>> +
> >>> +LogicalSwitchHasACLs(ls, true) :-
> >>> + LogicalSwitchACL(ls, _).
> >>> +
> >>> +LogicalSwitchHasACLs(ls, false) :-
> >>> + nb::Logical_Switch(._uuid = ls),
> >>> + not LogicalSwitchACL(ls, _).
> >>> +
> >>> /*
> >>> * LogicalSwitchLocalnetPorts maps from each logical switch UUID
> >>> * to the logical switch's set of localnet ports. Each localnet
> >>> @@ -189,6 +198,7 @@ LogicalSwitchHasNonRouterPort(ls, false) :-
> >>> relation &Switch(
> >>> ls: nb::Logical_Switch,
> >>> has_stateful_acl: bool,
> >>> + has_acls: bool,
> >>> has_lb_vip: bool,
> >>> has_dns_records: bool,
> >>> has_unknown_ports: bool,
> >>> @@ -215,6 +225,7 @@ function ipv6_parse_prefix(s: string): Option<in6_addr> {
> >>>
> >>> &Switch(.ls = ls,
> >>> .has_stateful_acl = has_stateful_acl,
> >>> + .has_acls = has_acls,
> >>> .has_lb_vip = has_lb_vip,
> >>> .has_dns_records = has_dns_records,
> >>> .has_unknown_ports = has_unknown_ports,
> >>> @@ -226,6 +237,7 @@ function ipv6_parse_prefix(s: string): Option<in6_addr> {
> >>> .is_vlan_transparent = is_vlan_transparent) :-
> >>> nb::Logical_Switch[ls],
> >>> LogicalSwitchHasStatefulACL(ls._uuid, has_stateful_acl),
> >>> + LogicalSwitchHasACLs(ls._uuid, has_acls),
> >>> LogicalSwitchHasLBVIP(ls._uuid, has_lb_vip),
> >>> LogicalSwitchHasDNSRecords(ls._uuid, has_dns_records),
> >>> LogicalSwitchHasUnknownPorts(ls._uuid, has_unknown_ports),
> >>> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> >>> index 54e88d3fac..90a1f7d0b3 100644
> >>> --- a/northd/ovn-northd.8.xml
> >>> +++ b/northd/ovn-northd.8.xml
> >>> @@ -545,6 +545,14 @@
> >>> <p>
> >>> The table contains the following flows:
> >>> </p>
> >>> + <ul>
> >>> + <li>
> >>> + A priority-65535 flow to advance to the next table if the logical
> >>> + switch has <code>no</code> ACLs configured, otherwise a
> >>> + priority-0 flow to advance to the next table.
> >>> + </li>
> >>> + </ul>
> >>> +
> >>> <ul>
> >>> <li>
> >>> A priority-7 flow that matches on packets that initiate a new session.
> >>> @@ -585,9 +593,6 @@
> >>> This flow sets <code>reg0[10]</code> and then advances to the next
> >>> table.
> >>> </li>
> >>> - <li>
> >>> - A priority-0 flow to advance to the next table.
> >>> - </li>
> >>> </ul>
> >>>
> >>> <h3>Ingress table 9: <code>from-lport</code> ACLs</h3>
> >>> @@ -633,9 +638,14 @@
> >>> </ul>
> >>>
> >>> <p>
> >>> - This table also contains a priority 0 flow with action
> >>> - <code>next;</code>, so that ACLs allow packets by default. If the
> >>> - logical datapath has a stateful ACL or a load balancer with VIP
> >>> + This table contains a priority-65535 flow to advance to the next table
> >>> + if the logical switch has <code>no</code> ACLs configured, otherwise a
> >>> + priority-0 flow to advance to the next table so that ACLs allow
> >>> + packets by default.
> >>> + </p>
> >>> +
> >>> + <p>
> >>> + If the logical datapath has a stateful ACL or a load balancer with VIP
> >>> configured, the following flows will also be added:
> >>> </p>
> >>>
> >>> @@ -649,7 +659,7 @@
> >>> </li>
> >>>
> >>> <li>
> >>> - A priority-65535 flow that allows any traffic in the reply
> >>> + A priority-65532 flow that allows any traffic in the reply
> >>> direction for a connection that has been committed to the
> >>> connection tracker (i.e., established flows), as long as
> >>> the committed flow does not have <code>ct_label.blocked</code> set.
> >>> @@ -662,19 +672,19 @@
> >>> </li>
> >>>
> >>> <li>
> >>> - A priority-65535 flow that allows any traffic that is considered
> >>> + A priority-65532 flow that allows any traffic that is considered
> >>> related to a committed flow in the connection tracker (e.g., an
> >>> ICMP Port Unreachable from a non-listening UDP port), as long
> >>> as the committed flow does not have <code>ct_label.blocked</code> set.
> >>> </li>
> >>>
> >>> <li>
> >>> - A priority-65535 flow that drops all traffic marked by the
> >>> + A priority-65532 flow that drops all traffic marked by the
> >>> connection tracker as invalid.
> >>> </li>
> >>>
> >>> <li>
> >>> - A priority-65535 flow that drops all traffic in the reply direction
> >>> + A priority-65532 flow that drops all traffic in the reply direction
> >>> with <code>ct_label.blocked</code> set meaning that the connection
> >>> should no longer be allowed due to a policy change. Packets
> >>> in the request direction are skipped here to let a newly created
> >>> @@ -682,7 +692,7 @@
> >>> </li>
> >>>
> >>> <li>
> >>> - A priority-65535 flow that allows IPv6 Neighbor solicitation,
> >>> + A priority-65532 flow that allows IPv6 Neighbor solicitation,
> >>> Neighbor discover, Router solicitation, Router advertisement and MLD
> >>> packets.
> >>> </li>
> >>> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> >>> index 94fae5648a..dfe4225bb3 100644
> >>> --- a/northd/ovn-northd.c
> >>> +++ b/northd/ovn-northd.c
> >>> @@ -627,6 +627,7 @@ struct ovn_datapath {
> >>> bool has_stateful_acl;
> >>> bool has_lb_vip;
> >>> bool has_unknown;
> >>> + bool has_acls;
> >>>
> >>> /* IPAM data. */
> >>> struct ipam_info ipam_info;
> >>> @@ -4783,6 +4784,23 @@ ls_has_stateful_acl(struct ovn_datapath *od)
> >>> return false;
> >>> }
> >>>
> >>> +static bool
> >>> +ls_has_acls(struct ovn_datapath *od)
> >>> +{
> >>> + if (od->nbs->n_acls) {
> >>> + return true;
> >>> + }
> >>> +
> >>> + struct ovn_ls_port_group *ls_pg;
> >>> + HMAP_FOR_EACH (ls_pg, key_node, &od->nb_pgs) {
> >>> + if (ls_pg->nb_pg->n_acls) {
> >>> + return true;
> >>> + }
> >>> + }
> >>> +
> >>> + return false;
> >>> +}
> >>
> >> nit: ls_has_stateful_acl() and ls_has_acl() both walk the port groups
> >> associated to a logical switch. I wonder if it makes sense to combine
> >> the functions in one that sets both "has_acls" and "has_stateful_acl" in
> >> one go.
> >>
> >
> > Ack. Sounds good.
> >
> >>> +
> >>> /* Logical switch ingress table 0: Ingress port security - L2
> >>> * (priority 50).
> >>> * Ingress table 1: Ingress port security - IP (priority 90 and 80)
> >>> @@ -5237,7 +5255,11 @@ build_acl_hints(struct ovn_datapath *od, struct hmap *lflows)
> >>> enum ovn_stage stage = stages[i];
> >>>
> >>> /* In any case, advance to the next stage. */
> >>> - ovn_lflow_add(lflows, od, stage, 0, "1", "next;");
> >>> + if (!od->has_acls && !od->has_lb_vip) {
> >>> + ovn_lflow_add(lflows, od, stage, UINT16_MAX, "1", "next;");
> >>> + } else {
> >>> + ovn_lflow_add(lflows, od, stage, 0, "1", "next;");
> >>> + }
> >>>
> >>> if (!od->has_stateful_acl && !od->has_lb_vip) {
> >>> continue;
> >>
> >> I didn't test this it out thoroughly but isn't it enough to change this
> >> whole block to:
> >>
> >> if (!od->has_stateful_acl && !od->has_lb_vip) {
> >> ovn_lflow_add(lflows, od, stage, UINT16_MAX, "1", "next;");
> >> continue;
> >> } else {
> >> ovn_lflow_add(lflows, od, stage, 0, "1", "next;");
> >> }
> >
> > Not really.
> >
> > If a logical switch has no ACLs or no LB VIPs we want to add
> > 65535-prio flow to advance the
> > packet to the next stage. But if a logical switch has any ACL (be it
> > allow, allow-related or drop)
> > we want to add a normal 0-priority flow to advance the packet to the next stage.
> >
>
> But if the switch has no stateful ACL or load balancer we might as well
> skip adding the flows that set CT hints, because they will not be used,
> I think.
Yes. I agree. We do the same with the current master and this patch
too doesn't change
that behavior.
This patch adds a 65535 flow to advance the pkt to the next stage in
ls_in_acl_hint if there are
no ACLs at all. If there is at least one ACL (be it allow, drop or
allow-related) associated with the logical switch,
priority-0 flow will be added to advance the pkt to the next stage.
So you want a 65535 flow to advance the pkt to next stage if there are
no stateful ACLs ?
Please note that in the stage - "ls_in_acl" we cannot add
65535-priority flow to advance the pkt to next stage
if there are stateless ACLs associated with logical switch (be it
allow or drop).
So I thought to be consistent for both the stages - ls_in_acl_hint and
ls_in_acl.
i.e If there are NO acls associated with a logical switch, then the
priority of the flow to advance the pkt to next stage
will be 65535, else it will be 0.
Please let me know if I've missed anything or if I misunderstood you ?
Thanks
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