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>
> >> 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.
>
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
Louis Peens
>
> >>
> >> 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
