[dpdk-dev] [PATCH] ixgbe: Discard SRIOV transparent vlan packet headers.

[Tom Kiely]

Hi,
     Yes I intend to look at the issue again considering the various 
points raised as soon as I can get some bandwidth.
Tom

On 02/10/2016 03:53 PM, Bruce Richardson wrote:
> On Thu, Dec 17, 2015 at 10:30:42AM +0000, Tom Kiely wrote:
>> Sorry for the delay in replying to this thread. I was on vacation for the
>> last 3 days. Please see inline for my comments.
>>
>> On 12/15/2015 02:37 PM, Ananyev, Konstantin wrote:
>>>> -----Original Message-----
>>>> From: Stephen Hemminger [mailto:stephen at networkplumber.org]
>>>> Sent: Monday, December 14, 2015 9:35 PM
>>>> To: Ananyev, Konstantin
>>>> Cc: Zhang, Helin; dev at dpdk.org; Tom Kiely
>>>> Subject: Re: [PATCH] ixgbe: Discard SRIOV transparent vlan packet headers.
>>>>
>>>> On Mon, 14 Dec 2015 19:57:10 +0000
>>>> "Ananyev, Konstantin" <konstantin.ananyev at intel.com> wrote:
>>>>
>>>>>> -----Original Message-----
>>>>>> From: Stephen Hemminger [mailto:stephen at networkplumber.org]
>>>>>> Sent: Monday, December 14, 2015 7:25 PM
>>>>>> To: Ananyev, Konstantin
>>>>>> Cc: Zhang, Helin; dev at dpdk.org; Tom Kiely
>>>>>> Subject: Re: [PATCH] ixgbe: Discard SRIOV transparent vlan packet headers.
>>>>>>
>>>>>> On Mon, 14 Dec 2015 19:12:26 +0000
>>>>>> "Ananyev, Konstantin" <konstantin.ananyev at intel.com> wrote:
>>>>>>
>>>>>>>> From: Stephen Hemminger [mailto:stephen at networkplumber.org]
>>>>>>>> Sent: Friday, December 11, 2015 4:59 PM
>>>>>>>> To: Zhang, Helin; Ananyev, Konstantin
>>>>>>>> Cc: dev at dpdk.org; Tom Kiely; Stephen Hemminger
>>>>>>>> Subject: [PATCH] ixgbe: Discard SRIOV transparent vlan packet headers.
>>>>>>>>
>>>>>>>> From: Tom Kiely <tkiely at brocade.com>
>>>>>>>>
>>>>>>>> SRIOV VFs support "transparent" vlans. Traffic from/to a VM
>>>>>>>> associated with a VF is tagged/untagged with the specified
>>>>>>>> vlan in a manner intended to be totally transparent to the VM.
>>>>>>>>
>>>>>>>> The vlan is specified by "ip link set <device> vf <n> vlan <v>".
>>>>>>>> The VM is not configured for any vlan on the VF and the VM
>>>>>>>> should never see these transparent vlan headers for that reason.
>>>>>>>>
>>>>>>>> However, in practice these vlan headers are being received by
>>>>>>>> the VM which discards the packets as that vlan is unknown to it.
>>>>>>>> The Linux kernel explicitly discards such vlan headers but DPDK
>>>>>>>> does not.
>>>>>>>> This patch mirrors the kernel behaviour for SRIOV VFs only
>>>>>>> I have few concerns about that approach:
>>>>>>>
>>>>>>> 1. I don't think vlan_tci info should *always* be stripped by vf RX routine.
>>>>>>> There could be configurations when that information might be needed by upper layer.
>>>>>>> Let say VF can be member of 2 or more VLANs and upper layer would like to have that information
>>>>>>> for further processing.
>>>>>>> Or special mirror VF, that does traffic snnoping, or something else.
>>>>>>> 2. Proposed implementation would introduce a slowdown for all VF RX routines.
>>>>>>> 3. From the description it seems like the aim is to clear VLAN information for the RX packet.
>>>>>>> Though the patch actually clears VLAN info only for the RX packet whose VLAN tag is not present inside SW copy of VFTA table.
>>>>>>> Which makes no much point to me:
>>>>>>> If VLAN is not present in HW VFTA table, then packet with that VLAN tag will be discarded by HW anyway.
>>>>>>> If it is present inside VFTA table (both SW & HW), then VLAN information would be preserved with and without the patch.
>>>>>>>
>>>>>>> If you need to clear VLAN information, why not to do it on the upper layer - inside your application itself?
>>>>>>> Either create some sort of wrapper around rx_burst(), or setup an RX call-back for your VF device.
>>>>>>>
>>>>>>> Konstantin
>>>>>> The aim is to get SRIOV to work when the transparent VLAN tag feature is used.
>>>>>> Please talk to the Linux driver team. Similar code exists there in ixgbevf_process_skb_fields.
>>>>> Ah ok, I realised what you are trying to achieve now:
>>>>> You setup HW VFTA[] from the PF, so from VF point of view SW copy of the VFTA[] remains unset.
>>>>> So HW will pass VLAN packet in, but then SW will clear VLAN tag.
>>>>> Ok, that clears #3 above, but I think #1,2 still remain.
>>>> On the host, what configured is a vlan tag per VF per guest
>>>>
>>>> Tom had more info in the original mail.
>>>>
>>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__permalink.gmane.org_gmane.comp.networking.dpdk.devel_28932&d=CwIBAg&c=IL_XqQWOjubgfqINi2jTzg&r=y34_c4q8RNNx7qnpFjr4XfT9FY_APdXBcXaTc590Mbg&m=Rz6uG1FpvzXdDtv7VB0vE89zfOcmUvpycGnC0-P5jUA&s=m7W6zcBeqmY2yhkyvv8qabYLSdGLUA0uYxm-wLHttC0&e=
>>>>
>>>>>> The other option is have a copy of all the receive logic which is only
>>>>>> used by VF code.
>>>>> Why that's the only option?
>>>>> Why can't you clear that VLAN information above the PMD layer?
>>>>> Keep/obtain a copy of VFTA[] somewhere on the upper layer,
>>>>> and do actual clear after rx_burst() returns?
>>>>> Konstantin
>>>> The problem is that the guest is supposed to not see the VLAN tags (it has no reason to),
>>>> but the hardware leaves a VLAN tag on there.
>>> Yes, I understand what you are trying to achieve.
>>>   What I am trying to say:
>>> 1. VLAN tag removing shouldn't be forced for all VFs.
>>> I think there are scenarios where existing behaviour (keeping vlan_tci and ol_flags intact) are what people need.
>>> One example would be mirror VF doing other VFs traffic snooping.
>>> Probably some other cases too.
>>> 2. The way you implemented it - it might cause a RX performance degradation (specially for VF).
>>> That's why I think it better to be implemented on top of PMD:
>>> i.e: some sort of wrapper that checks all packets returned by rx_burst() and clears vlan_tci if needed.
>>> That would give you desired behaviour and keep current implementation intact.
>>>
>>> Konstantin
>>>
>>>
>>> Hi Konstantin,
>>       To address your comments:
>>
>> (1) Only tags corresponding to VLANs that the client knows nothing about are
>> stripped. These tags are not intended to be seen by the client.
>> Maybe your concern would be addressed by disabling this functionality when
>> snooping in the same way that vlan offloading is disabled ?
>> I think further analysis is required here on our part.
>> (2) In relation to performance, for the non-SRIOV case, the hit is one "if"
>> per packet to test whether the functionality is enabled or not. We saw no
>> significant performance impact for the SRIOV case.
>> Moving the functionality above PMD is certainly something that we can
>> examine.
>>
>> Thanks,
>>
> Hi Tom, Stephen, Konstantin,
>
> have we reached a consensus on this patch? From what Tom says above on point 1,
> it seems to me like this some re-evaluation is going to be done and a patch for
> this issue will be sent again at a later date. Is that correct?
>
> Regards,
> /Bruce
>