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

[Bruce Richardson <bruce.richardson@intel.com>]

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@networkplumber.org]
> >>Sent: Monday, December 14, 2015 9:35 PM
> >>To: Ananyev, Konstantin
> >>Cc: Zhang, Helin; dev@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@intel.com> wrote:
> >>
> >>>
> >>>>-----Original Message-----
> >>>>From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> >>>>Sent: Monday, December 14, 2015 7:25 PM
> >>>>To: Ananyev, Konstantin
> >>>>Cc: Zhang, Helin; dev@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@intel.com> wrote:
> >>>>
> >>>>>
> >>>>>>From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> >>>>>>Sent: Friday, December 11, 2015 4:59 PM
> >>>>>>To: Zhang, Helin; Ananyev, Konstantin
> >>>>>>Cc: dev@dpdk.org; Tom Kiely; Stephen Hemminger
> >>>>>>Subject: [PATCH] ixgbe: Discard SRIOV transparent vlan packet headers.
> >>>>>>
> >>>>>>From: Tom Kiely <tkiely@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.
> >>
> >>http://permalink.gmane.org/gmane.comp.networking.dpdk.devel/28932
> >>
> >>>>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