[ovs-dev] [PATCH ovn] northd: Support flow offloading for logical switches with no ACLs.
Dumitru Ceara
dceara at redhat.com
Mon May 10 14:52:12 UTC 2021
On 5/10/21 4:44 PM, Numan Siddique wrote:
> On Mon, May 10, 2021 at 9:29 AM Dumitru Ceara <dceara at redhat.com> wrote:
>>
>> On 5/10/21 11:58 AM, Numan Siddique wrote:
>>> 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 ?
>>
>> Yes, that was what I was thinking.
>>
>>>
>>> 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).
>>
>> I see now.
>>
>>>
>>> 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 ?
>>
>> I'm worried of the case when:
>>
>> LS1-ACLs:
>> - <match1>, prio X, then "allow-related"
>>
>> LS2-ACLs:
>> - <match2>, prio Y, then "allow"
>>
>> Even though the two logical switches are independent and the ACLs
>> defined on LS2 are "stateless" we'll still not be able to offload the
>> flows, right?
>>
>> What if reduce the maximum number of allowed ACL priorities? It's
>> currently (UINT16_MAX - 1000). Would it make sense to split it in half
>> and when a logical switch has only "allow" ACLs use the top half,
>> otherwise use the bottom half?
>>
>> That would ensure that flows for ACLs defined on LS2 always have higher
>> priority than flows defined for ACLs on LS1 (which match on ct_state).
>>
>
> This is an interesting idea. Thanks for the suggestion. Until now all
> the flows would match on ct_state even if one binding in the chassis
> belongs to a logical switch configured with ACLs.
>
> I think we can probably divide this in 2 stages.
> 1. What this patch does.
> 2. What you suggested.
>
> If that sounds good, I will work on your suggestion as a follow up patch ?
>
> I think having (1) would still help. I don't see too much of work for
> (2), but I might
> take some time to address it.
>
> Let me know if it works for you ?
Sure, sounds good to me. I'll have a look at the v2 of this patch soon.
Regards,
Dumitru
More information about the dev
mailing list