diff mbox series

net/bonding: make bonded device configure method re-entrant

Message ID 20220911122405.3995083-1-ivan.malov@oktetlabs.ru (mailing list archive)
State Superseded, archived
Delegated to: Andrew Rybchenko
Headers
Series net/bonding: make bonded device configure method re-entrant |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-testing warning apply patch failure
ci/Intel-compilation warning apply issues

Commit Message

Ivan Malov Sept. 11, 2022, 12:24 p.m. UTC 
  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(-)

Comments

Andrew Rybchenko Oct. 17, 2022, 8:42 a.m. UTC | #1 
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);
> +
>   	/*
>   	 * If RSS is enabled, fill table with default values and
>   	 * set key to the value specified in port RSS configuration.
Chas Williams Oct. 17, 2022, 12:32 p.m. UTC | #2 
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.

>>       /*
>>        * If RSS is enabled, fill table with default values and
>>        * set key to the value specified in port RSS configuration.
>
Andrew Rybchenko Oct. 17, 2022, 2:10 p.m. UTC | #3 
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?

> 
>>>       /*
>>>        * If RSS is enabled, fill table with default values and
>>>        * set key to the value specified in port RSS configuration.
>>
Chas Williams Oct. 18, 2022, 10:34 a.m. UTC | #4 
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.
>>>
>
Andrew Rybchenko Oct. 18, 2022, 12:44 p.m. UTC | #5 
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.
>>>>
>>
diff mbox series

Patch

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);
+
 	/*
 	 * If RSS is enabled, fill table with default values and
 	 * set key to the value specified in port RSS configuration.