diff mbox series

[v3] ethdev: support QinQ strip dynamic configuration

Message ID 1555653564-27263-1-git-send-email-viveksharma@marvell.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series [v3] ethdev: support QinQ strip dynamic configuration |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/mellanox-Performance-Testing success Performance Testing PASS
ci/intel-Performance-Testing success Performance Testing PASS

Commit Message

Vivek Kumar Sharma April 19, 2019, 5:59 a.m. UTC 
  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>
---
v3:
* Make patch as 'non-fix'.

v2:
* Use pointer to dereference dev->data->dev_conf.rxmode.offloads.
 
 lib/librte_ethdev/rte_ethdev.c | 55 +++++++++++++++++++++++-------------------
 lib/librte_ethdev/rte_ethdev.h |  5 +++-
 2 files changed, 34 insertions(+), 26 deletions(-)

Comments

Ferruh Yigit June 27, 2019, 11:08 a.m. UTC | #1 
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)?



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.
Andrew Rybchenko July 1, 2019, 10:07 a.m. UTC | #2 
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.
Ferruh Yigit July 1, 2019, 1:05 p.m. UTC | #3 
On 7/1/2019 11:07 AM, Andrew Rybchenko wrote:
> 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.

Not sure if "dynamic switching of offloads" required, but I agree we may need to
add "dynamically switchable offload" reporting for that.

> 
>> 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.

OK

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

+1

@Vivek, can you please send a new version with this update?
diff mbox series

Patch

diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
index 243beb4..3e770bc 100644
--- a/lib/librte_ethdev/rte_ethdev.c
+++ b/lib/librte_ethdev/rte_ethdev.c
@@ -2723,53 +2723,56 @@  rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask)
 	int mask = 0;
 	int cur, org = 0;
 	uint64_t orig_offloads;
+	uint64_t *dev_offloads;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
 
 	/* save original values in case of failure */
 	orig_offloads = dev->data->dev_conf.rxmode.offloads;
+	dev_offloads = &dev->data->dev_conf.rxmode.offloads;
 
 	/*check which option changed by application*/
 	cur = !!(offload_mask & ETH_VLAN_STRIP_OFFLOAD);
-	org = !!(dev->data->dev_conf.rxmode.offloads &
-		 DEV_RX_OFFLOAD_VLAN_STRIP);
+	org = !!(*dev_offloads & DEV_RX_OFFLOAD_VLAN_STRIP);
 	if (cur != org) {
 		if (cur)
-			dev->data->dev_conf.rxmode.offloads |=
-				DEV_RX_OFFLOAD_VLAN_STRIP;
+			*dev_offloads |= DEV_RX_OFFLOAD_VLAN_STRIP;
 		else
-			dev->data->dev_conf.rxmode.offloads &=
-				~DEV_RX_OFFLOAD_VLAN_STRIP;
+			*dev_offloads &= ~DEV_RX_OFFLOAD_VLAN_STRIP;
 		mask |= ETH_VLAN_STRIP_MASK;
 	}
 
 	cur = !!(offload_mask & ETH_VLAN_FILTER_OFFLOAD);
-	org = !!(dev->data->dev_conf.rxmode.offloads &
-		 DEV_RX_OFFLOAD_VLAN_FILTER);
+	org = !!(*dev_offloads & DEV_RX_OFFLOAD_VLAN_FILTER);
 	if (cur != org) {
 		if (cur)
-			dev->data->dev_conf.rxmode.offloads |=
-				DEV_RX_OFFLOAD_VLAN_FILTER;
+			*dev_offloads |= DEV_RX_OFFLOAD_VLAN_FILTER;
 		else
-			dev->data->dev_conf.rxmode.offloads &=
-				~DEV_RX_OFFLOAD_VLAN_FILTER;
+			*dev_offloads &= ~DEV_RX_OFFLOAD_VLAN_FILTER;
 		mask |= ETH_VLAN_FILTER_MASK;
 	}
 
 	cur = !!(offload_mask & ETH_VLAN_EXTEND_OFFLOAD);
