Re: [dpdk-dev] [PATCH] net/ixgbe: Strip SR-IOV transparent VLANs in VF

["Zhang, Qi Z" <qi.z.zhang@intel.com>]

Hi Robert:

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of
> robertshearman@gmail.com
> Sent: Saturday, August 25, 2018 12:35 AM
> To: dev@dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; Robert Shearman
> <robert.shearman@att.com>
> Subject: [dpdk-dev] [PATCH] net/ixgbe: Strip SR-IOV transparent VLANs in VF
> 
> From: Robert Shearman <robert.shearman@att.com>
> 
> SR-IOV VFs support "transparent" VLANs. Traffic from/to a VM associated with
> a VF has a VLAN tag inserted/stripped in a manner intended to be totally
> transparent to the VM.  On a Linux hypervisor the vlan can be specified by "ip
> link set <device> vf <n> vlan <v>".
> The VM VF driver is not configured to use any VLAN and the VM should never
> see the transparent VLAN 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 ixbge driver explicitly removes the VLAN in
> this case (presumably due to the hardware not being able to do this) but the
> DPDK driver does not.

I'm not quite understand this part.
What does explicitly remove the VLAN means?, 
DPDK also discard unmatched VLAN and strip vlan if vlan_strip is enabled what is the gap?
It will be better if you can give same examples

> 
> This patch mirrors the kernel driver behaviour by removing the VLAN on the VF
> side. This is done by checking the VLAN in the VFTA, where the hypervisor will
> have set the bit in the VFTA corresponding to the VLAN if transparent VLANs
> were being used for the VF. If the VLAN is set in the VFTA then it is known that
> it's a transparent VLAN case and so the VLAN is stripped from the mbuf. 

This is missing leading.
>From your code, I only saw vlan flag in ol_flag is stripped, but not VLAN is stripped.
I think vlan is always be stripped if we enable vlan strip on queue.

> To
> limit any potential performance impact on the PF data path, the RX path is split
> into PF and VF versions with the transparent VLAN stripping only done in the
> VF path. Measurements with our application show ~2% performance hit for
> the VF case and none for the PF case.
> 

...

> +/*
> + * Filter out unknown vlans resulting from use of transparent vlan.
> + *
> + * When a VF is configured to use transparent vlans then the VF can
> + * see this VLAN being set in the packet, meaning that the transparent
> + * property isn't preserved. Furthermore, when the VF is used in a
> + * guest VM then there's no way of knowing for sure that transparent
> + * VLAN is in use and what tag value has been configured. So work
> + * around this by removing the VLAN flag if the VF isn't interested in
> + * the VLAN tag.
> + */
> +static inline void
> +ixgbevf_trans_vlan_sw_filter_hdr(struct rte_mbuf *m,
> +				 const struct ixgbe_vfta *vfta)
> +{
> +	if (m->ol_flags & PKT_RX_VLAN) {
> +		uint16_t vlan = m->vlan_tci & 0xFFF;
> +
> +		if (!ixgbe_vfta_is_vlan_set(vfta, vlan))
> +			m->ol_flags &= ~PKT_RX_VLAN;
> +	}
> +}
> +

Ideally all driver's behavior should be consistent with the same configure.
if "transparent vlan" looks like a general feature, it may not only bind to VF or even just ixgbevf.  (what about i40evf?)
Otherwise, it should be handled in application , but not the driver.

...

> +		ixgbe_unknown_vlan_sw_filter_hdr(rx_pkts[pos + 3], vfta, rxq);

Where is ixgbe_unknown_vlan_sw_filter_hdr be defined? I saw it is only be used in ixgbe_rxtx_vec_neon.c, so assume there will be a compile error on that platform?

Regards
Qi