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

[Chas Williams <3chas3@gmail.com>]

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.