-	org = !!(dev->data->dev_conf.rxmode.offloads &
-		 DEV_RX_OFFLOAD_VLAN_EXTEND);
+	org = !!(*dev_offloads & DEV_RX_OFFLOAD_VLAN_EXTEND);
 	if (cur != org) {
 		if (cur)
-			dev->data->dev_conf.rxmode.offloads |=
-				DEV_RX_OFFLOAD_VLAN_EXTEND;
+			*dev_offloads |= DEV_RX_OFFLOAD_VLAN_EXTEND;
 		else
-			dev->data->dev_conf.rxmode.offloads &=
-				~DEV_RX_OFFLOAD_VLAN_EXTEND;
+			*dev_offloads &= ~DEV_RX_OFFLOAD_VLAN_EXTEND;
 		mask |= ETH_VLAN_EXTEND_MASK;
 	}
 
+	cur = !!(offload_mask & ETH_QINQ_STRIP_OFFLOAD);
+	org = !!(*dev_offloads & DEV_RX_OFFLOAD_QINQ_STRIP);
+	if (cur != org) {
+		if (cur)
+			*dev_offloads |= DEV_RX_OFFLOAD_QINQ_STRIP;
+		else
+			*dev_offloads &= ~DEV_RX_OFFLOAD_QINQ_STRIP;
+		mask |= ETH_QINQ_STRIP_MASK;
+	}
+
 	/*no change*/
 	if (mask == 0)
 		return ret;
@@ -2778,7 +2781,7 @@  rte_eth_dev_set_vlan_offload(uint16_t port_id, int offload_mask)
 	ret = (*dev->dev_ops->vlan_offload_set)(dev, mask);
 	if (ret) {
 		/* hit an error restore  original values */
-		dev->data->dev_conf.rxmode.offloads = orig_offloads;
+		*dev_offloads = orig_offloads;
 	}
 
 	return eth_err(port_id, ret);
@@ -2788,23 +2791,25 @@  int
 rte_eth_dev_get_vlan_offload(uint16_t port_id)
 {
 	struct rte_eth_dev *dev;
+	uint64_t *dev_offloads;
 	int ret = 0;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	dev = &rte_eth_devices[port_id];
+	dev_offloads = &dev->data->dev_conf.rxmode.offloads;
 
-	if (dev->data->dev_conf.rxmode.offloads &
-	    DEV_RX_OFFLOAD_VLAN_STRIP)
+	if (*dev_offloads & DEV_RX_OFFLOAD_VLAN_STRIP)
 		ret |= ETH_VLAN_STRIP_OFFLOAD;
 
-	if (dev->data->dev_conf.rxmode.offloads &
-	    DEV_RX_OFFLOAD_VLAN_FILTER)
+	if (*dev_offloads & DEV_RX_OFFLOAD_VLAN_FILTER)
 		ret |= ETH_VLAN_FILTER_OFFLOAD;
 
-	if (dev->data->dev_conf.rxmode.offloads &
-	    DEV_RX_OFFLOAD_VLAN_EXTEND)
+	if (*dev_offloads & DEV_RX_OFFLOAD_VLAN_EXTEND)
 		ret |= ETH_VLAN_EXTEND_OFFLOAD;
 
+	if (*dev_offloads & DEV_RX_OFFLOAD_QINQ_STRIP)
+		ret |= DEV_RX_OFFLOAD_QINQ_STRIP;
+
 	return ret;
 }
 
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*/
 
 /* Definitions used for receive MAC address   */
@@ -965,7 +967,8 @@  struct rte_eth_conf {
 				 DEV_RX_OFFLOAD_TCP_CKSUM)
 #define DEV_RX_OFFLOAD_VLAN (DEV_RX_OFFLOAD_VLAN_STRIP | \
 			     DEV_RX_OFFLOAD_VLAN_FILTER | \
-			     DEV_RX_OFFLOAD_VLAN_EXTEND)
+			     DEV_RX_OFFLOAD_VLAN_EXTEND | \
+			     DEV_RX_OFFLOAD_QINQ_STRIP)
 
 /*
  * If new Rx offload capabilities are defined, they also must be