On 6/11/2019 2:21 PM, Ilya Maximets wrote:
>> On Mon, Jun 3, 2019 at 4:42 PM Simon Horman <simon.horman at netronome.com>
>> wrote:
>>
>>> On Fri, May 31, 2019 at 02:45:25PM +0200, Simon Horman wrote:
>>>> Sorry Eli,
>>>>
>>>> I had missed this but I have it now.
>>>>
>>>> On Fri, 31 May 2019 at 09:35, Eli Britstein <elibr at mellanox.com> wrote:
>>>>
>>>>> Ping
>>>>> ------------------------------
>>>>> *From:* Eli Britstein <elibr at mellanox.com>
>>>>> *Sent:* Tuesday, May 21, 2019 3:11:51 PM
>>>>> *To:* dev at openvswitch.org; Simon Horman
>>>>> *Cc:* Roi Dayan; Paul Blakey; Eli Cohen; Eli Britstein
>>>>> *Subject:* [PATCH 1/1] netdev-tc-offloads: Support match on priority
>>> tags
>>>>> The logic by which a TC rule has a VLAN match is by the VLAN TCI field,
>>>>> either the VID, PCP or CFI are non-zero. For priority-tag packets
>>>>> there is a VLAN tag header with a zero VLAN TCI. Match on existence of
>>>>> VLAN header (TPID) regardless of TCI matching.
>>>>>
>>>>> Signed-off-by: Eli Britstein <elibr at mellanox.com>
>>>>> Reviewed-by: Roi Dayan <roid at mellanox.com>
>>> Thanks this seems to be a nice enhancement, applied to master.
>>>
>> Hey
>>
>> During some internal testing we found that this patch broke some
>> set-L4 cases when using tc-offloads. Setting up a simple bridge and
>> flow rule looking something like this:
>>
>> ovs-vsctl show:
>> Bridge "br0"
>> Port "br0"
>> Interface "br0"
>> type: internal
>> Port "veth0r"
>> Interface "veth0r"
>> Port "veth1r"
>> Interface "veth1r"
>>
>> ovs-ofctl add-flow br0 cookie=1,table=0,in_port=veth0r,tcp,tcp_dst=4000, \
>> actions=set_field:4002-\>tcp_dst,output:veth1r
>>
>> and then sending VLAN tagged traffic leads to packets not egressing the
>> port.
>>
>> The TC rule that gets installed looks like this:
>> vlan_ethtype ip
> There must be no vlan_ethertype match just because there was no such field
> match
> in the original flow.
>
> Looking at the code I see that it checks for key.tpid which makes no sense,
> because it must check for the mask first. At least there must be something
> like
> this:
>
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 2af0f10d9..7e7160426 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -1146,9 +1146,11 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
> *match,
> }
> mask->mpls_lse[0] = 0;
>
> - if (eth_type_vlan(key->vlans[0].tpid)) {
> + if (mask->vlans[0].tpid && eth_type_vlan(key->vlans[0].tpid)) {
> flower.key.encap_eth_type[0] = flower.key.eth_type;
> + flower.mask.encap_eth_type[0] = flower.mask.eth_type;
> flower.key.eth_type = key->vlans[0].tpid;
> + flower.mask.eth_type = mask->vlans[0].tpid;
> }
> if (mask->vlans[0].tci) {
> ovs_be16 vid_mask = mask->vlans[0].tci & htons(VLAN_VID_MASK);
> @@ -1179,9 +1181,11 @@ netdev_tc_flow_put(struct netdev *netdev, struct match
> *match,
> }
> }
>
> - if (eth_type_vlan(key->vlans[1].tpid)) {
> + if (mask->vlans[1].tpid && eth_type_vlan(key->vlans[1].tpid)) {
> flower.key.encap_eth_type[1] = flower.key.encap_eth_type[0];
> + flower.mask.encap_eth_type[1] = flower.mask.encap_eth_type[0];
> flower.key.encap_eth_type[0] = key->vlans[1].tpid;
> + flower.mask.encap_eth_type[0] = mask->vlans[1].tpid;
> }
> if (mask->vlans[1].tci) {
> ovs_be16 vid_mask = mask->vlans[1].tci & htons(VLAN_VID_MASK);
> ---
>
> I'm not an expert here, so it needs checking and review form someone more
> familiar with flower.
This seems correct. Need to test it.
>
>
>> eth_type ipv4
>> ip_proto tcp
>> dst_port 4000
>> ip_flags nofrag
>> not_in_hw
>> action order 1: pedit action pipe keys 1
>> index 1 ref 1 bind 1 installed 3 sec used 3 sec
>> key #0 at tcp+0: val 00000fa2 mask ffff0000
>> Action statistics:
>> Sent 368 bytes 8 pkt (dropped 0, overlimits 0 requeues 0)
>> backlog 0b 0p requeues 0
>>
>> action order 2: csum (tcp) action pipe
>> index 1 ref 1 bind 1 installed 3 sec used 3 sec
>> Action statistics:
>> Sent 368 bytes 8 pkt (dropped 8, overlimits 0 requeues 0)
>> backlog 0b 0p requeues 0
>>
>> action order 3: mirred (Egress Redirect to device ens260np1) stolen
>> index 1 ref 1 bind 1 installed 3 sec used 3 sec
>> Action statistics:
>> Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>> backlog 0b 0p requeues 0
>> cookie 8eb331199f4d41dc739e769a2f79f1ba
>>
>> Note the drop counters in the csum(tcp) section. Before the patch the rule
>> looks like this:
>> eth_type ipv4
>> ip_proto tcp
>> dst_port 4000
>> ip_flags nofrag
>> not_in_hw
>> action order 1: pedit action pipe keys 1
>> index 1 ref 1 bind 1 installed 3 sec used 3 sec
>> key #0 at tcp+0: val 00000fa2 mask ffff0000
>> Action statistics:
>> Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>> backlog 0b 0p requeues 0
>>
>> action order 2: csum (tcp) action pipe
>> index 1 ref 1 bind 1 installed 3 sec used 3 sec
>> Action statistics:
>> Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>> backlog 0b 0p requeues 0
>>
>> action order 3: mirred (Egress Redirect to device ens260np1) stolen
>> index 1 ref 1 bind 1 installed 3 sec used 3 sec
>> Action statistics:
>> Sent 0 bytes 0 pkt (dropped 0, overlimits 0 requeues 0)
>> backlog 0b 0p requeues 0
>> cookie 83b3464d0c47e3747956bca7e8857a53
>>
>> Here I suspect there may be a different issue as none of the counters
>> increase
>> so the TC rule is probably never hit (confirmed by the datapath rule timing
>> out even with continuous traffic), but at least packets are correctly
>> egressing via the userspace fallback path. It looks like this patch is
>> installing a rule in TC that does get hit, but is now mysteriously
>> dropping packets in the middle of the action processing pipeline.
>>
>> Another strange observation is that this only seems to happen when setting
>> one of the layer4 fields, setting layer3 seems to work as expected.
I tried setting such tc rule (using tc tool), and it works OK for me.
Make sure you have commit 2ecba2d1e45b ("net: sched: act_csum: Fix csum
calc for tagged packets") in your kernel.
>>
>> Regards
>> Louis Peens
>>
>>
>>> _______________________________________________
>>> dev mailing list
>>> dev at openvswitch.org
>>> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=02%7C01%7Celibr%40mellanox.com%7Cc6de245c204d41f12c4408d6ee5ef971%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636958489017862941&sdata=Z4b89HrFIv%2F%2BMnGVuyUqACYLBag%2Fmxi8YYt8i0omc3A%3D&reserved=0
>>>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
