* [PATCH] net/bonding: make bonded device configure method re-entrant
@ 2022-09-11 12:24 Ivan Malov
2022-10-17 8:42 ` Andrew Rybchenko
2022-10-18 19:45 ` [PATCH v2] " Ivan Malov
0 siblings, 2 replies; 11+ messages in thread
From: Ivan Malov @ 2022-09-11 12:24 UTC (permalink / raw)
To: dev; +Cc: Andrew Rybchenko, Chas Williams, Min Hu (Connor), Anatoly Burakov
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.
--
2.30.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net/bonding: make bonded device configure method re-entrant
2022-09-11 12:24 [PATCH] net/bonding: make bonded device configure method re-entrant Ivan Malov
@ 2022-10-17 8:42 ` Andrew Rybchenko
2022-10-17 12:32 ` Chas Williams
2022-10-18 19:45 ` [PATCH v2] " Ivan Malov
1 sibling, 1 reply; 11+ messages in thread
From: Andrew Rybchenko @ 2022-10-17 8:42 UTC (permalink / raw)
To: Chas Williams, Min Hu (Connor); +Cc: Anatoly Burakov, Ivan Malov, dev
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net/bonding: make bonded device configure method re-entrant
2022-10-17 8:42 ` Andrew Rybchenko
@ 2022-10-17 12:32 ` Chas Williams
2022-10-17 14:10 ` Andrew Rybchenko
0 siblings, 1 reply; 11+ messages in thread
From: Chas Williams @ 2022-10-17 12:32 UTC (permalink / raw)
To: Andrew Rybchenko, Chas Williams, Min Hu (Connor)
Cc: Anatoly Burakov, Ivan Malov, dev
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.
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net/bonding: make bonded device configure method re-entrant
2022-10-17 12:32 ` Chas Williams
@ 2022-10-17 14:10 ` Andrew Rybchenko
2022-10-18 10:34 ` Chas Williams
0 siblings, 1 reply; 11+ messages in thread
From: Andrew Rybchenko @ 2022-10-17 14:10 UTC (permalink / raw)
To: Chas Williams, Chas Williams, Min Hu (Connor)
Cc: Anatoly Burakov, Ivan Malov, dev
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.
>>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net/bonding: make bonded device configure method re-entrant
2022-10-17 14:10 ` Andrew Rybchenko
@ 2022-10-18 10:34 ` Chas Williams
2022-10-18 12:44 ` Andrew Rybchenko
0 siblings, 1 reply; 11+ messages in thread
From: Chas Williams @ 2022-10-18 10:34 UTC (permalink / raw)
To: Andrew Rybchenko, Chas Williams, Min Hu (Connor)
Cc: Anatoly Burakov, Ivan Malov, dev
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.
>>>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] net/bonding: make bonded device configure method re-entrant
2022-10-18 10:34 ` Chas Williams
@ 2022-10-18 12:44 ` Andrew Rybchenko
0 siblings, 0 replies; 11+ messages in thread
From: Andrew Rybchenko @ 2022-10-18 12:44 UTC (permalink / raw)
To: Ivan Malov
Cc: Anatoly Burakov, dev, Chas Williams, Chas Williams, Min Hu (Connor)
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.
>>>>
>>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] net/bonding: make bonded device configure method re-entrant
2022-09-11 12:24 [PATCH] net/bonding: make bonded device configure method re-entrant Ivan Malov
2022-10-17 8:42 ` Andrew Rybchenko
@ 2022-10-18 19:45 ` Ivan Malov
2022-10-19 10:26 ` Andrew Rybchenko
2022-11-01 10:56 ` Jiang, YuX
1 sibling, 2 replies; 11+ messages in thread
From: Ivan Malov @ 2022-10-18 19:45 UTC (permalink / raw)
To: dev
Cc: Chas Williams, Min Hu (Connor),
Andrew Rybchenko, Chas Williams, Anatoly Burakov
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>
Acked-by: Chas Williams <3chas3@gmail.com>
---
drivers/net/bonding/rte_eth_bond_pmd.c | 25 +++++++++++++++++++------
1 file changed, 19 insertions(+), 6 deletions(-)
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index 27005c747c..486b7fc9f7 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -2148,18 +2148,14 @@ 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);
while (internals->slave_count != skipped) {
uint16_t port_id = internals->slaves[skipped].port_id;
@@ -2178,6 +2174,20 @@ bond_ethdev_close(struct rte_eth_dev *dev)
}
}
bond_flow_ops.flush(dev, &ferror);
+}
+
+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);
@@ -3610,6 +3620,9 @@ 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.
--
2.30.2
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] net/bonding: make bonded device configure method re-entrant
2022-10-18 19:45 ` [PATCH v2] " Ivan Malov
@ 2022-10-19 10:26 ` Andrew Rybchenko
2022-11-01 10:56 ` Jiang, YuX
1 sibling, 0 replies; 11+ messages in thread
From: Andrew Rybchenko @ 2022-10-19 10:26 UTC (permalink / raw)
To: Ivan Malov, dev
Cc: Chas Williams, Min Hu (Connor), Chas Williams, Anatoly Burakov
On 10/18/22 22:45, 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>
> Acked-by: Chas Williams <3chas3@gmail.com>
Applied to dpdk-next-net/main, thanks.
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH v2] net/bonding: make bonded device configure method re-entrant
2022-10-18 19:45 ` [PATCH v2] " Ivan Malov
2022-10-19 10:26 ` Andrew Rybchenko
@ 2022-11-01 10:56 ` Jiang, YuX
2022-11-01 16:33 ` Ivan Malov
1 sibling, 1 reply; 11+ messages in thread
From: Jiang, YuX @ 2022-11-01 10:56 UTC (permalink / raw)
To: Ivan Malov, dev
Cc: Chas Williams, Min Hu (Connor),
Andrew Rybchenko, Chas Williams, Burakov, Anatoly
Hi Ivan,
This patch introduces a bug https://bugs.dpdk.org/show_bug.cgi?id=1119, can you pls have a look quickly?
Best regards,
Yu Jiang
> -----Original Message-----
> From: Ivan Malov <ivan.malov@oktetlabs.ru>
> Sent: Wednesday, October 19, 2022 3:46 AM
> To: dev@dpdk.org
> Cc: Chas Williams <3chas3@gmail.com>; Min Hu (Connor)
> <humin29@huawei.com>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Chas Williams <chas3@att.com>; Burakov,
> Anatoly <anatoly.burakov@intel.com>
> Subject: [PATCH v2] net/bonding: make bonded device configure method re-
> entrant
>
> 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>
> Acked-by: Chas Williams <3chas3@gmail.com>
> ---
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH v2] net/bonding: make bonded device configure method re-entrant
2022-11-01 10:56 ` Jiang, YuX
@ 2022-11-01 16:33 ` Ivan Malov
2022-11-02 5:20 ` Jiang, YuX
0 siblings, 1 reply; 11+ messages in thread
From: Ivan Malov @ 2022-11-01 16:33 UTC (permalink / raw)
To: Jiang, YuX
Cc: dev, Chas Williams, Min Hu (Connor),
Andrew Rybchenko, Chas Williams, Burakov, Anatoly
Hi,
Thanks for noticing the bug. A fix has been proposed already:
https://patches.dpdk.org/project/dpdk/patch/20221101161853.2702425-1-ivan.malov@oktetlabs.ru/
With the fix applied, does the problem still manifest itself?
Please let me know.
Thank you.
On Tue, 1 Nov 2022, Jiang, YuX wrote:
> Hi Ivan,
>
> This patch introduces a bug https://bugs.dpdk.org/show_bug.cgi?id=1119, can you pls have a look quickly?
>
> Best regards,
> Yu Jiang
>
>> -----Original Message-----
>> From: Ivan Malov <ivan.malov@oktetlabs.ru>
>> Sent: Wednesday, October 19, 2022 3:46 AM
>> To: dev@dpdk.org
>> Cc: Chas Williams <3chas3@gmail.com>; Min Hu (Connor)
>> <humin29@huawei.com>; Andrew Rybchenko
>> <andrew.rybchenko@oktetlabs.ru>; Chas Williams <chas3@att.com>; Burakov,
>> Anatoly <anatoly.burakov@intel.com>
>> Subject: [PATCH v2] net/bonding: make bonded device configure method re-
>> entrant
>>
>> 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>
>> Acked-by: Chas Williams <3chas3@gmail.com>
>> ---
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH v2] net/bonding: make bonded device configure method re-entrant
2022-11-01 16:33 ` Ivan Malov
@ 2022-11-02 5:20 ` Jiang, YuX
0 siblings, 0 replies; 11+ messages in thread
From: Jiang, YuX @ 2022-11-02 5:20 UTC (permalink / raw)
To: Ivan Malov
Cc: dev, Chas Williams, Min Hu (Connor),
Andrew Rybchenko, Chas Williams, Burakov, Anatoly
Hi Ivan,
https://bugs.dpdk.org/show_bug.cgi?id=1119
https://bugs.dpdk.org/show_bug.cgi?id=1118
Can be fixed by patches:
https://patches.dpdk.org/project/dpdk/patch/20221101161853.2702425-1-ivan.malov@oktetlabs.ru/
https://patches.dpdk.org/project/dpdk/patch/20221031131744.2340150-1-ivan.malov@oktetlabs.ru/
Best regards,
Yu Jiang
> -----Original Message-----
> From: Ivan Malov <ivan.malov@oktetlabs.ru>
> Sent: Wednesday, November 2, 2022 12:34 AM
> To: Jiang, YuX <yux.jiang@intel.com>
> Cc: dev@dpdk.org; Chas Williams <3chas3@gmail.com>; Min Hu (Connor)
> <humin29@huawei.com>; Andrew Rybchenko
> <andrew.rybchenko@oktetlabs.ru>; Chas Williams <chas3@att.com>; Burakov,
> Anatoly <anatoly.burakov@intel.com>
> Subject: RE: [PATCH v2] net/bonding: make bonded device configure method
> re-entrant
>
> Hi,
>
> Thanks for noticing the bug. A fix has been proposed already:
> https://patches.dpdk.org/project/dpdk/patch/20221101161853.2702425-1-
> ivan.malov@oktetlabs.ru/
>
> With the fix applied, does the problem still manifest itself?
> Please let me know.
>
> Thank you.
>
> On Tue, 1 Nov 2022, Jiang, YuX wrote:
>
> > Hi Ivan,
> >
> > This patch introduces a bug https://bugs.dpdk.org/show_bug.cgi?id=1119, can
> you pls have a look quickly?
> >
> > Best regards,
> > Yu Jiang
> >
> >> -----Original Message-----
> >> From: Ivan Malov <ivan.malov@oktetlabs.ru>
> >> Sent: Wednesday, October 19, 2022 3:46 AM
> >> To: dev@dpdk.org
> >> Cc: Chas Williams <3chas3@gmail.com>; Min Hu (Connor)
> >> <humin29@huawei.com>; Andrew Rybchenko
> >> <andrew.rybchenko@oktetlabs.ru>; Chas Williams <chas3@att.com>;
> >> Burakov, Anatoly <anatoly.burakov@intel.com>
> >> Subject: [PATCH v2] net/bonding: make bonded device configure method
> >> re- entrant
> >>
> >> 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>
> >> Acked-by: Chas Williams <3chas3@gmail.com>
> >> ---
> >
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-11-02 5:20 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-11 12:24 [PATCH] net/bonding: make bonded device configure method re-entrant Ivan Malov
2022-10-17 8:42 ` Andrew Rybchenko
2022-10-17 12:32 ` Chas Williams
2022-10-17 14:10 ` Andrew Rybchenko
2022-10-18 10:34 ` Chas Williams
2022-10-18 12:44 ` Andrew Rybchenko
2022-10-18 19:45 ` [PATCH v2] " Ivan Malov
2022-10-19 10:26 ` Andrew Rybchenko
2022-11-01 10:56 ` Jiang, YuX
2022-11-01 16:33 ` Ivan Malov
2022-11-02 5:20 ` Jiang, YuX
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).