Re: [PATCH] net/bonding: make bonded device configure method re-entrant

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

Ivan,

could you rebase the patch, please, and send a new version.

Thanks,
Andrew.

On 10/18/22 13:34, Chas Williams wrote:
> 
> 
> On 10/17/22 10:10, Andrew Rybchenko wrote:
>> On 10/17/22 15:32, Chas Williams wrote:
>>> This appears to be correct. A minor comment inline.
>>>
>>> On 10/17/22 04:42, Andrew Rybchenko wrote:
>>>> Chas, Cornor, could you review the patch, please.
>>>>
>>>> Thanks,
>>>> Andrew.
>>>>
>>>> On 9/11/22 15:24, Ivan Malov wrote:
>>>>> According to the documentation, rte_eth_dev_configure()
>>>>> can be invoked repeatedly while in stopped state.
>>>>> The current implementation in the bonding driver
>>>>> allows for that (technically), but the user sees
>>>>> warnings which say that back-end devices have
>>>>> already been harnessed. Re-factor the code
>>>>> to have cleanup before each (re-)configure.
>>>>>
>>>>> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
>>>>> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>> ---
>>>>>   drivers/net/bonding/rte_eth_bond_pmd.c | 27 
>>>>> ++++++++++++++++++--------
>>>>>   1 file changed, 19 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c 
>>>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>> index d01c954296..92a33b45bd 100644
>>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>> @@ -2143,18 +2143,13 @@ bond_ethdev_stop(struct rte_eth_dev *eth_dev)
>>>>>       return 0;
>>>>>   }
>>>>> -int
>>>>> -bond_ethdev_close(struct rte_eth_dev *dev)
>>>>> +static void
>>>>> +bond_ethdev_cfg_cleanup(struct rte_eth_dev *dev)
>>>>>   {
>>>>>       struct bond_dev_private *internals = dev->data->dev_private;
>>>>>       uint16_t bond_port_id = internals->port_id;
>>>>> -    int skipped = 0;
>>>>>       struct rte_flow_error ferror;
>>>>> -
>>>>> -    if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>>>>> -        return 0;
>>>>> -
>>>>> -    RTE_BOND_LOG(INFO, "Closing bonded device %s", 
>>>>> dev->device->name);
>>>>> +    int skipped = 0;
>>>>>       /* Flush flows in all back-end devices before removing them */
>>>>>       bond_flow_ops.flush(dev, &ferror);
>>>>> @@ -2176,6 +2171,20 @@ bond_ethdev_close(struct rte_eth_dev *dev)
>>>>>               skipped++;
>>>>>           }
>>>>>       }
>>>>> +}
>>>>> +
>>>>> +int
>>>>> +bond_ethdev_close(struct rte_eth_dev *dev)
>>>>> +{
>>>>> +    struct bond_dev_private *internals = dev->data->dev_private;
>>>>> +
>>>>> +    if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>>>>> +        return 0;
>>>>> +
>>>>> +    RTE_BOND_LOG(INFO, "Closing bonded device %s", 
>>>>> dev->device->name);
>>>>> +
>>>>> +    bond_ethdev_cfg_cleanup(dev);
>>>>> +
>>>>>       bond_ethdev_free_queues(dev);
>>>>>       rte_bitmap_reset(internals->vlan_filter_bmp);
>>>>>       rte_bitmap_free(internals->vlan_filter_bmp);
>>>>> @@ -3606,6 +3615,8 @@ bond_ethdev_configure(struct rte_eth_dev *dev)
>>>>>       unsigned i, j;
>>>>> +    bond_ethdev_cfg_cleanup(dev);
>>>>> +
>>>
>>> You might want a space after the variable declaration section. It makes
>>> it easier to read.
>>
>> I can fix it on applying. May I add your ack?
> 
> Acked-by: Chas Williams <3chas3@gmail.com>
> 
>>>
>>>>>       /*
>>>>>        * If RSS is enabled, fill table with default values and
>>>>>        * set key to the value specified in port RSS configuration.
>>>>
>>