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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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
>