On Fri, Jun 14, 2019 at 6:01 PM Eli Britstein <el...@mellanox.com> wrote:
> > On 6/14/2019 5:40 PM, Louis Peens wrote: > > > > On Wed, Jun 12, 2019 at 11:53 AM Eli Britstein <el...@mellanox.com> 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> >> wrote: >> >>>> >> >>>>> Ping >> >>>>> ------------------------------ >> >>>>> *From:* Eli Britstein <elibr at 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> >> >>>>> 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. >> > 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/ > Hey Sorry about the delay, I only got back to this now. Thanks for the pointer to the patches above, they do seem to be in the correct direction. Unfortunately they don't make a difference in my case since it only makes changes for L3 and not L4 (tcp src/dst in my test). I tried implementing something similar for L4 but could not do so with any success yet. I did find that both: "skb_network_offset" and "skb_transport_offset" are 0 in the "TCA_PEDIT_KEY_EX_HDR_TYPE_TCP/UDP" case statement in "pedit_skb_hdr_offset". This is inside the "skb_transport_header_was_set" check, so I would expect the transport_offset to be non-zero or at least different to the network_offset. That explains why pedit is changing the value at the incorrect offset, however I could not figure out what is causing this yet. That is the update from my side, maybe this triggers another idea from someone on what is going wrong. Thanks > > 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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev