diff mbox

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

Message ID 20180518072341.27051-1-yanglong.wu@intel.com (mailing list archive)
State Changes Requested, archived
Delegated to: Helin Zhang
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Yanglong Wu May 18, 2018, 7:23 a.m. UTC 
  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>
---
v2:
rework as comments asked
---
 drivers/net/ixgbe/ixgbe_ethdev.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

Comments

Qi Zhang May 18, 2018, 7:45 a.m. UTC | #1 
> -----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>
Wei Dai May 18, 2018, 11:06 a.m. UTC | #2 
> -----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.
Qi Zhang May 18, 2018, 12:36 p.m. UTC | #3 
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
Ferruh Yigit May 18, 2018, 2:08 p.m. UTC | #4 
On 5/18/2018 1:36 PM, Zhang, Qi Z wrote:
> 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. 

rte_eth_dev_set_vlan_offload() API requests changes on VLAN related offloading
configuration, this API should cause:
1) Update configuration structure according API input
2) Apply new configuration to hardware

1) is done by API for port offloads.

2) is done by PMD "vlan_offload_set" dev_ops implementation.

But queue offload configuration struct is not updated anywhere, I guess this
patch is filling that gap. Otherwise wrong configuration will be applied to
hardware.


And there is still a defect with this patch, although I think it is minor, if
vlan is a queue offload and not enabled in configure but enabled for some set of
queues, when application asks to disable that offload, since rxmode.offloads
doesn't have it API will think it is already disable and don't send a request to
PMD, and this will keeps vlan offload enabled in some queues.
But comparing the what this patch fixes, I guess the remaining one is smaller
defect.


Perhaps it is better to update queue offload configuration too in API and PMD
only applies the configuration to hardware.

The rte_eth_dev_set_vlan_offload() API is not queue offload friendly, it may
disable/enable the offload for all queues if vlan offload is reported in queue
offload capability.

BUT, what do you think go with PMD update for this release, and if required put
a deprecation notice for the API and do the API change into next release?


> Thanks
> Qi
>
Wei Dai May 18, 2018, 3:05 p.m. UTC | #5 
> -----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 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()
Wei Dai May 18, 2018, 4:10 p.m. UTC | #6 
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()
>
diff mbox

Patch

diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c b/drivers/net/ixgbe/ixgbe_ethdev.c
index f5006bc94..94d28878a 100644
--- a/drivers/net/ixgbe/ixgbe_ethdev.c
+++ b/drivers/net/ixgbe/ixgbe_ethdev.c
@@ -2114,6 +2114,14 @@  ixgbe_vlan_hw_strip_config(struct rte_eth_dev *dev)
 		for (i = 0; i < dev->data->nb_rx_queues; i++) {
 			rxq = dev->data->rx_queues[i];
 			ctrl = IXGBE_READ_REG(hw, IXGBE_RXDCTL(rxq->reg_idx));
+
+			/* rxq->offload should synchronize with dev_conf*/
+			if (dev->data->dev_conf.rxmode.offloads &
+					DEV_RX_OFFLOAD_VLAN_STRIP)
+				rxq->offloads |= DEV_RX_OFFLOAD_VLAN_STRIP;
+			else
+				rxq->offloads &= ~DEV_RX_OFFLOAD_VLAN_STRIP;
+
 			if (rxq->offloads & DEV_RX_OFFLOAD_VLAN_STRIP) {
 				ctrl |= IXGBE_RXDCTL_VME;
 				on = TRUE;
@@ -5230,6 +5238,14 @@  ixgbevf_vlan_offload_set(struct rte_eth_dev *dev, int mask)
 	if (mask & ETH_VLAN_STRIP_MASK) {
 		for (i = 0; i < dev->data->nb_rx_queues; i++) {
 			rxq = dev->data->rx_queues[i];
+
+			/* rxq->offload should synchronize with dev_conf*/
+			if (dev->data->dev_conf.rxmode.offloads &
+						DEV_RX_OFFLOAD_VLAN_STRIP)
+				rxq->offloads |= DEV_RX_OFFLOAD_VLAN_STRIP;
+			else
+				rxq->offloads &= ~DEV_RX_OFFLOAD_VLAN_STRIP;
+
 			on = !!(rxq->offloads &	DEV_RX_OFFLOAD_VLAN_STRIP);
 			ixgbevf_vlan_strip_queue_set(dev, i, on);
 		}