On 12.06.2019 12:53, Eli Britstein 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.
I'll send this as an official patch.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
