On 6/14/2019 5:40 PM, Louis Peens wrote:
On Wed, Jun 12, 2019 at 11:53 AM Eli Britstein <[email protected]<mailto:[email protected]>> wrote: 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<https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fnetronome.com&data=02%7C01%7Celibr%40mellanox.com%7C139fdfc2545245c825a208d6f0d6389d%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636961200227223260&sdata=nO14Y2PABBRpM4JGuW26l5Fg5oP47m0VDz10xGCcheI%3D&reserved=0>> >> 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<http://mellanox.com>> wrote: >>>> >>>>> Ping >>>>> ------------------------------ >>>>> *From:* Eli Britstein <elibr at mellanox.com<http://mellanox.com>> >>>>> *Sent:* Tuesday, May 21, 2019 3:11:51 PM >>>>> *To:* dev at >>>>> openvswitch.org<https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopenvswitch.org&data=02%7C01%7Celibr%40mellanox.com%7C139fdfc2545245c825a208d6f0d6389d%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636961200227223260&sdata=t3dIZiyDY7ZUmrjZQd%2FwCQ3QSCxsQGhVa8bevoD1zwQ%3D&reserved=0>; >>>>> 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<http://mellanox.com>> >>>>> Reviewed-by: Roi Dayan <roid at mellanox.com<http://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. Hey, thanks for the input so far. Currently I'm still seeing this issue, even with the above mentioned csum patch included, but I suspect the issue I'm seeing is probably related. I can reproduce the issue using TC rules only, so this moves the blame from OVS. In case somebody wants do dig deeper, here is my TC setup: echo "Create veth interfaces" ip link add $V0r type veth peer name $V0n ip link add $V1r type veth peer name $V1n echo "Recreate qdiscs" tc qdisc del dev $V0r handle ffff: ingress tc qdisc del dev $V1r handle ffff: ingress tc qdisc add dev $V0r handle ffff: ingress tc qdisc add dev $V1r handle ffff: ingress echo "Add flow rules" match="802.1Q flower vlan_ethtype ipv4 ip_proto tcp dst_port 4000 ip_flags nofrag" action="pedit ex munge tcp dport set 4002 pipe csum tcp " action+="pipe mirred egress redirect dev $V1r" tc filter add dev $V0r parent ffff: protocol ${match} action ${action} The pcap I use looks like this: reading from file tcp_vlan.pcap, link-type EN10MB (Ethernet) 15:16:55.293780 52:12:ef:32:45:ab > 54:a2:32:ef:54:1b, ethertype 802.1Q (0x8100), length 64: vlan 100, p 2, ethertype IPv4, (tos 0xc, ttl 64, id 1, offset 0, flags [none], proto TCP (6), length 46) 1.2.3.4.3000 > 5.6.7.8.4000: Flags [S], cksum 0x2529 (correct), seq 0:6, win 8192, length 6 With the above config I see the same behaviour where the packets are dropped at the csum action part. Further instrumentation in the act_csum module shows that there is skb corruption happening - the 4002 value that is suppose to end up in the TCP destination field ends up in the IP-header length field. This makes me think that there is possibly another issue similar to the act_csum one that needs to be fixed in act_pedit but I've not found that yet. This was on the linux stable tree at tag v5.2-rc4. Hope somebody here with more knowledge on the kernel tc code can provide some input. Thanks Can you try this? it is still not final (and not yet accepted), but maybe it fixes your issue https://lore.kernel.org/netdev/830aa8f07b528b50b212c01d53de6ec651500535.1559322531.git.dcara...@redhat.com/ Louis Peens >> >> Regards >> Louis Peens >> >> >>> _______________________________________________ >>> dev mailing list >>> dev at >>> openvswitch.org<https://eur03.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopenvswitch.org&data=02%7C01%7Celibr%40mellanox.com%7C139fdfc2545245c825a208d6f0d6389d%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636961200227233250&sdata=IX1K1vVoW%2B1EH8WJzpZ8sY0jHbpH%2F5fkSj5HE3rAfvU%3D&reserved=0> >>> 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<https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fmail.openvswitch.org%2Fmailman%2Flistinfo%2Fovs-dev&data=02%7C01%7Celibr%40mellanox.com%7C139fdfc2545245c825a208d6f0d6389d%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636961200227233250&sdata=7THf8MLACqYwOrnsT1xIOiBS1mfMJyBPOM5ijl%2Fwr44%3D&reserved=0> >>> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
