net/bonding: fix create bonded device test failure

  Create bonded device test is failing due to improper initialisation in
bonded device configuration. which leads to crash while setting up queues.

The value of nb_rx_desc is checked if it is not in range of rx_desc_lim of
bonded device which fails.
This is due to "rx_desc_lim" is set to 0 as default value of bonded device
during bond_alloc().
Hence nb_rx_desc (1024) is > 0 and test fails.

Fix is to set the default values of rx_desc_lim of bonded device to
appropriate value.

Fixes: 2efb58cbab ("bond: new link bonding library")
Cc: stable@dpdk.org

Signed-off-by: Hari Kumar Vemula <hari.kumarx.vemula@intel.com>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
  

On 1/7/19 8:01 AM, Hari Kumar Vemula wrote:
> Create bonded device test is failing due to improper initialisation in
> bonded device configuration. which leads to crash while setting up queues.
> 
> The value of nb_rx_desc is checked if it is not in range of rx_desc_lim of
> bonded device which fails.
> This is due to "rx_desc_lim" is set to 0 as default value of bonded device
> during bond_alloc().
> Hence nb_rx_desc (1024) is > 0 and test fails.
> 
> Fix is to set the default values of rx_desc_lim of bonded device to
> appropriate value.

The default values for the bond device aren't known until the first
slave is added.  Can you explain your setup?  What PMD are you
using for testing?

