Re: [dpdk-dev] [PATCH v3] ethdev: support QinQ strip dynamic configuration

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 27.06.2019 14:08, Ferruh Yigit wrote:
> On 4/19/2019 6:59 AM, viveksharma@marvell.com wrote:
>> From: Vivek Sharma <viveksharma@marvell.com>
>>
>> Enable missing support for runtime configuration (setting/getting)
>> of QinQ strip rx offload for a given ethdev.
>>
>> Signed-off-by: Vivek Sharma <viveksharma@marvell.com>
> <...>
>
>> diff --git a/lib/librte_ethdev/rte_ethdev.h b/lib/librte_ethdev/rte_ethdev.h
>> index 40a068f..c1792f4 100644
>> --- a/lib/librte_ethdev/rte_ethdev.h
>> +++ b/lib/librte_ethdev/rte_ethdev.h
>> @@ -550,11 +550,13 @@ struct rte_eth_rss_conf {
>>   #define ETH_VLAN_STRIP_OFFLOAD   0x0001 /**< VLAN Strip  On/Off */
>>   #define ETH_VLAN_FILTER_OFFLOAD  0x0002 /**< VLAN Filter On/Off */
>>   #define ETH_VLAN_EXTEND_OFFLOAD  0x0004 /**< VLAN Extend On/Off */
>> +#define ETH_QINQ_STRIP_OFFLOAD   0x0008 /**< QINQ Strip On/Off */
>>   
>>   /* Definitions used for mask VLAN setting */
>>   #define ETH_VLAN_STRIP_MASK   0x0001 /**< VLAN Strip  setting mask */
>>   #define ETH_VLAN_FILTER_MASK  0x0002 /**< VLAN Filter  setting mask*/
>>   #define ETH_VLAN_EXTEND_MASK  0x0004 /**< VLAN Extend  setting mask*/
>> +#define ETH_QINQ_STRIP_MASK   0x0008 /**< QINQ Strip  setting mask */
>>   #define ETH_VLAN_ID_MAX       0x0FFF /**< VLAN ID is in lower 12 bits*/
> On its own patch looks ok but a few high level questions:
>
> 1- Why we need this interim defines, instead of using actual offload values?
> Although changing this will be API/ABI breakage.
>
> 2- Why we have specific function to set vlan offloads, for other offloads the
> way is stop device and reconfigure it with new offload flags.
>
> 3- If devices can change offload configuration dynamically, do we need a generic
> API to alter the offload configs? (similar to these vlan APIs but more generic)?

If dynamic switching of offloads is really required, I think it would be
good if it is unified. If so, we can deprecate
rte_eth_dev_{s,g}et_vlan_offload() and implement it using the new one
to remove the old one from driver interface at least.
It requires reporting of dynamically switchable offloads on device and
queue level. Or just dynamically switchable offloads in assumption that
if the offload is supported on queue level and dynamically switchable,
it is dynamically switchable on queue level.

> Related to the patch, what do you think about following options:
> a)
> - Get this patch
> - Send a deprecation notice for 1) in this release
> - Next release remove these flags, which will be practically reverse this patch
> and more
>
> b)
> - Send a deprecation notice for 1) in this release
> - Next release update the APIs and a smaller/different version of this patch
> will be required.

I'm OK with both options, but I think it would be fair to go (a) to avoid
postponing of the feature too long.

The patch itself should update rte_eth_dev_{s,g}et_vlan_offload()
descriptions which enumerate switchable offloads.

Andrew.