diff mbox

[dpdk-dev] net/bonding: switch to new offloading flags

Message ID 20180313122444.160759-1-ferruh.yigit@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers

Checks

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

Commit Message

Ferruh Yigit March 13, 2018, 12:24 p.m. UTC 
  Switch from using deprecated bitfields in rxmode to offloads variable.

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 drivers/net/bonding/rte_eth_bond_api.c | 3 ++-
 drivers/net/bonding/rte_eth_bond_pmd.c | 9 +++++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

Comments

Radu Nicolau March 14, 2018, 10:40 a.m. UTC | #1 
On 3/13/2018 12:24 PM, Ferruh Yigit wrote:
> Switch from using deprecated bitfields in rxmode to offloads variable.
>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>
Acked-by: Radu Nicolau <radu.nicolau@intel.com>
Matan Azrad March 14, 2018, 12:50 p.m. UTC | #2 
Hi Ferruh

From: Ferruh Yigit, Sent: Tuesday, March 13, 2018 2:25 PM
> Switch from using deprecated bitfields in rxmode to offloads variable.
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
>  drivers/net/bonding/rte_eth_bond_api.c | 3 ++-
> drivers/net/bonding/rte_eth_bond_pmd.c | 9 +++++++--
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_api.c
> b/drivers/net/bonding/rte_eth_bond_api.c
> index f854b7375..669004fec 100644
> --- a/drivers/net/bonding/rte_eth_bond_api.c
> +++ b/drivers/net/bonding/rte_eth_bond_api.c
> @@ -194,7 +194,8 @@ slave_vlan_filter_set(uint16_t bonded_port_id,
> uint16_t slave_port_id)
>  	uint16_t first;
> 
>  	bonded_eth_dev = &rte_eth_devices[bonded_port_id];
> -	if (bonded_eth_dev->data->dev_conf.rxmode.hw_vlan_filter == 0)
> +	if ((bonded_eth_dev->data->dev_conf.rxmode.offloads &
> +			DEV_RX_OFFLOAD_VLAN_FILTER) == 0)
>  		return 0;
> 
>  	internals = bonded_eth_dev->data->dev_private;
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
> b/drivers/net/bonding/rte_eth_bond_pmd.c
> index c34c3251f..c18aca222 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -1818,8 +1818,13 @@ slave_configure(struct rte_eth_dev
> *bonded_eth_dev,
>  				bonded_eth_dev->data-
> >dev_conf.rxmode.mq_mode;
>  	}
> 
> -	slave_eth_dev->data->dev_conf.rxmode.hw_vlan_filter =
> -			bonded_eth_dev->data-
> >dev_conf.rxmode.hw_vlan_filter;
> +	if (bonded_eth_dev->data->dev_conf.rxmode.offloads &
> +			DEV_RX_OFFLOAD_VLAN_FILTER)
> +		slave_eth_dev->data->dev_conf.rxmode.offloads |=
> +				DEV_RX_OFFLOAD_VLAN_FILTER;
> +	else
> +		slave_eth_dev->data->dev_conf.rxmode.offloads &=
> +				~DEV_RX_OFFLOAD_VLAN_FILTER;
> 
>  	nb_rx_queues = bonded_eth_dev->data->nb_rx_queues;
>  	nb_tx_queues = bonded_eth_dev->data->nb_tx_queues;
> --
> 2.13.6

The bonding PMD is using internal variables to save the offload capabilities (Actually holds the offloads intersection set of all the bond slaves).
I think you are missing next:
	You should change the next variable types to uint64_t to support the new offload flags:
		internals->rx_offload_capa
		internals->tx_offload_capa

	You should add the new per queue offload variables to save the intersection set of it too:
		rx_queue_offload_capa
		tx_queue_offload_capa

Questions:
Have you an idea why bonding PMD doesn't adjust the slaves port configurations to the bonding port configuration like he does for slave queue configuration?
Is the responsibility to fill the slave port configuration structure for the application?

What do you think about next configuration checks (both per port and per queue)?
	Validate the actual bonding offloads with the bonding capability.
	Validate that the queue offloads includes all the port configured offloads.