> 
> Fixes: 2efb58cbab ("bond: new link bonding library")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Hari Kumar Vemula <hari.kumarx.vemula@intel.com>
> ---
>   drivers/net/bonding/rte_eth_bond_pmd.c | 11 +++++++----
>   1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 44deaf119..e0888e960 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -2225,6 +2225,11 @@ static void
>   bond_ethdev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>   {
>   	struct bond_dev_private *internals = dev->data->dev_private;
> +	struct rte_eth_desc_lim bond_lim = {
> +		.nb_max = UINT16_MAX,
> +		.nb_min = 0,
> +		.nb_align = 1,
> +	};
>   
>   	uint16_t max_nb_rx_queues = UINT16_MAX;
>   	uint16_t max_nb_tx_queues = UINT16_MAX;
> @@ -2263,10 +2268,8 @@ bond_ethdev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>   	memcpy(&dev_info->default_txconf, &internals->default_txconf,
>   	       sizeof(dev_info->default_txconf));
>   
> -	memcpy(&dev_info->rx_desc_lim, &internals->rx_desc_lim,
> -	       sizeof(dev_info->rx_desc_lim));
> -	memcpy(&dev_info->tx_desc_lim, &internals->tx_desc_lim,
> -	       sizeof(dev_info->tx_desc_lim));
> +	dev_info->rx_desc_lim = bond_lim;
> +	dev_info->tx_desc_lim = bond_lim;
>   
>   	/**
>   	 * If dedicated hw queues enabled for link bonding device in LACP mode
>
  

On 1/7/2019 6:44 PM, Chas Williams wrote:
> 
> 
> On 1/7/19 8:01 AM, Hari Kumar Vemula wrote:
>> Create bonded device test is failing due to improper initialisation in
>> bonded device configuration. which leads to crash while setting up queues.
>>
>> The value of nb_rx_desc is checked if it is not in range of rx_desc_lim of
>> bonded device which fails.
>> This is due to "rx_desc_lim" is set to 0 as default value of bonded device
>> during bond_alloc().
>> Hence nb_rx_desc (1024) is > 0 and test fails.
>>
>> Fix is to set the default values of rx_desc_lim of bonded device to
>> appropriate value.
> 
> The default values for the bond device aren't known until the first
> slave is added.  Can you explain your setup?  What PMD are you
> using for testing?

As far as I understand, 'rte_eth_rx_queue_setup()' is failing with bond eth
device since 'nb_rx_desc' should be less than 'dev_info.rx_desc_lim.nb_max' but
bonding sets 0 to 'nb_max'.

But not sure how to handle this from bonding point of view, this patch gives
most permissive values, but should bonding get these values from its slaves?

> 
>>
>> Fixes: 2efb58cbab ("bond: new link bonding library")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Hari Kumar Vemula <hari.kumarx.vemula@intel.com>
>> ---
>>   drivers/net/bonding/rte_eth_bond_pmd.c | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
>> index 44deaf119..e0888e960 100644
>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>> @@ -2225,6 +2225,11 @@ static void
>>   bond_ethdev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>>   {
>>   	struct bond_dev_private *internals = dev->data->dev_private;
>> +	struct rte_eth_desc_lim bond_lim = {
>> +		.nb_max = UINT16_MAX,
>> +		.nb_min = 0,
>> +		.nb_align = 1,
>> +	};
>>   
>>   	uint16_t max_nb_rx_queues = UINT16_MAX;
>>   	uint16_t max_nb_tx_queues = UINT16_MAX;
>> @@ -2263,10 +2268,8 @@ bond_ethdev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
>>   	memcpy(&dev_info->default_txconf, &internals->default_txconf,
>>   	       sizeof(dev_info->default_txconf));
>>   
>> -	memcpy(&dev_info->rx_desc_lim, &internals->rx_desc_lim,
>> -	       sizeof(dev_info->rx_desc_lim));
>> -	memcpy(&dev_info->tx_desc_lim, &internals->tx_desc_lim,
>> -	       sizeof(dev_info->tx_desc_lim));
>> +	dev_info->rx_desc_lim = bond_lim;
>> +	dev_info->tx_desc_lim = bond_lim;
>>   
>>   	/**
>>   	 * If dedicated hw queues enabled for link bonding device in LACP mode
>>
  

Hi Williams,


> -----Original Message-----
> From: Chas Williams [mailto:3chas3@gmail.com]
> Sent: Tuesday, January 8, 2019 12:15 AM
> To: Vemula, Hari KumarX <hari.kumarx.vemula@intel.com>; dev@dpdk.org
> Cc: Pattan, Reshma <reshma.pattan@intel.com>; Doherty, Declan
> <declan.doherty@intel.com>; Parthasarathy, JananeeX M
> <jananeex.m.parthasarathy@intel.com>; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/bonding: fix create bonded device test
> failure
> 
> 
> 
> On 1/7/19 8:01 AM, Hari Kumar Vemula wrote:
> > Create bonded device test is failing due to improper initialisation in
> > bonded device configuration. which leads to crash while setting up queues.
> >
> > The value of nb_rx_desc is checked if it is not in range of
> > rx_desc_lim of bonded device which fails.
> > This is due to "rx_desc_lim" is set to 0 as default value of bonded
> > device during bond_alloc().
> > Hence nb_rx_desc (1024) is > 0 and test fails.
> >
> > Fix is to set the default values of rx_desc_lim of bonded device to
> > appropriate value.
> 
> The default values for the bond device aren't known until the first slave is
> added.  Can you explain your setup?  What PMD are you using for testing?
> 
We ran link_bonding_autotest in test application and was failing the test_create_bonded_device as in below log.
Slaves are added during test suite setup and then bonded device is created as per ut.

./x86_64-native-linuxapp-clang/app/test -n 1 -c f
RTE>>link_bonding_autotest

+ ------------------------------------------------------- +
 + Test Suite : Link Bonding Unit Test Suite
 + ------------------------------------------------------- +
Invalid value for nb_rx_desc(=1024), should be: <= 0, = 0, and a product of 0
 + TestCase [ 0] : test_create_bonded_device failed

> >
> > Fixes: 2efb58cbab ("bond: new link bonding library")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Hari Kumar Vemula <hari.kumarx.vemula@intel.com>
> > ---
> >   drivers/net/bonding/rte_eth_bond_pmd.c | 11 +++++++----
> >   1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
> > b/drivers/net/bonding/rte_eth_bond_pmd.c
> > index 44deaf119..e0888e960 100644
> > --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> > +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> > @@ -2225,6 +2225,11 @@ static void
> >   bond_ethdev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info
> *dev_info)
> >   {
> >   	struct bond_dev_private *internals = dev->data->dev_private;
> > +	struct rte_eth_desc_lim bond_lim = {
> > +		.nb_max = UINT16_MAX,
> > +		.nb_min = 0,
> > +		.nb_align = 1,
> > +	};
> >
> >   	uint16_t max_nb_rx_queues = UINT16_MAX;
> >   	uint16_t max_nb_tx_queues = UINT16_MAX; @@ -2263,10 +2268,8
> @@
> > bond_ethdev_info(struct rte_eth_dev *dev, struct rte_eth_dev_info
> *dev_info)
> >   	memcpy(&dev_info->default_txconf, &internals->default_txconf,
> >   	       sizeof(dev_info->default_txconf));
> >
> > -	memcpy(&dev_info->rx_desc_lim, &internals->rx_desc_lim,
> > -	       sizeof(dev_info->rx_desc_lim));
> > -	memcpy(&dev_info->tx_desc_lim, &internals->tx_desc_lim,
> > -	       sizeof(dev_info->tx_desc_lim));
> > +	dev_info->rx_desc_lim = bond_lim;
> > +	dev_info->tx_desc_lim = bond_lim;
> >
> >   	/**
> >   	 * If dedicated hw queues enabled for link bonding device in LACP
> > mode
> >


Thanks,
Hari.
--------------------------------------------------------------
Intel Research and Development Ireland Limited
Registered in Ireland
Registered Office: Collinstown Industrial Park, Leixlip, County Kildare
Registered Number: 308263


This e-mail and any attachments may contain confidential material for the sole
use of the intended recipient(s). Any review or distribution by others is
strictly prohibited. If you are not the intended recipient, please contact the
sender and delete all copies.
  

> -----Original Message-----
> From: Vemula, Hari KumarX
> Sent: Monday, January 7, 2019 1:01 PM
> 
> 
> Signed-off-by: Hari Kumar Vemula <hari.kumarx.vemula@intel.com>
> ---
>  drivers/net/bonding/rte_eth_bond_pmd.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> -	memcpy(&dev_info->rx_desc_lim, &internals->rx_desc_lim,
> -	       sizeof(dev_info->rx_desc_lim));
> -	memcpy(&dev_info->tx_desc_lim, &internals->tx_desc_lim,
> -	       sizeof(dev_info->tx_desc_lim));
> +	dev_info->rx_desc_lim = bond_lim;
> +	dev_info->tx_desc_lim = bond_lim;


I relooked into this, these values should be filled from salve data like the way done for max_nb_rx_queues and max_nb_tx_queues
Existing code snippet:
if (slave_info.max_rx_queues < max_nb_rx_queues)
                          max_nb_rx_queues = slave_info.max_rx_queues;

 if (slave_info.max_tx_queues < max_nb_tx_queues)
                        max_nb_tx_queues = slave_info.max_tx_queues;

So, something like below you should add for rx/tx desc limit I guess.

if (slave_info.rx_desc_lim.nb_max < max_rx_desc_lim)
                                max_rx_desc_lim = slave_info.rx_desc_lim.nb_max;

if (slave_info.tx_desc_lim.nb_max < max_tx_desc_lim)
                                max_tx_desc_lim = slave_info.tx_desc_lim.nb_max;

dev_info->rx_desc_lim.nb_max = max_rx_desc_lim;
        dev_info->tx_desc_lim.nb_max = max_tx_desc_lim;

@Williams/Declan: Does this make sense?

Thanks,
Reshma