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

[Andrew Rybchenko <arybchenko@solarflare.com>]

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.