Matan.
Ferruh Yigit March 20, 2018, 3:18 p.m. UTC | #3 
On 3/14/2018 12:50 PM, Matan Azrad wrote:
> Hi Ferruh
> 
> From: Ferruh Yigit, Sent: Tuesday, March 13, 2018 2:25 PM
>> Switch from using deprecated bitfields in rxmode to offloads variable.
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> ---
>>  drivers/net/bonding/rte_eth_bond_api.c | 3 ++-
>> drivers/net/bonding/rte_eth_bond_pmd.c | 9 +++++++--
>>  2 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/bonding/rte_eth_bond_api.c
>> b/drivers/net/bonding/rte_eth_bond_api.c
>> index f854b7375..669004fec 100644
>> --- a/drivers/net/bonding/rte_eth_bond_api.c
>> +++ b/drivers/net/bonding/rte_eth_bond_api.c
>> @@ -194,7 +194,8 @@ slave_vlan_filter_set(uint16_t bonded_port_id,
>> uint16_t slave_port_id)
>>  	uint16_t first;
>>
>>  	bonded_eth_dev = &rte_eth_devices[bonded_port_id];
>> -	if (bonded_eth_dev->data->dev_conf.rxmode.hw_vlan_filter == 0)
>> +	if ((bonded_eth_dev->data->dev_conf.rxmode.offloads &
>> +			DEV_RX_OFFLOAD_VLAN_FILTER) == 0)
>>  		return 0;
>>
>>  	internals = bonded_eth_dev->data->dev_private;
>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>> index c34c3251f..c18aca222 100644
>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>> @@ -1818,8 +1818,13 @@ slave_configure(struct rte_eth_dev
>> *bonded_eth_dev,
>>  				bonded_eth_dev->data-
>>> dev_conf.rxmode.mq_mode;
>>  	}
>>
>> -	slave_eth_dev->data->dev_conf.rxmode.hw_vlan_filter =
>> -			bonded_eth_dev->data-
>>> dev_conf.rxmode.hw_vlan_filter;
>> +	if (bonded_eth_dev->data->dev_conf.rxmode.offloads &
>> +			DEV_RX_OFFLOAD_VLAN_FILTER)
>> +		slave_eth_dev->data->dev_conf.rxmode.offloads |=
>> +				DEV_RX_OFFLOAD_VLAN_FILTER;
>> +	else
>> +		slave_eth_dev->data->dev_conf.rxmode.offloads &=
>> +				~DEV_RX_OFFLOAD_VLAN_FILTER;
>>
>>  	nb_rx_queues = bonded_eth_dev->data->nb_rx_queues;
>>  	nb_tx_queues = bonded_eth_dev->data->nb_tx_queues;
>> --
>> 2.13.6
> 
> The bonding PMD is using internal variables to save the offload capabilities (Actually holds the offloads intersection set of all the bond slaves).
> I think you are missing next:
> 	You should change the next variable types to uint64_t to support the new offload flags:
> 		internals->rx_offload_capa
> 		internals->tx_offload_capa
> 
> 	You should add the new per queue offload variables to save the intersection set of it too:
> 		rx_queue_offload_capa
> 		tx_queue_offload_capa

Thanks, I will update this v2.

> 
> Questions:
> Have you an idea why bonding PMD doesn't adjust the slaves port configurations to the bonding port configuration like he does for slave queue configuration?
> Is the responsibility to fill the slave port configuration structure for the application?
> 
> What do you think about next configuration checks (both per port and per queue)?
> 	Validate the actual bonding offloads with the bonding capability.
> 	Validate that the queue offloads includes all the port configured offloads.
> 
> Matan.
>
Ferruh Yigit March 22, 2018, 6:14 p.m. UTC | #4 
On 3/14/2018 12:50 PM, Matan Azrad wrote:
> Questions:
> Have you an idea why bonding PMD doesn't adjust the slaves port configurations to the bonding port configuration like he does for slave queue configuration?
> Is the responsibility to fill the slave port configuration structure for the application?
> 
> What do you think about next configuration checks (both per port and per queue)?
> 	Validate the actual bonding offloads with the bonding capability.
> 	Validate that the queue offloads includes all the port configured offloads.

I don't have answers for these, perhaps Declan or Radu can help.
Matan Azrad March 28, 2018, 7:31 p.m. UTC | #5 
Hi Declan, Radu

Please try to answer for the below questions.

From: Ferruh Yigit, Sent: Thursday, March 22, 2018 8:14 PM

> On 3/14/2018 12:50 PM, Matan Azrad wrote:

> > Questions:

> > Have you an idea why bonding PMD doesn't adjust the slaves port

> configurations to the bonding port configuration like it does for slave queue

> configuration?

> > Is the responsibility to fill the slave port configuration structure for the

> application?

> >

> > What do you think about next configuration checks (both per port and per

> queue)?

> > 	Validate the actual bonding offloads with the bonding capability.

> > 	Validate that the queue offloads includes all the port configured

> offloads.

> 

> I don't have answers for these, perhaps Declan or Radu can help.
diff mbox

Patch

diff --git a/drivers/net/bonding/rte_eth_bond_api.c b/drivers/net/bonding/rte_eth_bond_api.c
index f854b7375..669004fec 100644
--- a/drivers/net/bonding/rte_eth_bond_api.c
+++ b/drivers/net/bonding/rte_eth_bond_api.c
@@ -194,7 +194,8 @@  slave_vlan_filter_set(uint16_t bonded_port_id, uint16_t slave_port_id)
 	uint16_t first;
 
 	bonded_eth_dev = &rte_eth_devices[bonded_port_id];
-	if (bonded_eth_dev->data->dev_conf.rxmode.hw_vlan_filter == 0)
+	if ((bonded_eth_dev->data->dev_conf.rxmode.offloads &
+			DEV_RX_OFFLOAD_VLAN_FILTER) == 0)
 		return 0;
 
 	internals = bonded_eth_dev->data->dev_private;
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index c34c3251f..c18aca222 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1818,8 +1818,13 @@  slave_configure(struct rte_eth_dev *bonded_eth_dev,
 				bonded_eth_dev->data->dev_conf.rxmode.mq_mode;
 	}
 
-	slave_eth_dev->data->dev_conf.rxmode.hw_vlan_filter =
-			bonded_eth_dev->data->dev_conf.rxmode.hw_vlan_filter;
+	if (bonded_eth_dev->data->dev_conf.rxmode.offloads &
+			DEV_RX_OFFLOAD_VLAN_FILTER)
+		slave_eth_dev->data->dev_conf.rxmode.offloads |=
+				DEV_RX_OFFLOAD_VLAN_FILTER;
+	else
+		slave_eth_dev->data->dev_conf.rxmode.offloads &=
+				~DEV_RX_OFFLOAD_VLAN_FILTER;
 
 	nb_rx_queues = bonded_eth_dev->data->nb_rx_queues;
 	nb_tx_queues = bonded_eth_dev->data->nb_tx_queues;