> 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.
> 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.
>
> Regards
> Louis Peens
>
>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
