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

[Andrew Rybchenko <arybchenko@solarflare.com>]

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?