[dpdk-stable] [PATCH] net/bonding: do not inherit slave device configuration

[dpdk-stable] [PATCH] net/bonding: do not inherit slave device configuration

[Andrew Rybchenko]

Bonding device should control bonded devices configuration.

Also avoid usage of slave's data->dev_conf.

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

Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 drivers/net/bonding/rte_eth_bond_pmd.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 707a0f3cdd..4f0e83205d 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1679,6 +1679,7 @@ int
 slave_configure(struct rte_eth_dev *bonded_eth_dev,
 		struct rte_eth_dev *slave_eth_dev)
 {
+	struct rte_eth_conf dev_conf;
 	struct bond_rx_queue *bd_rx_q;
 	struct bond_tx_queue *bd_tx_q;
 	uint16_t nb_rx_queues;
@@ -1693,34 +1694,34 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
 	/* Stop slave */
 	rte_eth_dev_stop(slave_eth_dev->data->port_id);
 
+	memset(&dev_conf, 0, sizeof(dev_conf));
+
 	/* Enable interrupts on slave device if supported */
 	if (slave_eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
-		slave_eth_dev->data->dev_conf.intr_conf.lsc = 1;
+		dev_conf.intr_conf.lsc = 1;
 
 	/* If RSS is enabled for bonding, try to enable it for slaves  */
 	if (bonded_eth_dev->data->dev_conf.rxmode.mq_mode & ETH_MQ_RX_RSS_FLAG) {
 		if (internals->rss_key_len != 0) {
-			slave_eth_dev->data->dev_conf.rx_adv_conf.rss_conf.rss_key_len =
+			dev_conf.rx_adv_conf.rss_conf.rss_key_len =
 					internals->rss_key_len;
-			slave_eth_dev->data->dev_conf.rx_adv_conf.rss_conf.rss_key =
+			dev_conf.rx_adv_conf.rss_conf.rss_key =
 					internals->rss_key;
 		} else {
-			slave_eth_dev->data->dev_conf.rx_adv_conf.rss_conf.rss_key = NULL;
+			dev_conf.rx_adv_conf.rss_conf.rss_key = NULL;
 		}
 
-		slave_eth_dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf =
+		dev_conf.rx_adv_conf.rss_conf.rss_hf =
 				bonded_eth_dev->data->dev_conf.rx_adv_conf.rss_conf.rss_hf;
-		slave_eth_dev->data->dev_conf.rxmode.mq_mode =
+		dev_conf.rxmode.mq_mode =
 				bonded_eth_dev->data->dev_conf.rxmode.mq_mode;
 	}
 
 	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;
+		dev_conf.rxmode.offloads |= DEV_RX_OFFLOAD_VLAN_FILTER;
 	else
-		slave_eth_dev->data->dev_conf.rxmode.offloads &=
-				~DEV_RX_OFFLOAD_VLAN_FILTER;
+		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;
@@ -1742,8 +1743,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
 
 	/* Configure device */
 	errval = rte_eth_dev_configure(slave_eth_dev->data->port_id,
-			nb_rx_queues, nb_tx_queues,
-			&(slave_eth_dev->data->dev_conf));
+			nb_rx_queues, nb_tx_queues, &dev_conf);
 	if (errval != 0) {
 		RTE_BOND_LOG(ERR, "Cannot configure slave device: port %u, err (%d)",
 				slave_eth_dev->data->port_id, errval);
-- 
2.17.1

Re: [dpdk-stable] [PATCH] net/bonding: do not inherit slave device configuration

[Ferruh Yigit]

On 11/19/2019 9:03 AM, Andrew Rybchenko wrote:
> Bonding device should control bonded devices configuration.
> 
> Also avoid usage of slave's data->dev_conf.
> 
> Fixes: 2efb58cbab6e ("bond: new link bonding library")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
>  drivers/net/bonding/rte_eth_bond_pmd.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index 707a0f3cdd..4f0e83205d 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -1679,6 +1679,7 @@ int
>  slave_configure(struct rte_eth_dev *bonded_eth_dev,
>  		struct rte_eth_dev *slave_eth_dev)
>  {
> +	struct rte_eth_conf dev_conf;
>  	struct bond_rx_queue *bd_rx_q;
>  	struct bond_tx_queue *bd_tx_q;
>  	uint16_t nb_rx_queues;
> @@ -1693,34 +1694,34 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>  	/* Stop slave */
>  	rte_eth_dev_stop(slave_eth_dev->data->port_id);
>  
> +	memset(&dev_conf, 0, sizeof(dev_conf));
> +
>  	/* Enable interrupts on slave device if supported */
>  	if (slave_eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
> -		slave_eth_dev->data->dev_conf.intr_conf.lsc = 1;
> +		dev_conf.intr_conf.lsc = 1;

I assume the original intention is making incremental changes to the existing
slave configuration, if so we should copy the 'slave_eth_dev->data->dev_conf' to
'dev_conf' before start updating it.

Re: [dpdk-stable] [PATCH] net/bonding: do not inherit slave device configuration

[Andrew Rybchenko]

On 11/19/19 3:18 PM, Ferruh Yigit wrote:
> On 11/19/2019 9:03 AM, Andrew Rybchenko wrote:
>> Bonding device should control bonded devices configuration.
>>
>> Also avoid usage of slave's data->dev_conf.
>>
>> Fixes: 2efb58cbab6e ("bond: new link bonding library")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>>  drivers/net/bonding/rte_eth_bond_pmd.c | 24 ++++++++++++------------
>>  1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
>> index 707a0f3cdd..4f0e83205d 100644
>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>> @@ -1679,6 +1679,7 @@ int
>>  slave_configure(struct rte_eth_dev *bonded_eth_dev,
>>  		struct rte_eth_dev *slave_eth_dev)
>>  {
>> +	struct rte_eth_conf dev_conf;
>>  	struct bond_rx_queue *bd_rx_q;
>>  	struct bond_tx_queue *bd_tx_q;
>>  	uint16_t nb_rx_queues;
>> @@ -1693,34 +1694,34 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>>  	/* Stop slave */
>>  	rte_eth_dev_stop(slave_eth_dev->data->port_id);
>>  
>> +	memset(&dev_conf, 0, sizeof(dev_conf));
>> +
>>  	/* Enable interrupts on slave device if supported */
>>  	if (slave_eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
>> -		slave_eth_dev->data->dev_conf.intr_conf.lsc = 1;
>> +		dev_conf.intr_conf.lsc = 1;
> I assume the original intention is making incremental changes to the existing
> slave configuration, if so we should copy the 'slave_eth_dev->data->dev_conf' to
> 'dev_conf' before start updating it.

The problem is that I don't understand how incremental changes
happen. It simply looks wrong or I don't understand something.
It looks like it is the only place in bonding where slave configuration
is done.

Re: [dpdk-stable] [PATCH] net/bonding: do not inherit slave device configuration

[Ferruh Yigit]

On 11/19/2019 12:40 PM, Andrew Rybchenko wrote:
> On 11/19/19 3:18 PM, Ferruh Yigit wrote:
>> On 11/19/2019 9:03 AM, Andrew Rybchenko wrote:
>>> Bonding device should control bonded devices configuration.
>>>
>>> Also avoid usage of slave's data->dev_conf.
>>>
>>> Fixes: 2efb58cbab6e ("bond: new link bonding library")
>>> Cc: stable@dpdk.org
>>>
>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>> ---
>>>  drivers/net/bonding/rte_eth_bond_pmd.c | 24 ++++++++++++------------
>>>  1 file changed, 12 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
>>> index 707a0f3cdd..4f0e83205d 100644
>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>> @@ -1679,6 +1679,7 @@ int
>>>  slave_configure(struct rte_eth_dev *bonded_eth_dev,
>>>  		struct rte_eth_dev *slave_eth_dev)
>>>  {
>>> +	struct rte_eth_conf dev_conf;
>>>  	struct bond_rx_queue *bd_rx_q;
>>>  	struct bond_tx_queue *bd_tx_q;
>>>  	uint16_t nb_rx_queues;
>>> @@ -1693,34 +1694,34 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>>>  	/* Stop slave */
>>>  	rte_eth_dev_stop(slave_eth_dev->data->port_id);
>>>  
>>> +	memset(&dev_conf, 0, sizeof(dev_conf));
>>> +
>>>  	/* Enable interrupts on slave device if supported */
>>>  	if (slave_eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
>>> -		slave_eth_dev->data->dev_conf.intr_conf.lsc = 1;
>>> +		dev_conf.intr_conf.lsc = 1;
>> I assume the original intention is making incremental changes to the existing
>> slave configuration, if so we should copy the 'slave_eth_dev->data->dev_conf' to
>> 'dev_conf' before start updating it.
> 
> The problem is that I don't understand how incremental changes
> happen. It simply looks wrong or I don't understand something.
> It looks like it is the only place in bonding where slave configuration
> is done.
> 
Hi Chas, Declan,

Can you please check the patch, and if possible comment on the initial intention
of the code?

Thanks,
ferruh

Re: [dpdk-stable] [dpdk-dev] [PATCH] net/bonding: do not inherit slave device configuration

[Chas Williams]

On 2019-11-19 07:40, Andrew Rybchenko wrote:
 > On 11/19/19 3:18 PM, Ferruh Yigit wrote:
 >> On 11/19/2019 9:03 AM, Andrew Rybchenko wrote:
 >>> Bonding device should control bonded devices configuration.
 >>>
 >>> Also avoid usage of slave's data->dev_conf.
 >>>
 >>> Fixes: 2efb58cbab6e ("bond: new link bonding library")
 >>> Cc: stable@dpdk.org
 >>>
 >>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
 >>> ---
 >>>   drivers/net/bonding/rte_eth_bond_pmd.c | 24 ++++++++++++------------
 >>>   1 file changed, 12 insertions(+), 12 deletions(-)
 >>>
 >>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c 
b/drivers/net/bonding/rte_eth_bond_pmd.c
 >>> index 707a0f3cdd..4f0e83205d 100644
 >>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
 >>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
 >>> @@ -1679,6 +1679,7 @@ int
 >>>   slave_configure(struct rte_eth_dev *bonded_eth_dev,
 >>>   		struct rte_eth_dev *slave_eth_dev)
 >>>   {
 >>> +	struct rte_eth_conf dev_conf;
 >>>   	struct bond_rx_queue *bd_rx_q;
 >>>   	struct bond_tx_queue *bd_tx_q;
 >>>   	uint16_t nb_rx_queues;
 >>> @@ -1693,34 +1694,34 @@ slave_configure(struct rte_eth_dev 
*bonded_eth_dev,
 >>>   	/* Stop slave */
 >>>   	rte_eth_dev_stop(slave_eth_dev->data->port_id);
 >>>
 >>> +	memset(&dev_conf, 0, sizeof(dev_conf));
 >>> +
 >>>   	/* Enable interrupts on slave device if supported */
 >>>   	if (slave_eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
 >>> -		slave_eth_dev->data->dev_conf.intr_conf.lsc = 1;
 >>> +		dev_conf.intr_conf.lsc = 1;
 >> I assume the original intention is making incremental changes to the 
existing
 >> slave configuration, if so we should copy the 
'slave_eth_dev->data->dev_conf' to
 >> 'dev_conf' before start updating it.
 >
 > The problem is that I don't understand how incremental changes
 > happen. It simply looks wrong or I don't understand something.
 > It looks like it is the only place in bonding where slave configuration
 > is done.
 >

I understand your confusion. Yes, it certainly looks like
slave_configure() is doing things wrong by directly modifying the slave's
data->dev_conf. If rte_eth_dev_configure() fails, the changes made do
get rolled back and become visible anyway despite the device having
failed to meet that configuration. rte_eth_dev_configure() handles the
rollback, but can't do anything in this case because it doesn't know
the device was directly modified.

You should make a copy of the dev_conf instead of starting from scratch.
There are other capabilities in there that bonding doesn't care about
but the application might.

Re: [dpdk-stable] [dpdk-dev] [PATCH] net/bonding: do not inherit slave device configuration

[Andrew Rybchenko]

On 12/8/19 6:44 PM, Chas Williams wrote:
> On 2019-11-19 07:40, Andrew Rybchenko wrote:
>> On 11/19/19 3:18 PM, Ferruh Yigit wrote:
>>> On 11/19/2019 9:03 AM, Andrew Rybchenko wrote:
>>>> Bonding device should control bonded devices configuration.
>>>>
>>>> Also avoid usage of slave's data->dev_conf.
>>>>
>>>> Fixes: 2efb58cbab6e ("bond: new link bonding library")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>> ---
>>>>   drivers/net/bonding/rte_eth_bond_pmd.c | 24 ++++++++++++------------
>>>>   1 file changed, 12 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
> b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>> index 707a0f3cdd..4f0e83205d 100644
>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>> @@ -1679,6 +1679,7 @@ int
>>>>   slave_configure(struct rte_eth_dev *bonded_eth_dev,
>>>>           struct rte_eth_dev *slave_eth_dev)
>>>>   {
>>>> +    struct rte_eth_conf dev_conf;
>>>>       struct bond_rx_queue *bd_rx_q;
>>>>       struct bond_tx_queue *bd_tx_q;
>>>>       uint16_t nb_rx_queues;
>>>> @@ -1693,34 +1694,34 @@ slave_configure(struct rte_eth_dev
> *bonded_eth_dev,
>>>>       /* Stop slave */
>>>>       rte_eth_dev_stop(slave_eth_dev->data->port_id);
>>>>
>>>> +    memset(&dev_conf, 0, sizeof(dev_conf));
>>>> +
>>>>       /* Enable interrupts on slave device if supported */
>>>>       if (slave_eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
>>>> -        slave_eth_dev->data->dev_conf.intr_conf.lsc = 1;
>>>> +        dev_conf.intr_conf.lsc = 1;
>>> I assume the original intention is making incremental changes to the
> existing
>>> slave configuration, if so we should copy the
> 'slave_eth_dev->data->dev_conf' to
>>> 'dev_conf' before start updating it.
>>
>> The problem is that I don't understand how incremental changes
>> happen. It simply looks wrong or I don't understand something.
>> It looks like it is the only place in bonding where slave configuration
>> is done.
>>
> 
> I understand your confusion. Yes, it certainly looks like
> slave_configure() is doing things wrong by directly modifying the slave's
> data->dev_conf. If rte_eth_dev_configure() fails, the changes made do
> get rolled back and become visible anyway despite the device having
> failed to meet that configuration. rte_eth_dev_configure() handles the
> rollback, but can't do anything in this case because it doesn't know
> the device was directly modified.
> 
> You should make a copy of the dev_conf instead of starting from scratch.
> There are other capabilities in there that bonding doesn't care about
> but the application might.

May application configure slave device directly (e.g. before
adding in bond) and bonding should respect it?
Are there usecases behind?
Of course, if an application configures both slaves directly
and via bonding device, it could understand the configuration,
but it looks very error-prone and over-complicated.
Wouldn't it be better if bonding device configuration is
passed to slaves?
May be the reason behind is that net/bonding does not forward
configuration to slaves except RSS configuration right now.
Is the behaviour documented anywhere?
Of course, any changes in the area would be behaviour change
which should be documented in release notes at least or
even go through deprecation process.

Re: [dpdk-stable] [dpdk-dev] [PATCH] net/bonding: do not inherit slave device configuration

[Chas Williams]

On 12/9/19 2:16 AM, Andrew Rybchenko wrote:
 > On 12/8/19 6:44 PM, Chas Williams wrote:
 >> On 2019-11-19 07:40, Andrew Rybchenko wrote:
 >>> On 11/19/19 3:18 PM, Ferruh Yigit wrote:
 >>>> On 11/19/2019 9:03 AM, Andrew Rybchenko wrote:
 >>>>> Bonding device should control bonded devices configuration.
 >>>>>
 >>>>> Also avoid usage of slave's data->dev_conf.
 >>>>>
 >>>>> Fixes: 2efb58cbab6e ("bond: new link bonding library")
 >>>>> Cc: stable@dpdk.org
 >>>>>
 >>>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
 >>>>> ---
 >>>>>     drivers/net/bonding/rte_eth_bond_pmd.c | 24 
++++++++++++------------
 >>>>>     1 file changed, 12 insertions(+), 12 deletions(-)
 >>>>>
 >>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
 >> b/drivers/net/bonding/rte_eth_bond_pmd.c
 >>>>> index 707a0f3cdd..4f0e83205d 100644
 >>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
 >>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
 >>>>> @@ -1679,6 +1679,7 @@ int
 >>>>>     slave_configure(struct rte_eth_dev *bonded_eth_dev,
 >>>>>             struct rte_eth_dev *slave_eth_dev)
 >>>>>     {
 >>>>> +    struct rte_eth_conf dev_conf;
 >>>>>         struct bond_rx_queue *bd_rx_q;
 >>>>>         struct bond_tx_queue *bd_tx_q;
 >>>>>         uint16_t nb_rx_queues;
 >>>>> @@ -1693,34 +1694,34 @@ slave_configure(struct rte_eth_dev
 >> *bonded_eth_dev,
 >>>>>         /* Stop slave */
 >>>>>         rte_eth_dev_stop(slave_eth_dev->data->port_id);
 >>>>>
 >>>>> +    memset(&dev_conf, 0, sizeof(dev_conf));
 >>>>> +
 >>>>>         /* Enable interrupts on slave device if supported */
 >>>>>         if (slave_eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
 >>>>> -        slave_eth_dev->data->dev_conf.intr_conf.lsc = 1;
 >>>>> +        dev_conf.intr_conf.lsc = 1;
 >>>> I assume the original intention is making incremental changes to the
 >> existing
 >>>> slave configuration, if so we should copy the
 >> 'slave_eth_dev->data->dev_conf' to
 >>>> 'dev_conf' before start updating it.
 >>>
 >>> The problem is that I don't understand how incremental changes
 >>> happen. It simply looks wrong or I don't understand something.
 >>> It looks like it is the only place in bonding where slave configuration
 >>> is done.
 >>>
 >>
 >> I understand your confusion. Yes, it certainly looks like
 >> slave_configure() is doing things wrong by directly modifying the 
slave's
 >> data->dev_conf. If rte_eth_dev_configure() fails, the changes made do
 >> get rolled back and become visible anyway despite the device having
 >> failed to meet that configuration. rte_eth_dev_configure() handles the
 >> rollback, but can't do anything in this case because it doesn't know
 >> the device was directly modified.
 >>
 >> You should make a copy of the dev_conf instead of starting from scratch.
 >> There are other capabilities in there that bonding doesn't care about
 >> but the application might.
 >
 > May application configure slave device directly (e.g. before
 > adding in bond) and bonding should respect it?

That's not the issue here. dev_conf contains rx_offload_capa,
tx_offload_capa et al. You can't just reset those to 0. Those are set
by the driver's PMD and the application, whether bonding or otherwise,
needs to be able to examine those.

 > Are there usecases behind?
 > Of course, if an application configures both slaves directly
 > and via bonding device, it could understand the configuration,
 > but it looks very error-prone and over-complicated.
 > Wouldn't it be better if bonding device configuration is
 > passed to slaves?

Bonding is a layer on top of the existing ports. It doesn't control
everything aspect of the bonded interfaces though. Bonding doesn't care
about the particular offloads a PMD may or may not support. That's an
application issue. For instance, your application may not be able
to support scatter/gather. That's not really bonding's concern.

 > May be the reason behind is that net/bonding does not forward
 > configuration to slaves except RSS configuration right now.

Bonding does try to forward some configuration to the slaves, atleast
where it makes sense. If you change the bonding MTU for instance, this
is forwarded to the slaves. It doesn't make sense to change the
bonding interface MTU without also changing the slave MTUs.

 > Is the behaviour documented anywhere?

Not that I am aware of.

 > Of course, any changes in the area would be behaviour change
 > which should be documented in release notes at least or
 > even go through deprecation process.

You current patch does propose a signficiant change because it clears
any existing configuration on the slave PMDs. If you simply copy the
slave's dev_conf first, you can avoid making a change.

Re: [dpdk-stable] [dpdk-dev] [PATCH] net/bonding: do not inherit slave device configuration

[Andrew Rybchenko]

On 12/9/19 4:36 PM, Chas Williams wrote:
> 
> 
> On 12/9/19 2:16 AM, Andrew Rybchenko wrote:
>> On 12/8/19 6:44 PM, Chas Williams wrote:
>>> On 2019-11-19 07:40, Andrew Rybchenko wrote:
>>>> On 11/19/19 3:18 PM, Ferruh Yigit wrote:
>>>>> On 11/19/2019 9:03 AM, Andrew Rybchenko wrote:
>>>>>> Bonding device should control bonded devices configuration.
>>>>>>
>>>>>> Also avoid usage of slave's data->dev_conf.
>>>>>>
>>>>>> Fixes: 2efb58cbab6e ("bond: new link bonding library")
>>>>>> Cc: stable@dpdk.org
>>>>>>
>>>>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>>> ---
>>>>>>     drivers/net/bonding/rte_eth_bond_pmd.c | 24
> ++++++++++++------------
>>>>>>     1 file changed, 12 insertions(+), 12 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>> index 707a0f3cdd..4f0e83205d 100644
>>>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>> @@ -1679,6 +1679,7 @@ int
>>>>>>     slave_configure(struct rte_eth_dev *bonded_eth_dev,
>>>>>>             struct rte_eth_dev *slave_eth_dev)
>>>>>>     {
>>>>>> +    struct rte_eth_conf dev_conf;
>>>>>>         struct bond_rx_queue *bd_rx_q;
>>>>>>         struct bond_tx_queue *bd_tx_q;
>>>>>>         uint16_t nb_rx_queues;
>>>>>> @@ -1693,34 +1694,34 @@ slave_configure(struct rte_eth_dev
>>> *bonded_eth_dev,
>>>>>>         /* Stop slave */
>>>>>>         rte_eth_dev_stop(slave_eth_dev->data->port_id);
>>>>>>
>>>>>> +    memset(&dev_conf, 0, sizeof(dev_conf));
>>>>>> +
>>>>>>         /* Enable interrupts on slave device if supported */
>>>>>>         if (slave_eth_dev->data->dev_flags & RTE_ETH_DEV_INTR_LSC)
>>>>>> -        slave_eth_dev->data->dev_conf.intr_conf.lsc = 1;
>>>>>> +        dev_conf.intr_conf.lsc = 1;
>>>>> I assume the original intention is making incremental changes to the
>>> existing
>>>>> slave configuration, if so we should copy the
>>> 'slave_eth_dev->data->dev_conf' to
>>>>> 'dev_conf' before start updating it.
>>>>
>>>> The problem is that I don't understand how incremental changes
>>>> happen. It simply looks wrong or I don't understand something.
>>>> It looks like it is the only place in bonding where slave configuration
>>>> is done.
>>>>
>>>
>>> I understand your confusion. Yes, it certainly looks like
>>> slave_configure() is doing things wrong by directly modifying the
> slave's
>>> data->dev_conf. If rte_eth_dev_configure() fails, the changes made do
>>> get rolled back and become visible anyway despite the device having
>>> failed to meet that configuration. rte_eth_dev_configure() handles the
>>> rollback, but can't do anything in this case because it doesn't know
>>> the device was directly modified.
>>>
>>> You should make a copy of the dev_conf instead of starting from scratch.
>>> There are other capabilities in there that bonding doesn't care about
>>> but the application might.
>>
>> May application configure slave device directly (e.g. before
>> adding in bond) and bonding should respect it?
> 
> That's not the issue here. dev_conf contains rx_offload_capa,
> tx_offload_capa et al. You can't just reset those to 0. Those are set
> by the driver's PMD and the application, whether bonding or otherwise,
> needs to be able to examine those.
> 
>> Are there usecases behind?
>> Of course, if an application configures both slaves directly
>> and via bonding device, it could understand the configuration,
>> but it looks very error-prone and over-complicated.
>> Wouldn't it be better if bonding device configuration is
>> passed to slaves?
> 
> Bonding is a layer on top of the existing ports. It doesn't control
> everything aspect of the bonded interfaces though. Bonding doesn't care
> about the particular offloads a PMD may or may not support. That's an
> application issue. For instance, your application may not be able
> to support scatter/gather. That's not really bonding's concern.

OK, I see now.

As I understand it requires from applications to be bonding-
aware. I.e. an application which works with a provided DPDK
port cannot accept bonding port since bonding port differs
a lot and requires application to configure slave ports first
to enable offloads etc. IMHO it is a drawback and limitation.

>> May be the reason behind is that net/bonding does not forward
>> configuration to slaves except RSS configuration right now.
> 
> Bonding does try to forward some configuration to the slaves, atleast
> where it makes sense. If you change the bonding MTU for instance, this
> is forwarded to the slaves. It doesn't make sense to change the
> bonding interface MTU without also changing the slave MTUs.
> 
>> Is the behaviour documented anywhere?
> 
> Not that I am aware of.
> 
>> Of course, any changes in the area would be behaviour change
>> which should be documented in release notes at least or
>> even go through deprecation process.
> 
> You current patch does propose a signficiant change because it clears
> any existing configuration on the slave PMDs. If you simply copy the
> slave's dev_conf first, you can avoid making a change.

Yes, it is pretty clear now. I'm just trying to understand
theory of how it is expected to work right now. Many
thanks for explanations.

I'll update the patch.

Does it make sense to address discuss above limitation
in the future?

Re: [dpdk-stable] [dpdk-dev] [PATCH] net/bonding: do not inherit slave device configuration

[Chas Williams]

On 12/9/19 9:31 AM, Andrew Rybchenko wrote:
 > On 12/9/19 4:36 PM, Chas Williams wrote:
 >>
 >>
 >> On 12/9/19 2:16 AM, Andrew Rybchenko wrote:
 >>> On 12/8/19 6:44 PM, Chas Williams wrote:
 >>>> On 2019-11-19 07:40, Andrew Rybchenko wrote:
 >>>>> On 11/19/19 3:18 PM, Ferruh Yigit wrote:
 >>>>>> On 11/19/2019 9:03 AM, Andrew Rybchenko wrote:
 >>>>>>> Bonding device should control bonded devices configuration.
 >>>>>>>
 >>>>>>> Also avoid usage of slave's data->dev_conf.
 >>>>>>>
 >>>>>>> Fixes: 2efb58cbab6e ("bond: new link bonding library")
 >>>>>>> Cc: stable@dpdk.org
 >>>>>>>
 >>>>>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
 >>>>>>> ---
 >>>>>>>       drivers/net/bonding/rte_eth_bond_pmd.c | 24
 >> ++++++++++++------------
 >>>>>>>       1 file changed, 12 insertions(+), 12 deletions(-)
 >>>>>>>
 >>>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
 >>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
 >>>>>>> index 707a0f3cdd..4f0e83205d 100644
 >>>>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
 >>>>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
 >>>>>>> @@ -1679,6 +1679,7 @@ int
 >>>>>>>       slave_configure(struct rte_eth_dev *bonded_eth_dev,
 >>>>>>>               struct rte_eth_dev *slave_eth_dev)
 >>>>>>>       {
 >>>>>>> +    struct rte_eth_conf dev_conf;
 >>>>>>>           struct bond_rx_queue *bd_rx_q;
 >>>>>>>           struct bond_tx_queue *bd_tx_q;
 >>>>>>>           uint16_t nb_rx_queues;
 >>>>>>> @@ -1693,34 +1694,34 @@ slave_configure(struct rte_eth_dev
 >>>> *bonded_eth_dev,
 >>>>>>>           /* Stop slave */
 >>>>>>>           rte_eth_dev_stop(slave_eth_dev->data->port_id);
 >>>>>>>
 >>>>>>> +    memset(&dev_conf, 0, sizeof(dev_conf));
 >>>>>>> +
 >>>>>>>           /* Enable interrupts on slave device if supported */
 >>>>>>>           if (slave_eth_dev->data->dev_flags & 
RTE_ETH_DEV_INTR_LSC)
 >>>>>>> -        slave_eth_dev->data->dev_conf.intr_conf.lsc = 1;
 >>>>>>> +        dev_conf.intr_conf.lsc = 1;
 >>>>>> I assume the original intention is making incremental changes to the
 >>>> existing
 >>>>>> slave configuration, if so we should copy the
 >>>> 'slave_eth_dev->data->dev_conf' to
 >>>>>> 'dev_conf' before start updating it.
 >>>>>
 >>>>> The problem is that I don't understand how incremental changes
 >>>>> happen. It simply looks wrong or I don't understand something.
 >>>>> It looks like it is the only place in bonding where slave 
configuration
 >>>>> is done.
 >>>>>
 >>>>
 >>>> I understand your confusion. Yes, it certainly looks like
 >>>> slave_configure() is doing things wrong by directly modifying the
 >> slave's
 >>>> data->dev_conf. If rte_eth_dev_configure() fails, the changes made do
 >>>> get rolled back and become visible anyway despite the device having
 >>>> failed to meet that configuration. rte_eth_dev_configure() handles the
 >>>> rollback, but can't do anything in this case because it doesn't know
 >>>> the device was directly modified.
 >>>>
 >>>> You should make a copy of the dev_conf instead of starting from 
scratch.
 >>>> There are other capabilities in there that bonding doesn't care about
 >>>> but the application might.
 >>>
 >>> May application configure slave device directly (e.g. before
 >>> adding in bond) and bonding should respect it?
 >>
 >> That's not the issue here. dev_conf contains rx_offload_capa,
 >> tx_offload_capa et al. You can't just reset those to 0. Those are set
 >> by the driver's PMD and the application, whether bonding or otherwise,
 >> needs to be able to examine those.
 >>
 >>> Are there usecases behind?
 >>> Of course, if an application configures both slaves directly
 >>> and via bonding device, it could understand the configuration,
 >>> but it looks very error-prone and over-complicated.
 >>> Wouldn't it be better if bonding device configuration is
 >>> passed to slaves?
 >>
 >> Bonding is a layer on top of the existing ports. It doesn't control
 >> everything aspect of the bonded interfaces though. Bonding doesn't care
 >> about the particular offloads a PMD may or may not support. That's an
 >> application issue. For instance, your application may not be able
 >> to support scatter/gather. That's not really bonding's concern.
 >
 > OK, I see now.
 >
 > As I understand it requires from applications to be bonding-
 > aware. I.e. an application which works with a provided DPDK
 > port cannot accept bonding port since bonding port differs
 > a lot and requires application to configure slave ports first
 > to enable offloads etc. IMHO it is a drawback and limitation.

I don't think you can hide all the messy bits of the bonding PMD by
adding more API. If your application can already correctly operate with
a particular port, then the bonding PMD doesn't do anything new. The
bonding PMD could be viewed as the set of the common capabilities of
the slaves.  If your application can't use the port without bonding,
adding bonding as an additional layer won't help.

Most applications will need some awareness of bonding to know that they
need to ignore what the bonding presents as capabilities because the
capabilities of the bonding PMD are really just those of the individual
ports which the application should have already vetted.

 >>> May be the reason behind is that net/bonding does not forward
 >>> configuration to slaves except RSS configuration right now.
 >>
 >> Bonding does try to forward some configuration to the slaves, atleast
 >> where it makes sense. If you change the bonding MTU for instance, this
 >> is forwarded to the slaves. It doesn't make sense to change the
 >> bonding interface MTU without also changing the slave MTUs.
 >>
 >>> Is the behaviour documented anywhere?
 >>
 >> Not that I am aware of.
 >>
 >>> Of course, any changes in the area would be behaviour change
 >>> which should be documented in release notes at least or
 >>> even go through deprecation process.
 >>
 >> You current patch does propose a signficiant change because it clears
 >> any existing configuration on the slave PMDs. If you simply copy the
 >> slave's dev_conf first, you can avoid making a change.
 >
 > Yes, it is pretty clear now. I'm just trying to understand
 > theory of how it is expected to work right now. Many
 > thanks for explanations.
 >
 > I'll update the patch.
 >
 > Does it make sense to address discuss above limitation
 > in the future?

For some features, possibly. One example of this issue is how to handle
reception of the LACPDUs. Some PMDs can only support promiscuous mode,
others are smarter and could potentially just receive the particular
multicast group. Is this something the application should configure? Is
this something that the bonding PMD should setup? If bonding does it,
does the user need fine-grained control?