Re: [dpdk-dev] [PATCH v2] net/ixgbe: fix VLAN strip setting fail for per port

["Dai, Wei" <wei.dai@intel.com>]

As 18.05 release is coming soon.
I'd like to submit http://dpdk.org/dev/patchwork/patch/40226/
in reply to yanglong's v2 patch for quick review and validation.
Thanks for your understanding.

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Dai, Wei
> Sent: Friday, May 18, 2018 11:06 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Yanglong
> <yanglong.wu@intel.com>; dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2] net/ixgbe: fix VLAN strip setting fail for
> per port
> 
> > -----Original Message-----
> > From: Zhang, Qi Z
> > Sent: Friday, May 18, 2018 8:37 PM
> > To: Dai, Wei <wei.dai@intel.com>; Wu, Yanglong
> > <yanglong.wu@intel.com>; dev@dpdk.org
> > Subject: RE: [PATCH v2] net/ixgbe: fix VLAN strip setting fail for per
> > port
> >
> > Hi Daiwei:
> >
> > > -----Original Message-----
> > > From: Dai, Wei
> > > Sent: Friday, May 18, 2018 7:07 PM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>; Wu, Yanglong
> > > <yanglong.wu@intel.com>; dev@dpdk.org
> > > Subject: RE: [PATCH v2] net/ixgbe: fix VLAN strip setting fail for
> > > per port
> > >
> > > > -----Original Message-----
> > > > From: Zhang, Qi Z
> > > > Sent: Friday, May 18, 2018 3:46 PM
> > > > To: Wu, Yanglong <yanglong.wu@intel.com>; dev@dpdk.org
> > > > Cc: Dai, Wei <wei.dai@intel.com>
> > > > Subject: RE: [PATCH v2] net/ixgbe: fix VLAN strip setting fail for
> > > > per port
> > > >
> > > > > -----Original Message-----
> > > > > From: Wu, Yanglong
> > > > > Sent: Friday, May 18, 2018 3:24 PM
> > > > > To: dev@dpdk.org
> > > > > Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Dai, Wei
> > > > > <wei.dai@intel.com>; Wu, Yanglong <yanglong.wu@intel.com>
> > > > > Subject: [PATCH v2] net/ixgbe: fix VLAN strip setting fail for
> > > > > per port
> > > > >
> > > > > rxq->offload should synchronize with dev_conf
> > > > >
> > > > > Fixes: 860a94d3c692 ("net/ixgbe: support VLAN strip per queue
> > > > > offloading in
> > > > > VF")
> > > > > Signed-off-by: Yanglong Wu <yanglong.wu@intel.com>
> > > >
> > > > Acked-by: Qi Zhang <qi.z.zhang@intel.com>
> > >
> > > The release date is coming soon.
> > > Sorry, I have to NACK it.
> > > VLAN strip is per-queue feature,
> > > If it is disabled on port level, it still can be enabled on queue level.
> > > So the else branches still should be removed.
> >
> > Remove the else branch will not disable all queues if some queue is
> > enabled at queue configure level, I think this is not user expected.
> > The purpose of i40e_vlan_offload_set here is to disable all queue's
> > vlan strip, though vlan strip is per queue offload and some queue may
> > be enabled at queue configure level, I don't know why we can't disable
> > them in this function.
> >
> > Thanks
> > Qi
> 
> As VLAN_STRIP has already been advertised to per-queue offloading on
> ixgbe 82599/X540/X550.
> The else branches will break this per-queue feature.
> 
> The issues is from the testpmd command "vlan set strip on <port_id>"
> Which is meant to enable/disable VLAN_STRIP on whole port on the fly.
> The call stack is as follows:
> rx_vlan_strip_set(portid_t port_id, int on)
> 	rte_eth_dev_set_vlan_offload(port_id, vlan_offload); //modify
> dev->data->dev_conf.rxmode.offloads
> 		dev->dev_ops->vlan_offload_set(dev, mask)
> 			ixgbe_vlan_offload_set(dev, mask)
> 				ixgbe_vlan_hw_strip_config(struct rte_eth_dev *dev)
> 					ixgbe_vlan_hw_strip_bitmap_set(struct rte_eth_dev
> *dev, uint16_t queue, bool on)
> 
> As the VLAN_STRIP is per-queue capability, ixgbe_vlan_hw_strip_config()
> only get it from rxq->offloads which hasn't been update in
> rte_eth_dev_set_vlan_offload(port_id, vlan_offload);
> 
> And the ixgbe_vlan_offload_set() is also be called by ixgbe_dev_start() which
> is normal called after dev_configure() and queue_setup().
> 
> These are 2 different path to config VLAN_STRIP.
> So we can add a function ixgbe_vlan_offload_config() to let them go
> different way as follows:
> 
> dev->dev_ops->vlan_offload_set(dev, mask) -> point to new function
> dev->ixgbe_vlan_offload_config()
> 		ixgbe_vlan_offload_config(dev, mask) {
> 			if vlan_strip is configured on whole port {
> 				update dev->data->dev_conf.rxmode.offloads
> 				update rxq->offloads on all queues
> 			}
> 			Ixgbe_vlan_offload_set()
> 		}
> 
> Ixgbe_dev_start()
> 	ixgbe_vlan_offload_set()
>