* [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
@ 2016-09-07 12:28 Ilya Maximets
[not found] ` <CGME20160916050359eucas1p22998d07e190781e165082cdd9c917470@eucas1p2.samsung.com>
` (3 more replies)
0 siblings, 4 replies; 32+ messages in thread
From: Ilya Maximets @ 2016-09-07 12:28 UTC (permalink / raw)
To: dev, Declan Doherty
Cc: Heetae Ahn, Yuanhan Liu, Eric Kinzie, Bernard Iremonger,
Ilya Maximets, stable
This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
It is necessary to reconfigure all queues every time because configuration
can be changed.
For example, if we're reconfiguring bonding device with new memory pool,
already configured queues will still use the old one. And if the old
mempool be freed, application likely will panic in attempt to use
freed mempool.
This happens when we use the bonding device with OVS 2.6 while MTU
reconfiguration:
PANIC in rte_mempool_get_ops():
assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
Cc: <stable@dpdk.org>
Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index b20a272..eb5b6d1 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
struct bond_rx_queue *bd_rx_q;
struct bond_tx_queue *bd_tx_q;
- uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
- uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
int errval;
uint16_t q_id;
@@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
}
/* Setup Rx Queues */
- /* Use existing queues, if any */
- for (q_id = old_nb_rx_queues;
- q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
+ for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
bd_rx_q = (struct bond_rx_queue *)bonded_eth_dev->data->rx_queues[q_id];
errval = rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
@@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
}
/* Setup Tx Queues */
- /* Use existing queues, if any */
- for (q_id = old_nb_tx_queues;
- q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
+ for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
bd_tx_q = (struct bond_tx_queue *)bonded_eth_dev->data->tx_queues[q_id];
errval = rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
[not found] ` <CGME20160916050359eucas1p22998d07e190781e165082cdd9c917470@eucas1p2.samsung.com>
@ 2016-09-16 5:03 ` Ilya Maximets
0 siblings, 0 replies; 32+ messages in thread
From: Ilya Maximets @ 2016-09-16 5:03 UTC (permalink / raw)
To: dev, Declan Doherty
Cc: Heetae Ahn, Yuanhan Liu, Eric Kinzie, Bernard Iremonger, stable,
Dyasly Sergey
Ping.
Best regards, Ilya Maximets.
On 07.09.2016 15:28, Ilya Maximets wrote:
> This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
>
> It is necessary to reconfigure all queues every time because configuration
> can be changed.
>
> For example, if we're reconfiguring bonding device with new memory pool,
> already configured queues will still use the old one. And if the old
> mempool be freed, application likely will panic in attempt to use
> freed mempool.
>
> This happens when we use the bonding device with OVS 2.6 while MTU
> reconfiguration:
>
> PANIC in rte_mempool_get_ops():
> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
>
> Cc: <stable@dpdk.org>
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
> drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index b20a272..eb5b6d1 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> struct bond_rx_queue *bd_rx_q;
> struct bond_tx_queue *bd_tx_q;
>
> - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
> - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
> int errval;
> uint16_t q_id;
>
> @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> }
>
> /* Setup Rx Queues */
> - /* Use existing queues, if any */
> - for (q_id = old_nb_rx_queues;
> - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> bd_rx_q = (struct bond_rx_queue *)bonded_eth_dev->data->rx_queues[q_id];
>
> errval = rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
> @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> }
>
> /* Setup Tx Queues */
> - /* Use existing queues, if any */
> - for (q_id = old_nb_tx_queues;
> - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
> bd_tx_q = (struct bond_tx_queue *)bonded_eth_dev->data->tx_queues[q_id];
>
> errval = rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-09-07 12:28 [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues" Ilya Maximets
[not found] ` <CGME20160916050359eucas1p22998d07e190781e165082cdd9c917470@eucas1p2.samsung.com>
@ 2016-10-06 14:32 ` Declan Doherty
2016-10-19 9:55 ` Bruce Richardson
2016-10-07 2:02 ` Eric Kinzie
2016-10-18 12:28 ` [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues" Jan Blunck
3 siblings, 1 reply; 32+ messages in thread
From: Declan Doherty @ 2016-10-06 14:32 UTC (permalink / raw)
To: Ilya Maximets, dev
Cc: Heetae Ahn, Yuanhan Liu, Eric Kinzie, Bernard Iremonger, stable
On 07/09/16 13:28, Ilya Maximets wrote:
> This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
>
> It is necessary to reconfigure all queues every time because configuration
> can be changed.
>
Hey Ilya, this makes sense. I guess this was my original intention but I
missed this case in my review of the change.
> For example, if we're reconfiguring bonding device with new memory pool,
> already configured queues will still use the old one. And if the old
> mempool be freed, application likely will panic in attempt to use
> freed mempool.
>
> This happens when we use the bonding device with OVS 2.6 while MTU
> reconfiguration:
>
> PANIC in rte_mempool_get_ops():
> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
>
> Cc: <stable@dpdk.org>
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
> drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index b20a272..eb5b6d1 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> struct bond_rx_queue *bd_rx_q;
> struct bond_tx_queue *bd_tx_q;
>
> - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
> - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
> int errval;
> uint16_t q_id;
>
> @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> }
>
> /* Setup Rx Queues */
> - /* Use existing queues, if any */
> - for (q_id = old_nb_rx_queues;
> - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> bd_rx_q = (struct bond_rx_queue *)bonded_eth_dev->data->rx_queues[q_id];
>
> errval = rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
> @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> }
>
> /* Setup Tx Queues */
> - /* Use existing queues, if any */
> - for (q_id = old_nb_tx_queues;
> - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
> bd_tx_q = (struct bond_tx_queue *)bonded_eth_dev->data->tx_queues[q_id];
>
> errval = rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
>
Acked-by: Declan Doherty <declan.doherty@intel.com>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-09-07 12:28 [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues" Ilya Maximets
[not found] ` <CGME20160916050359eucas1p22998d07e190781e165082cdd9c917470@eucas1p2.samsung.com>
2016-10-06 14:32 ` Declan Doherty
@ 2016-10-07 2:02 ` Eric Kinzie
2016-10-12 13:24 ` Ilya Maximets
2016-10-18 12:28 ` [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues" Jan Blunck
3 siblings, 1 reply; 32+ messages in thread
From: Eric Kinzie @ 2016-10-07 2:02 UTC (permalink / raw)
To: Ilya Maximets
Cc: dev, Declan Doherty, Heetae Ahn, Yuanhan Liu, Bernard Iremonger, stable
On Wed Sep 07 15:28:10 +0300 2016, Ilya Maximets wrote:
> This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
>
> It is necessary to reconfigure all queues every time because configuration
> can be changed.
>
> For example, if we're reconfiguring bonding device with new memory pool,
> already configured queues will still use the old one. And if the old
> mempool be freed, application likely will panic in attempt to use
> freed mempool.
>
> This happens when we use the bonding device with OVS 2.6 while MTU
> reconfiguration:
>
> PANIC in rte_mempool_get_ops():
> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
>
> Cc: <stable@dpdk.org>
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
> drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index b20a272..eb5b6d1 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> struct bond_rx_queue *bd_rx_q;
> struct bond_tx_queue *bd_tx_q;
>
> - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
> - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
> int errval;
> uint16_t q_id;
>
> @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> }
>
> /* Setup Rx Queues */
> - /* Use existing queues, if any */
> - for (q_id = old_nb_rx_queues;
> - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> bd_rx_q = (struct bond_rx_queue *)bonded_eth_dev->data->rx_queues[q_id];
>
> errval = rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
> @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> }
>
> /* Setup Tx Queues */
> - /* Use existing queues, if any */
> - for (q_id = old_nb_tx_queues;
> - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
> bd_tx_q = (struct bond_tx_queue *)bonded_eth_dev->data->tx_queues[q_id];
>
> errval = rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
> --
> 2.7.4
>
NAK
There are still some users of this code. Let's give them a chance to
comment before removing it.
Thanks,
Eric
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-10-07 2:02 ` Eric Kinzie
@ 2016-10-12 13:24 ` Ilya Maximets
2016-10-12 15:24 ` Bruce Richardson
0 siblings, 1 reply; 32+ messages in thread
From: Ilya Maximets @ 2016-10-12 13:24 UTC (permalink / raw)
To: Eric Kinzie
Cc: dev, Declan Doherty, Heetae Ahn, Yuanhan Liu, Bernard Iremonger,
stable, Thomas Monjalon
On 07.10.2016 05:02, Eric Kinzie wrote:
> On Wed Sep 07 15:28:10 +0300 2016, Ilya Maximets wrote:
>> This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
>>
>> It is necessary to reconfigure all queues every time because configuration
>> can be changed.
>>
>> For example, if we're reconfiguring bonding device with new memory pool,
>> already configured queues will still use the old one. And if the old
>> mempool be freed, application likely will panic in attempt to use
>> freed mempool.
>>
>> This happens when we use the bonding device with OVS 2.6 while MTU
>> reconfiguration:
>>
>> PANIC in rte_mempool_get_ops():
>> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
>>
>> Cc: <stable@dpdk.org>
>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>> ---
>> drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
>> 1 file changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
>> index b20a272..eb5b6d1 100644
>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>> @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>> struct bond_rx_queue *bd_rx_q;
>> struct bond_tx_queue *bd_tx_q;
>>
>> - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
>> - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
>> int errval;
>> uint16_t q_id;
>>
>> @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>> }
>>
>> /* Setup Rx Queues */
>> - /* Use existing queues, if any */
>> - for (q_id = old_nb_rx_queues;
>> - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
>> bd_rx_q = (struct bond_rx_queue *)bonded_eth_dev->data->rx_queues[q_id];
>>
>> errval = rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
>> @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>> }
>>
>> /* Setup Tx Queues */
>> - /* Use existing queues, if any */
>> - for (q_id = old_nb_tx_queues;
>> - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
>> bd_tx_q = (struct bond_tx_queue *)bonded_eth_dev->data->tx_queues[q_id];
>>
>> errval = rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
>> --
>> 2.7.4
>>
>
> NAK
>
> There are still some users of this code. Let's give them a chance to
> comment before removing it.
Hi Eric,
Are these users in CC-list? If not, could you, please, add them?
This patch awaits in mail-list already more than a month. I think, it's enough
time period for all who wants to say something. Patch fixes a real bug that
prevent using of DPDK bonding in all applications that reconfigures devices
in runtime including OVS.
Best regards, Ilya Maximets.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-10-12 13:24 ` Ilya Maximets
@ 2016-10-12 15:24 ` Bruce Richardson
2016-10-13 23:37 ` Eric Kinzie
0 siblings, 1 reply; 32+ messages in thread
From: Bruce Richardson @ 2016-10-12 15:24 UTC (permalink / raw)
To: Ilya Maximets
Cc: Eric Kinzie, dev, Declan Doherty, Heetae Ahn, Yuanhan Liu,
Bernard Iremonger, stable, Thomas Monjalon
On Wed, Oct 12, 2016 at 04:24:54PM +0300, Ilya Maximets wrote:
> On 07.10.2016 05:02, Eric Kinzie wrote:
> > On Wed Sep 07 15:28:10 +0300 2016, Ilya Maximets wrote:
> >> This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
> >>
> >> It is necessary to reconfigure all queues every time because configuration
> >> can be changed.
> >>
> >> For example, if we're reconfiguring bonding device with new memory pool,
> >> already configured queues will still use the old one. And if the old
> >> mempool be freed, application likely will panic in attempt to use
> >> freed mempool.
> >>
> >> This happens when we use the bonding device with OVS 2.6 while MTU
> >> reconfiguration:
> >>
> >> PANIC in rte_mempool_get_ops():
> >> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
> >>
> >> Cc: <stable@dpdk.org>
> >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> >> ---
> >> drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
> >> 1 file changed, 2 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> >> index b20a272..eb5b6d1 100644
> >> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> >> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> >> @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> >> struct bond_rx_queue *bd_rx_q;
> >> struct bond_tx_queue *bd_tx_q;
> >>
> >> - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
> >> - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
> >> int errval;
> >> uint16_t q_id;
> >>
> >> @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> >> }
> >>
> >> /* Setup Rx Queues */
> >> - /* Use existing queues, if any */
> >> - for (q_id = old_nb_rx_queues;
> >> - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> >> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> >> bd_rx_q = (struct bond_rx_queue *)bonded_eth_dev->data->rx_queues[q_id];
> >>
> >> errval = rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
> >> @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> >> }
> >>
> >> /* Setup Tx Queues */
> >> - /* Use existing queues, if any */
> >> - for (q_id = old_nb_tx_queues;
> >> - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
> >> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
> >> bd_tx_q = (struct bond_tx_queue *)bonded_eth_dev->data->tx_queues[q_id];
> >>
> >> errval = rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
> >> --
> >> 2.7.4
> >>
> >
> > NAK
> >
> > There are still some users of this code. Let's give them a chance to
> > comment before removing it.
>
> Hi Eric,
>
> Are these users in CC-list? If not, could you, please, add them?
> This patch awaits in mail-list already more than a month. I think, it's enough
> time period for all who wants to say something. Patch fixes a real bug that
> prevent using of DPDK bonding in all applications that reconfigures devices
> in runtime including OVS.
>
Agreed.
Eric, does reverting this patch cause you problems directly, or is your concern
just with regards to potential impact to others?
Thanks,
/Bruce
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-10-12 15:24 ` Bruce Richardson
@ 2016-10-13 23:37 ` Eric Kinzie
2016-10-24 11:02 ` Declan Doherty
0 siblings, 1 reply; 32+ messages in thread
From: Eric Kinzie @ 2016-10-13 23:37 UTC (permalink / raw)
To: Bruce Richardson
Cc: Ilya Maximets, dev, Declan Doherty, Heetae Ahn, Yuanhan Liu,
Bernard Iremonger, stable, Thomas Monjalon
On Wed Oct 12 16:24:21 +0100 2016, Bruce Richardson wrote:
> On Wed, Oct 12, 2016 at 04:24:54PM +0300, Ilya Maximets wrote:
> > On 07.10.2016 05:02, Eric Kinzie wrote:
> > > On Wed Sep 07 15:28:10 +0300 2016, Ilya Maximets wrote:
> > >> This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
> > >>
> > >> It is necessary to reconfigure all queues every time because configuration
> > >> can be changed.
> > >>
> > >> For example, if we're reconfiguring bonding device with new memory pool,
> > >> already configured queues will still use the old one. And if the old
> > >> mempool be freed, application likely will panic in attempt to use
> > >> freed mempool.
> > >>
> > >> This happens when we use the bonding device with OVS 2.6 while MTU
> > >> reconfiguration:
> > >>
> > >> PANIC in rte_mempool_get_ops():
> > >> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
> > >>
> > >> Cc: <stable@dpdk.org>
> > >> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> > >> ---
> > >> drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
> > >> 1 file changed, 2 insertions(+), 8 deletions(-)
> > >>
> > >> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> > >> index b20a272..eb5b6d1 100644
> > >> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> > >> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> > >> @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> > >> struct bond_rx_queue *bd_rx_q;
> > >> struct bond_tx_queue *bd_tx_q;
> > >>
> > >> - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
> > >> - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
> > >> int errval;
> > >> uint16_t q_id;
> > >>
> > >> @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> > >> }
> > >>
> > >> /* Setup Rx Queues */
> > >> - /* Use existing queues, if any */
> > >> - for (q_id = old_nb_rx_queues;
> > >> - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> > >> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> > >> bd_rx_q = (struct bond_rx_queue *)bonded_eth_dev->data->rx_queues[q_id];
> > >>
> > >> errval = rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
> > >> @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> > >> }
> > >>
> > >> /* Setup Tx Queues */
> > >> - /* Use existing queues, if any */
> > >> - for (q_id = old_nb_tx_queues;
> > >> - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
> > >> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
> > >> bd_tx_q = (struct bond_tx_queue *)bonded_eth_dev->data->tx_queues[q_id];
> > >>
> > >> errval = rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
> > >> --
> > >> 2.7.4
> > >>
> > >
> > > NAK
> > >
> > > There are still some users of this code. Let's give them a chance to
> > > comment before removing it.
> >
> > Hi Eric,
> >
> > Are these users in CC-list? If not, could you, please, add them?
> > This patch awaits in mail-list already more than a month. I think, it's enough
> > time period for all who wants to say something. Patch fixes a real bug that
> > prevent using of DPDK bonding in all applications that reconfigures devices
> > in runtime including OVS.
> >
> Agreed.
>
> Eric, does reverting this patch cause you problems directly, or is your concern
> just with regards to potential impact to others?
>
> Thanks,
> /Bruce
This won't impact me directly. The users are CCed (different thread)
and I haven't seen any comment, so I no longer have any objection to
reverting this change.
Eric
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-09-07 12:28 [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues" Ilya Maximets
` (2 preceding siblings ...)
2016-10-07 2:02 ` Eric Kinzie
@ 2016-10-18 12:28 ` Jan Blunck
2016-10-18 12:49 ` Ilya Maximets
3 siblings, 1 reply; 32+ messages in thread
From: Jan Blunck @ 2016-10-18 12:28 UTC (permalink / raw)
To: Ilya Maximets
Cc: dev, Declan Doherty, Heetae Ahn, Yuanhan Liu, Eric Kinzie,
Bernard Iremonger, stable
If the application already configured queues the PMD should not
silently claim ownership and reset them.
What exactly is the problem when changing MTU? This works fine from
what I can tell.
Cheers,
Jan
On Wed, Sep 7, 2016 at 2:28 PM, Ilya Maximets <i.maximets@samsung.com> wrote:
> This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
>
> It is necessary to reconfigure all queues every time because configuration
> can be changed.
>
> For example, if we're reconfiguring bonding device with new memory pool,
> already configured queues will still use the old one. And if the old
> mempool be freed, application likely will panic in attempt to use
> freed mempool.
>
> This happens when we use the bonding device with OVS 2.6 while MTU
> reconfiguration:
>
> PANIC in rte_mempool_get_ops():
> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
>
> Cc: <stable@dpdk.org>
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
> drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
> index b20a272..eb5b6d1 100644
> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> struct bond_rx_queue *bd_rx_q;
> struct bond_tx_queue *bd_tx_q;
>
> - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
> - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
> int errval;
> uint16_t q_id;
>
> @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> }
>
> /* Setup Rx Queues */
> - /* Use existing queues, if any */
> - for (q_id = old_nb_rx_queues;
> - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> bd_rx_q = (struct bond_rx_queue *)bonded_eth_dev->data->rx_queues[q_id];
>
> errval = rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
> @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
> }
>
> /* Setup Tx Queues */
> - /* Use existing queues, if any */
> - for (q_id = old_nb_tx_queues;
> - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
> bd_tx_q = (struct bond_tx_queue *)bonded_eth_dev->data->tx_queues[q_id];
>
> errval = rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-10-18 12:28 ` [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues" Jan Blunck
@ 2016-10-18 12:49 ` Ilya Maximets
2016-10-18 15:19 ` Jan Blunck
0 siblings, 1 reply; 32+ messages in thread
From: Ilya Maximets @ 2016-10-18 12:49 UTC (permalink / raw)
To: Jan Blunck
Cc: dev, Declan Doherty, Heetae Ahn, Yuanhan Liu, Eric Kinzie,
Bernard Iremonger, stable
On 18.10.2016 15:28, Jan Blunck wrote:
> If the application already configured queues the PMD should not
> silently claim ownership and reset them.
>
> What exactly is the problem when changing MTU? This works fine from
> what I can tell.
Following scenario leads to APP PANIC:
1. mempool_1 = rte_mempool_create()
2. rte_eth_rx_queue_setup(bond0, ..., mempool_1);
3. rte_eth_dev_start(bond0);
4. mempool_2 = rte_mempool_create();
5. rte_eth_dev_stop(bond0);
6. rte_eth_rx_queue_setup(bond0, ..., mempool_2);
7. rte_eth_dev_start(bond0);
* RX queues still use 'mempool_1' because reconfiguration doesn't affect them. *
8. rte_mempool_free(mempool_1);
9. On any rx operation we'll get PANIC because of using freed 'mempool_1':
PANIC in rte_mempool_get_ops():
assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
You may just start OVS 2.6 with DPDK bonding device and attempt to change MTU via 'mtu_request'.
Bug is easily reproducible.
Best regards, Ilya Maximets.
>
> On Wed, Sep 7, 2016 at 2:28 PM, Ilya Maximets <i.maximets@samsung.com> wrote:
>> This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
>>
>> It is necessary to reconfigure all queues every time because configuration
>> can be changed.
>>
>> For example, if we're reconfiguring bonding device with new memory pool,
>> already configured queues will still use the old one. And if the old
>> mempool be freed, application likely will panic in attempt to use
>> freed mempool.
>>
>> This happens when we use the bonding device with OVS 2.6 while MTU
>> reconfiguration:
>>
>> PANIC in rte_mempool_get_ops():
>> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
>>
>> Cc: <stable@dpdk.org>
>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>> ---
>> drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
>> 1 file changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
>> index b20a272..eb5b6d1 100644
>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>> @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>> struct bond_rx_queue *bd_rx_q;
>> struct bond_tx_queue *bd_tx_q;
>>
>> - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
>> - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
>> int errval;
>> uint16_t q_id;
>>
>> @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>> }
>>
>> /* Setup Rx Queues */
>> - /* Use existing queues, if any */
>> - for (q_id = old_nb_rx_queues;
>> - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
>> bd_rx_q = (struct bond_rx_queue *)bonded_eth_dev->data->rx_queues[q_id];
>>
>> errval = rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
>> @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>> }
>>
>> /* Setup Tx Queues */
>> - /* Use existing queues, if any */
>> - for (q_id = old_nb_tx_queues;
>> - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
>> bd_tx_q = (struct bond_tx_queue *)bonded_eth_dev->data->tx_queues[q_id];
>>
>> errval = rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
>> --
>> 2.7.4
>>
>
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-10-18 12:49 ` Ilya Maximets
@ 2016-10-18 15:19 ` Jan Blunck
2016-10-19 9:47 ` Ilya Maximets
0 siblings, 1 reply; 32+ messages in thread
From: Jan Blunck @ 2016-10-18 15:19 UTC (permalink / raw)
To: Ilya Maximets
Cc: dev, Declan Doherty, Heetae Ahn, Yuanhan Liu, Eric Kinzie,
Bernard Iremonger, stable
On Tue, Oct 18, 2016 at 2:49 PM, Ilya Maximets <i.maximets@samsung.com> wrote:
> On 18.10.2016 15:28, Jan Blunck wrote:
>> If the application already configured queues the PMD should not
>> silently claim ownership and reset them.
>>
>> What exactly is the problem when changing MTU? This works fine from
>> what I can tell.
>
> Following scenario leads to APP PANIC:
>
> 1. mempool_1 = rte_mempool_create()
> 2. rte_eth_rx_queue_setup(bond0, ..., mempool_1);
> 3. rte_eth_dev_start(bond0);
> 4. mempool_2 = rte_mempool_create();
> 5. rte_eth_dev_stop(bond0);
> 6. rte_eth_rx_queue_setup(bond0, ..., mempool_2);
> 7. rte_eth_dev_start(bond0);
> * RX queues still use 'mempool_1' because reconfiguration doesn't affect them. *
> 8. rte_mempool_free(mempool_1);
> 9. On any rx operation we'll get PANIC because of using freed 'mempool_1':
> PANIC in rte_mempool_get_ops():
> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
>
> You may just start OVS 2.6 with DPDK bonding device and attempt to change MTU via 'mtu_request'.
> Bug is easily reproducible.
>
I see. I'm not 100% that this is expected to work without leaking the
driver's queues though. The driver is allowed to do allocations in
its rx_queue_setup() function that are being freed via
rx_queue_release() later. But rx_queue_release() is only called if you
reconfigure the
device with 0 queues. From what I understand there is no other way to
reconfigure a device to use another mempool.
But ... even that wouldn't work with the bonding driver right now: the
bonding master only configures the slaves during startup. I can put
that on my todo list.
Coming back to your original problem: changing the MTU for the bond
does work through rte_eth_dev_set_mtu() for slaves supporting that. In
any other case you could (re-)configure rxmode.max_rx_pkt_len (and
jumbo_frame / enable_scatter accordingly). This does work without a
call to rte_eth_rx_queue_setup().
Hope this helps,
Jan
> Best regards, Ilya Maximets.
>
>>
>> On Wed, Sep 7, 2016 at 2:28 PM, Ilya Maximets <i.maximets@samsung.com> wrote:
>>> This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
>>>
>>> It is necessary to reconfigure all queues every time because configuration
>>> can be changed.
>>>
>>> For example, if we're reconfiguring bonding device with new memory pool,
>>> already configured queues will still use the old one. And if the old
>>> mempool be freed, application likely will panic in attempt to use
>>> freed mempool.
>>>
>>> This happens when we use the bonding device with OVS 2.6 while MTU
>>> reconfiguration:
>>>
>>> PANIC in rte_mempool_get_ops():
>>> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
>>>
>>> Cc: <stable@dpdk.org>
>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>> ---
>>> drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
>>> 1 file changed, 2 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
>>> index b20a272..eb5b6d1 100644
>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>> @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>>> struct bond_rx_queue *bd_rx_q;
>>> struct bond_tx_queue *bd_tx_q;
>>>
>>> - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
>>> - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
>>> int errval;
>>> uint16_t q_id;
>>>
>>> @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>>> }
>>>
>>> /* Setup Rx Queues */
>>> - /* Use existing queues, if any */
>>> - for (q_id = old_nb_rx_queues;
>>> - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
>>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
>>> bd_rx_q = (struct bond_rx_queue *)bonded_eth_dev->data->rx_queues[q_id];
>>>
>>> errval = rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>> @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>>> }
>>>
>>> /* Setup Tx Queues */
>>> - /* Use existing queues, if any */
>>> - for (q_id = old_nb_tx_queues;
>>> - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
>>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
>>> bd_tx_q = (struct bond_tx_queue *)bonded_eth_dev->data->tx_queues[q_id];
>>>
>>> errval = rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>> --
>>> 2.7.4
>>>
>>
>>
>>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-10-18 15:19 ` Jan Blunck
@ 2016-10-19 9:47 ` Ilya Maximets
2016-10-24 14:54 ` Jan Blunck
0 siblings, 1 reply; 32+ messages in thread
From: Ilya Maximets @ 2016-10-19 9:47 UTC (permalink / raw)
To: Jan Blunck
Cc: dev, Declan Doherty, Heetae Ahn, Yuanhan Liu, Eric Kinzie,
Bernard Iremonger, stable, Dyasly Sergey
On 18.10.2016 18:19, Jan Blunck wrote:
> On Tue, Oct 18, 2016 at 2:49 PM, Ilya Maximets <i.maximets@samsung.com> wrote:
>> On 18.10.2016 15:28, Jan Blunck wrote:
>>> If the application already configured queues the PMD should not
>>> silently claim ownership and reset them.
>>>
>>> What exactly is the problem when changing MTU? This works fine from
>>> what I can tell.
>>
>> Following scenario leads to APP PANIC:
>>
>> 1. mempool_1 = rte_mempool_create()
>> 2. rte_eth_rx_queue_setup(bond0, ..., mempool_1);
>> 3. rte_eth_dev_start(bond0);
>> 4. mempool_2 = rte_mempool_create();
>> 5. rte_eth_dev_stop(bond0);
>> 6. rte_eth_rx_queue_setup(bond0, ..., mempool_2);
>> 7. rte_eth_dev_start(bond0);
>> * RX queues still use 'mempool_1' because reconfiguration doesn't affect them. *
>> 8. rte_mempool_free(mempool_1);
>> 9. On any rx operation we'll get PANIC because of using freed 'mempool_1':
>> PANIC in rte_mempool_get_ops():
>> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
>>
>> You may just start OVS 2.6 with DPDK bonding device and attempt to change MTU via 'mtu_request'.
>> Bug is easily reproducible.
>>
>
> I see. I'm not 100% that this is expected to work without leaking the
> driver's queues though. The driver is allowed to do allocations in
> its rx_queue_setup() function that are being freed via
> rx_queue_release() later. But rx_queue_release() is only called if you
> reconfigure the
> device with 0 queues. From what I understand there is no other way to
> reconfigure a device to use another mempool.
>
> But ... even that wouldn't work with the bonding driver right now: the
> bonding master only configures the slaves during startup. I can put
> that on my todo list.
>
> Coming back to your original problem: changing the MTU for the bond
> does work through rte_eth_dev_set_mtu() for slaves supporting that. In
> any other case you could (re-)configure rxmode.max_rx_pkt_len (and
> jumbo_frame / enable_scatter accordingly). This does work without a
> call to rte_eth_rx_queue_setup().
Thanks for suggestion, but using of rte_eth_dev_set_mtu() without
reconfiguration will require to have mempools with huge mbufs (9KB)
for all ports from the start. This is unacceptable because leads to
significant performance regressions because of fast cache exhausting.
Also this will require big work to rewrite OVS reconfiguration code
this way.
Anyway, it isn't the MTU only problem. Number of rx/tx descriptors
also can't be changed in runtime.
I'm not fully understand what is the use case for this 'reusing' code.
Could you, please, describe situation where this behaviour is necessary?
Best regards, Ilya Maximets.
>>
>>>
>>> On Wed, Sep 7, 2016 at 2:28 PM, Ilya Maximets <i.maximets@samsung.com> wrote:
>>>> This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
>>>>
>>>> It is necessary to reconfigure all queues every time because configuration
>>>> can be changed.
>>>>
>>>> For example, if we're reconfiguring bonding device with new memory pool,
>>>> already configured queues will still use the old one. And if the old
>>>> mempool be freed, application likely will panic in attempt to use
>>>> freed mempool.
>>>>
>>>> This happens when we use the bonding device with OVS 2.6 while MTU
>>>> reconfiguration:
>>>>
>>>> PANIC in rte_mempool_get_ops():
>>>> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
>>>>
>>>> Cc: <stable@dpdk.org>
>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>>> ---
>>>> drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
>>>> 1 file changed, 2 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>> index b20a272..eb5b6d1 100644
>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>> @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>>>> struct bond_rx_queue *bd_rx_q;
>>>> struct bond_tx_queue *bd_tx_q;
>>>>
>>>> - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
>>>> - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
>>>> int errval;
>>>> uint16_t q_id;
>>>>
>>>> @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>>>> }
>>>>
>>>> /* Setup Rx Queues */
>>>> - /* Use existing queues, if any */
>>>> - for (q_id = old_nb_rx_queues;
>>>> - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
>>>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
>>>> bd_rx_q = (struct bond_rx_queue *)bonded_eth_dev->data->rx_queues[q_id];
>>>>
>>>> errval = rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>>> @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>>>> }
>>>>
>>>> /* Setup Tx Queues */
>>>> - /* Use existing queues, if any */
>>>> - for (q_id = old_nb_tx_queues;
>>>> - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
>>>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
>>>> bd_tx_q = (struct bond_tx_queue *)bonded_eth_dev->data->tx_queues[q_id];
>>>>
>>>> errval = rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>>> --
>>>> 2.7.4
>>>>
>>>
>>>
>>>
>
>
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-10-06 14:32 ` Declan Doherty
@ 2016-10-19 9:55 ` Bruce Richardson
2016-10-25 13:59 ` Bruce Richardson
0 siblings, 1 reply; 32+ messages in thread
From: Bruce Richardson @ 2016-10-19 9:55 UTC (permalink / raw)
To: Declan Doherty
Cc: Ilya Maximets, dev, Heetae Ahn, Yuanhan Liu, Eric Kinzie,
Bernard Iremonger, stable
On Thu, Oct 06, 2016 at 03:32:36PM +0100, Declan Doherty wrote:
> On 07/09/16 13:28, Ilya Maximets wrote:
> > This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
> >
> > It is necessary to reconfigure all queues every time because configuration
> > can be changed.
> >
>
> Hey Ilya, this makes sense. I guess this was my original intention but I
> missed this case in my review of the change.
>
> > For example, if we're reconfiguring bonding device with new memory pool,
> > already configured queues will still use the old one. And if the old
> > mempool be freed, application likely will panic in attempt to use
> > freed mempool.
> >
> > This happens when we use the bonding device with OVS 2.6 while MTU
> > reconfiguration:
> >
> > PANIC in rte_mempool_get_ops():
> > assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
> >
> > Cc: <stable@dpdk.org>
> > Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> > ---
>
> Acked-by: Declan Doherty <declan.doherty@intel.com>
Applied to dpdk-next-net/rel_16_11
/Bruce
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-10-13 23:37 ` Eric Kinzie
@ 2016-10-24 11:02 ` Declan Doherty
2016-10-24 14:51 ` Jan Blunck
0 siblings, 1 reply; 32+ messages in thread
From: Declan Doherty @ 2016-10-24 11:02 UTC (permalink / raw)
To: Eric Kinzie, Bruce Richardson
Cc: Ilya Maximets, dev, Heetae Ahn, Yuanhan Liu, Bernard Iremonger,
stable, Thomas Monjalon
On 14/10/16 00:37, Eric Kinzie wrote:
> On Wed Oct 12 16:24:21 +0100 2016, Bruce Richardson wrote:
>> On Wed, Oct 12, 2016 at 04:24:54PM +0300, Ilya Maximets wrote:
>>> On 07.10.2016 05:02, Eric Kinzie wrote:
>>>> On Wed Sep 07 15:28:10 +0300 2016, Ilya Maximets wrote:
>>>>> This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
>>>>>
>>>>> It is necessary to reconfigure all queues every time because configuration
>>>>> can be changed.
>>>>>
>>>>> For example, if we're reconfiguring bonding device with new memory pool,
>>>>> already configured queues will still use the old one. And if the old
>>>>> mempool be freed, application likely will panic in attempt to use
>>>>> freed mempool.
>>>>>
>>>>> This happens when we use the bonding device with OVS 2.6 while MTU
>>>>> reconfiguration:
>>>>>
>>>>> PANIC in rte_mempool_get_ops():
>>>>> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
>>>>>
>>>>> Cc: <stable@dpdk.org>
>>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>>>> ---
>>>>> drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
>>>>> 1 file changed, 2 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>> index b20a272..eb5b6d1 100644
>>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>> @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>>>>> struct bond_rx_queue *bd_rx_q;
>>>>> struct bond_tx_queue *bd_tx_q;
>>>>>
>>>>> - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
>>>>> - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
>>>>> int errval;
>>>>> uint16_t q_id;
>>>>>
>>>>> @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>>>>> }
>>>>>
>>>>> /* Setup Rx Queues */
>>>>> - /* Use existing queues, if any */
>>>>> - for (q_id = old_nb_rx_queues;
>>>>> - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
>>>>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
>>>>> bd_rx_q = (struct bond_rx_queue *)bonded_eth_dev->data->rx_queues[q_id];
>>>>>
>>>>> errval = rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>>>> @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>>>>> }
>>>>>
>>>>> /* Setup Tx Queues */
>>>>> - /* Use existing queues, if any */
>>>>> - for (q_id = old_nb_tx_queues;
>>>>> - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
>>>>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
>>>>> bd_tx_q = (struct bond_tx_queue *)bonded_eth_dev->data->tx_queues[q_id];
>>>>>
>>>>> errval = rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>>>> --
>>>>> 2.7.4
>>>>>
>>>>
>>>> NAK
>>>>
>>>> There are still some users of this code. Let's give them a chance to
>>>> comment before removing it.
>>>
>>> Hi Eric,
>>>
>>> Are these users in CC-list? If not, could you, please, add them?
>>> This patch awaits in mail-list already more than a month. I think, it's enough
>>> time period for all who wants to say something. Patch fixes a real bug that
>>> prevent using of DPDK bonding in all applications that reconfigures devices
>>> in runtime including OVS.
>>>
>> Agreed.
>>
>> Eric, does reverting this patch cause you problems directly, or is your concern
>> just with regards to potential impact to others?
>>
>> Thanks,
>> /Bruce
>
> This won't impact me directly. The users are CCed (different thread)
> and I haven't seen any comment, so I no longer have any objection to
> reverting this change.
>
> Eric
>
As there has been no further objections and this reinstates the original
expected behavior of the bonding driver. I'm re-ack'ing for inclusion in
release.
Acked-by: Declan Doherty <declan.doherty@intel.com>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-10-24 11:02 ` Declan Doherty
@ 2016-10-24 14:51 ` Jan Blunck
2016-10-24 15:07 ` Declan Doherty
0 siblings, 1 reply; 32+ messages in thread
From: Jan Blunck @ 2016-10-24 14:51 UTC (permalink / raw)
To: Declan Doherty
Cc: Eric Kinzie, Bruce Richardson, Ilya Maximets, dev, Heetae Ahn,
Yuanhan Liu, Bernard Iremonger, stable, Thomas Monjalon
On Mon, Oct 24, 2016 at 7:02 AM, Declan Doherty
<declan.doherty@intel.com> wrote:
> On 14/10/16 00:37, Eric Kinzie wrote:
>>
>> On Wed Oct 12 16:24:21 +0100 2016, Bruce Richardson wrote:
>>>
>>> On Wed, Oct 12, 2016 at 04:24:54PM +0300, Ilya Maximets wrote:
>>>>
>>>> On 07.10.2016 05:02, Eric Kinzie wrote:
>>>>>
>>>>> On Wed Sep 07 15:28:10 +0300 2016, Ilya Maximets wrote:
>>>>>>
>>>>>> This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
>>>>>>
>>>>>> It is necessary to reconfigure all queues every time because
>>>>>> configuration
>>>>>> can be changed.
>>>>>>
>>>>>> For example, if we're reconfiguring bonding device with new memory
>>>>>> pool,
>>>>>> already configured queues will still use the old one. And if the old
>>>>>> mempool be freed, application likely will panic in attempt to use
>>>>>> freed mempool.
>>>>>>
>>>>>> This happens when we use the bonding device with OVS 2.6 while MTU
>>>>>> reconfiguration:
>>>>>>
>>>>>> PANIC in rte_mempool_get_ops():
>>>>>> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)"
>>>>>> failed
>>>>>>
>>>>>> Cc: <stable@dpdk.org>
>>>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>>>>> ---
>>>>>> drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
>>>>>> 1 file changed, 2 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>> index b20a272..eb5b6d1 100644
>>>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>> @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev
>>>>>> *bonded_eth_dev,
>>>>>> struct bond_rx_queue *bd_rx_q;
>>>>>> struct bond_tx_queue *bd_tx_q;
>>>>>>
>>>>>> - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
>>>>>> - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
>>>>>> int errval;
>>>>>> uint16_t q_id;
>>>>>>
>>>>>> @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev
>>>>>> *bonded_eth_dev,
>>>>>> }
>>>>>>
>>>>>> /* Setup Rx Queues */
>>>>>> - /* Use existing queues, if any */
>>>>>> - for (q_id = old_nb_rx_queues;
>>>>>> - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
>>>>>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues;
>>>>>> q_id++) {
>>>>>> bd_rx_q = (struct bond_rx_queue
>>>>>> *)bonded_eth_dev->data->rx_queues[q_id];
>>>>>>
>>>>>> errval =
>>>>>> rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>>>>> @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev
>>>>>> *bonded_eth_dev,
>>>>>> }
>>>>>>
>>>>>> /* Setup Tx Queues */
>>>>>> - /* Use existing queues, if any */
>>>>>> - for (q_id = old_nb_tx_queues;
>>>>>> - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
>>>>>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues;
>>>>>> q_id++) {
>>>>>> bd_tx_q = (struct bond_tx_queue
>>>>>> *)bonded_eth_dev->data->tx_queues[q_id];
>>>>>>
>>>>>> errval =
>>>>>> rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>>>>> --
>>>>>> 2.7.4
>>>>>>
>>>>>
>>>>> NAK
>>>>>
>>>>> There are still some users of this code. Let's give them a chance to
>>>>> comment before removing it.
>>>>
>>>>
>>>> Hi Eric,
>>>>
>>>> Are these users in CC-list? If not, could you, please, add them?
>>>> This patch awaits in mail-list already more than a month. I think, it's
>>>> enough
>>>> time period for all who wants to say something. Patch fixes a real bug
>>>> that
>>>> prevent using of DPDK bonding in all applications that reconfigures
>>>> devices
>>>> in runtime including OVS.
>>>>
>>> Agreed.
>>>
>>> Eric, does reverting this patch cause you problems directly, or is your
>>> concern
>>> just with regards to potential impact to others?
>>>
>>> Thanks,
>>> /Bruce
>>
>>
>> This won't impact me directly. The users are CCed (different thread)
>> and I haven't seen any comment, so I no longer have any objection to
>> reverting this change.
>>
>> Eric
>>
>
> As there has been no further objections and this reinstates the original
> expected behavior of the bonding driver. I'm re-ack'ing for inclusion in
> release.
>
> Acked-by: Declan Doherty <declan.doherty@intel.com>
Ok, I can revert the revert for us.
Do I read this correctly that you are not interested in fixing this properly?!
Thanks,
Jan
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-10-19 9:47 ` Ilya Maximets
@ 2016-10-24 14:54 ` Jan Blunck
2016-10-25 6:26 ` Ilya Maximets
0 siblings, 1 reply; 32+ messages in thread
From: Jan Blunck @ 2016-10-24 14:54 UTC (permalink / raw)
To: Ilya Maximets
Cc: dev, Declan Doherty, Heetae Ahn, Yuanhan Liu, Eric Kinzie,
Bernard Iremonger, stable, Dyasly Sergey
On Wed, Oct 19, 2016 at 5:47 AM, Ilya Maximets <i.maximets@samsung.com> wrote:
> On 18.10.2016 18:19, Jan Blunck wrote:
>> On Tue, Oct 18, 2016 at 2:49 PM, Ilya Maximets <i.maximets@samsung.com> wrote:
>>> On 18.10.2016 15:28, Jan Blunck wrote:
>>>> If the application already configured queues the PMD should not
>>>> silently claim ownership and reset them.
>>>>
>>>> What exactly is the problem when changing MTU? This works fine from
>>>> what I can tell.
>>>
>>> Following scenario leads to APP PANIC:
>>>
>>> 1. mempool_1 = rte_mempool_create()
>>> 2. rte_eth_rx_queue_setup(bond0, ..., mempool_1);
>>> 3. rte_eth_dev_start(bond0);
>>> 4. mempool_2 = rte_mempool_create();
>>> 5. rte_eth_dev_stop(bond0);
>>> 6. rte_eth_rx_queue_setup(bond0, ..., mempool_2);
>>> 7. rte_eth_dev_start(bond0);
>>> * RX queues still use 'mempool_1' because reconfiguration doesn't affect them. *
>>> 8. rte_mempool_free(mempool_1);
>>> 9. On any rx operation we'll get PANIC because of using freed 'mempool_1':
>>> PANIC in rte_mempool_get_ops():
>>> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
>>>
>>> You may just start OVS 2.6 with DPDK bonding device and attempt to change MTU via 'mtu_request'.
>>> Bug is easily reproducible.
>>>
>>
>> I see. I'm not 100% that this is expected to work without leaking the
>> driver's queues though. The driver is allowed to do allocations in
>> its rx_queue_setup() function that are being freed via
>> rx_queue_release() later. But rx_queue_release() is only called if you
>> reconfigure the
>> device with 0 queues. From what I understand there is no other way to
>> reconfigure a device to use another mempool.
>>
>> But ... even that wouldn't work with the bonding driver right now: the
>> bonding master only configures the slaves during startup. I can put
>> that on my todo list.
>>
>> Coming back to your original problem: changing the MTU for the bond
>> does work through rte_eth_dev_set_mtu() for slaves supporting that. In
>> any other case you could (re-)configure rxmode.max_rx_pkt_len (and
>> jumbo_frame / enable_scatter accordingly). This does work without a
>> call to rte_eth_rx_queue_setup().
>
> Thanks for suggestion, but using of rte_eth_dev_set_mtu() without
> reconfiguration will require to have mempools with huge mbufs (9KB)
> for all ports from the start. This is unacceptable because leads to
> significant performance regressions because of fast cache exhausting.
> Also this will require big work to rewrite OVS reconfiguration code
> this way.
> Anyway, it isn't the MTU only problem. Number of rx/tx descriptors
> also can't be changed in runtime.
>
>
> I'm not fully understand what is the use case for this 'reusing' code.
> Could you, please, describe situation where this behaviour is necessary?
The device that is added to the bond was used before and therefore
already has allocated queues. Therefore we reuse the existing queues
of the devices instead of borrowing the queues of the bond device. If
the slave is removed from the bond again there is no need to allocate
the queues again.
Hope that clarifies the usecase,
Jan
>
> Best regards, Ilya Maximets.
>
>>>
>>>>
>>>> On Wed, Sep 7, 2016 at 2:28 PM, Ilya Maximets <i.maximets@samsung.com> wrote:
>>>>> This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
>>>>>
>>>>> It is necessary to reconfigure all queues every time because configuration
>>>>> can be changed.
>>>>>
>>>>> For example, if we're reconfiguring bonding device with new memory pool,
>>>>> already configured queues will still use the old one. And if the old
>>>>> mempool be freed, application likely will panic in attempt to use
>>>>> freed mempool.
>>>>>
>>>>> This happens when we use the bonding device with OVS 2.6 while MTU
>>>>> reconfiguration:
>>>>>
>>>>> PANIC in rte_mempool_get_ops():
>>>>> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
>>>>>
>>>>> Cc: <stable@dpdk.org>
>>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>>>> ---
>>>>> drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
>>>>> 1 file changed, 2 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>> index b20a272..eb5b6d1 100644
>>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>> @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>>>>> struct bond_rx_queue *bd_rx_q;
>>>>> struct bond_tx_queue *bd_tx_q;
>>>>>
>>>>> - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
>>>>> - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
>>>>> int errval;
>>>>> uint16_t q_id;
>>>>>
>>>>> @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>>>>> }
>>>>>
>>>>> /* Setup Rx Queues */
>>>>> - /* Use existing queues, if any */
>>>>> - for (q_id = old_nb_rx_queues;
>>>>> - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
>>>>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
>>>>> bd_rx_q = (struct bond_rx_queue *)bonded_eth_dev->data->rx_queues[q_id];
>>>>>
>>>>> errval = rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>>>> @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>>>>> }
>>>>>
>>>>> /* Setup Tx Queues */
>>>>> - /* Use existing queues, if any */
>>>>> - for (q_id = old_nb_tx_queues;
>>>>> - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
>>>>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
>>>>> bd_tx_q = (struct bond_tx_queue *)bonded_eth_dev->data->tx_queues[q_id];
>>>>>
>>>>> errval = rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>>>> --
>>>>> 2.7.4
>>>>>
>>>>
>>>>
>>>>
>>
>>
>>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-10-24 14:51 ` Jan Blunck
@ 2016-10-24 15:07 ` Declan Doherty
2016-10-25 12:57 ` Bruce Richardson
0 siblings, 1 reply; 32+ messages in thread
From: Declan Doherty @ 2016-10-24 15:07 UTC (permalink / raw)
To: Jan Blunck
Cc: Eric Kinzie, Bruce Richardson, Ilya Maximets, dev, Heetae Ahn,
Yuanhan Liu, Bernard Iremonger, stable, Thomas Monjalon
On 24/10/16 15:51, Jan Blunck wrote:
> On Mon, Oct 24, 2016 at 7:02 AM, Declan Doherty
> <declan.doherty@intel.com> wrote:
>> On 14/10/16 00:37, Eric Kinzie wrote:
>>>
>>> On Wed Oct 12 16:24:21 +0100 2016, Bruce Richardson wrote:
>>>>
>>>> On Wed, Oct 12, 2016 at 04:24:54PM +0300, Ilya Maximets wrote:
>>>>>
>>>>> On 07.10.2016 05:02, Eric Kinzie wrote:
>>>>>>
>>>>>> On Wed Sep 07 15:28:10 +0300 2016, Ilya Maximets wrote:
>>>>>>>
>>>>>>> This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
>>>>>>>
>>>>>>> It is necessary to reconfigure all queues every time because
>>>>>>> configuration
>>>>>>> can be changed.
>>>>>>>
>>>>>>> For example, if we're reconfiguring bonding device with new memory
>>>>>>> pool,
>>>>>>> already configured queues will still use the old one. And if the old
>>>>>>> mempool be freed, application likely will panic in attempt to use
>>>>>>> freed mempool.
>>>>>>>
>>>>>>> This happens when we use the bonding device with OVS 2.6 while MTU
>>>>>>> reconfiguration:
>>>>>>>
>>>>>>> PANIC in rte_mempool_get_ops():
>>>>>>> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)"
>>>>>>> failed
>>>>>>>
>>>>>>> Cc: <stable@dpdk.org>
>>>>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>>>>>> ---
>>>>>>> drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
>>>>>>> 1 file changed, 2 insertions(+), 8 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>> index b20a272..eb5b6d1 100644
>>>>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>> @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev
>>>>>>> *bonded_eth_dev,
>>>>>>> struct bond_rx_queue *bd_rx_q;
>>>>>>> struct bond_tx_queue *bd_tx_q;
>>>>>>>
>>>>>>> - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
>>>>>>> - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
>>>>>>> int errval;
>>>>>>> uint16_t q_id;
>>>>>>>
>>>>>>> @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev
>>>>>>> *bonded_eth_dev,
>>>>>>> }
>>>>>>>
>>>>>>> /* Setup Rx Queues */
>>>>>>> - /* Use existing queues, if any */
>>>>>>> - for (q_id = old_nb_rx_queues;
>>>>>>> - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
>>>>>>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues;
>>>>>>> q_id++) {
>>>>>>> bd_rx_q = (struct bond_rx_queue
>>>>>>> *)bonded_eth_dev->data->rx_queues[q_id];
>>>>>>>
>>>>>>> errval =
>>>>>>> rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>>>>>> @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev
>>>>>>> *bonded_eth_dev,
>>>>>>> }
>>>>>>>
>>>>>>> /* Setup Tx Queues */
>>>>>>> - /* Use existing queues, if any */
>>>>>>> - for (q_id = old_nb_tx_queues;
>>>>>>> - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
>>>>>>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues;
>>>>>>> q_id++) {
>>>>>>> bd_tx_q = (struct bond_tx_queue
>>>>>>> *)bonded_eth_dev->data->tx_queues[q_id];
>>>>>>>
>>>>>>> errval =
>>>>>>> rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>>>>>> --
>>>>>>> 2.7.4
>>>>>>>
>>>>>>
>>>>>> NAK
>>>>>>
>>>>>> There are still some users of this code. Let's give them a chance to
>>>>>> comment before removing it.
>>>>>
>>>>>
>>>>> Hi Eric,
>>>>>
>>>>> Are these users in CC-list? If not, could you, please, add them?
>>>>> This patch awaits in mail-list already more than a month. I think, it's
>>>>> enough
>>>>> time period for all who wants to say something. Patch fixes a real bug
>>>>> that
>>>>> prevent using of DPDK bonding in all applications that reconfigures
>>>>> devices
>>>>> in runtime including OVS.
>>>>>
>>>> Agreed.
>>>>
>>>> Eric, does reverting this patch cause you problems directly, or is your
>>>> concern
>>>> just with regards to potential impact to others?
>>>>
>>>> Thanks,
>>>> /Bruce
>>>
>>>
>>> This won't impact me directly. The users are CCed (different thread)
>>> and I haven't seen any comment, so I no longer have any objection to
>>> reverting this change.
>>>
>>> Eric
>>>
>>
>> As there has been no further objections and this reinstates the original
>> expected behavior of the bonding driver. I'm re-ack'ing for inclusion in
>> release.
>>
>> Acked-by: Declan Doherty <declan.doherty@intel.com>
>
> Ok, I can revert the revert for us.
>
> Do I read this correctly that you are not interested in fixing this properly?!
>
> Thanks,
> Jan
>
Jan, sorry I missed the replies from last week due to the way my mail
client was filtering the conversation. Let me have another look at this
and I'll come back to the list.
Thanks
Declan
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-10-24 14:54 ` Jan Blunck
@ 2016-10-25 6:26 ` Ilya Maximets
2016-10-28 6:14 ` Ilya Maximets
0 siblings, 1 reply; 32+ messages in thread
From: Ilya Maximets @ 2016-10-25 6:26 UTC (permalink / raw)
To: Jan Blunck
Cc: dev, Declan Doherty, Heetae Ahn, Yuanhan Liu, Eric Kinzie,
Bernard Iremonger, stable, Dyasly Sergey, bruce.richardson
On 24.10.2016 17:54, Jan Blunck wrote:
> On Wed, Oct 19, 2016 at 5:47 AM, Ilya Maximets <i.maximets@samsung.com> wrote:
>> On 18.10.2016 18:19, Jan Blunck wrote:
>>> On Tue, Oct 18, 2016 at 2:49 PM, Ilya Maximets <i.maximets@samsung.com> wrote:
>>>> On 18.10.2016 15:28, Jan Blunck wrote:
>>>>> If the application already configured queues the PMD should not
>>>>> silently claim ownership and reset them.
>>>>>
>>>>> What exactly is the problem when changing MTU? This works fine from
>>>>> what I can tell.
>>>>
>>>> Following scenario leads to APP PANIC:
>>>>
>>>> 1. mempool_1 = rte_mempool_create()
>>>> 2. rte_eth_rx_queue_setup(bond0, ..., mempool_1);
>>>> 3. rte_eth_dev_start(bond0);
>>>> 4. mempool_2 = rte_mempool_create();
>>>> 5. rte_eth_dev_stop(bond0);
>>>> 6. rte_eth_rx_queue_setup(bond0, ..., mempool_2);
>>>> 7. rte_eth_dev_start(bond0);
>>>> * RX queues still use 'mempool_1' because reconfiguration doesn't affect them. *
>>>> 8. rte_mempool_free(mempool_1);
>>>> 9. On any rx operation we'll get PANIC because of using freed 'mempool_1':
>>>> PANIC in rte_mempool_get_ops():
>>>> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
>>>>
>>>> You may just start OVS 2.6 with DPDK bonding device and attempt to change MTU via 'mtu_request'.
>>>> Bug is easily reproducible.
>>>>
>>>
>>> I see. I'm not 100% that this is expected to work without leaking the
>>> driver's queues though. The driver is allowed to do allocations in
>>> its rx_queue_setup() function that are being freed via
>>> rx_queue_release() later. But rx_queue_release() is only called if you
>>> reconfigure the
>>> device with 0 queues.
It's not true. Drivers usually calls 'rx_queue_release()' inside
'rx_queue_setup()' function while reallocating the already allocated
queue. (See ixgbe driver for example). Also all HW queues are
usually destroyed inside 'eth_dev_stop()' and reallocated in
'eth_dev_start()' or '{rx,tx}_queue_setup()'.
So, there is no leaks at all.
>>> From what I understand there is no other way to
>>> reconfigure a device to use another mempool.
>>>
>>> But ... even that wouldn't work with the bonding driver right now: the
>>> bonding master only configures the slaves during startup. I can put
>>> that on my todo list.
No, bonding master reconfigures new slaves in 'rte_eth_bond_slave_add()'
if needed.
>>> Coming back to your original problem: changing the MTU for the bond
>>> does work through rte_eth_dev_set_mtu() for slaves supporting that. In
>>> any other case you could (re-)configure rxmode.max_rx_pkt_len (and
>>> jumbo_frame / enable_scatter accordingly). This does work without a
>>> call to rte_eth_rx_queue_setup().
>>
>> Thanks for suggestion, but using of rte_eth_dev_set_mtu() without
>> reconfiguration will require to have mempools with huge mbufs (9KB)
>> for all ports from the start. This is unacceptable because leads to
>> significant performance regressions because of fast cache exhausting.
>> Also this will require big work to rewrite OVS reconfiguration code
>> this way.
>> Anyway, it isn't the MTU only problem. Number of rx/tx descriptors
>> also can't be changed in runtime.
>>
>>
>> I'm not fully understand what is the use case for this 'reusing' code.
>> Could you, please, describe situation where this behaviour is necessary?
>
> The device that is added to the bond was used before and therefore
> already has allocated queues. Therefore we reuse the existing queues
> of the devices instead of borrowing the queues of the bond device. If
> the slave is removed from the bond again there is no need to allocate
> the queues again.
>
> Hope that clarifies the usecase
So, As I see, this is just an optimization that leads to differently
configured queues of same device and possible application crash if the
old configuration doesn't valid any more.
With revert applied in your usecase while adding the device to bond
it's queues will be automatically reconfigured according to configuration
of the bond device. If the slave is removed from the bond all its'
queues will remain as configured by bond. You can reconfigure them if
needed. I guess, that in your case configuration of slave devices,
actually, matches configuration of bond device. In that case slave
will remain in the same state after removing from bond as it was
before adding.
Best regards, Ilya Maximets.
>>>>>
>>>>> On Wed, Sep 7, 2016 at 2:28 PM, Ilya Maximets <i.maximets@samsung.com> wrote:
>>>>>> This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
>>>>>>
>>>>>> It is necessary to reconfigure all queues every time because configuration
>>>>>> can be changed.
>>>>>>
>>>>>> For example, if we're reconfiguring bonding device with new memory pool,
>>>>>> already configured queues will still use the old one. And if the old
>>>>>> mempool be freed, application likely will panic in attempt to use
>>>>>> freed mempool.
>>>>>>
>>>>>> This happens when we use the bonding device with OVS 2.6 while MTU
>>>>>> reconfiguration:
>>>>>>
>>>>>> PANIC in rte_mempool_get_ops():
>>>>>> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
>>>>>>
>>>>>> Cc: <stable@dpdk.org>
>>>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>>>>> ---
>>>>>> drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
>>>>>> 1 file changed, 2 insertions(+), 8 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>> index b20a272..eb5b6d1 100644
>>>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>> @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>>>>>> struct bond_rx_queue *bd_rx_q;
>>>>>> struct bond_tx_queue *bd_tx_q;
>>>>>>
>>>>>> - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
>>>>>> - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
>>>>>> int errval;
>>>>>> uint16_t q_id;
>>>>>>
>>>>>> @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>>>>>> }
>>>>>>
>>>>>> /* Setup Rx Queues */
>>>>>> - /* Use existing queues, if any */
>>>>>> - for (q_id = old_nb_rx_queues;
>>>>>> - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
>>>>>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
>>>>>> bd_rx_q = (struct bond_rx_queue *)bonded_eth_dev->data->rx_queues[q_id];
>>>>>>
>>>>>> errval = rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>>>>> @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev *bonded_eth_dev,
>>>>>> }
>>>>>>
>>>>>> /* Setup Tx Queues */
>>>>>> - /* Use existing queues, if any */
>>>>>> - for (q_id = old_nb_tx_queues;
>>>>>> - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
>>>>>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
>>>>>> bd_tx_q = (struct bond_tx_queue *)bonded_eth_dev->data->tx_queues[q_id];
>>>>>>
>>>>>> errval = rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>>>>> --
>>>>>> 2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-10-24 15:07 ` Declan Doherty
@ 2016-10-25 12:57 ` Bruce Richardson
2016-10-25 13:48 ` Declan Doherty
0 siblings, 1 reply; 32+ messages in thread
From: Bruce Richardson @ 2016-10-25 12:57 UTC (permalink / raw)
To: Declan Doherty
Cc: Jan Blunck, Eric Kinzie, Ilya Maximets, dev, Heetae Ahn,
Yuanhan Liu, Bernard Iremonger, stable, Thomas Monjalon
On Mon, Oct 24, 2016 at 04:07:17PM +0100, Declan Doherty wrote:
> On 24/10/16 15:51, Jan Blunck wrote:
> > On Mon, Oct 24, 2016 at 7:02 AM, Declan Doherty
> > <declan.doherty@intel.com> wrote:
> > > On 14/10/16 00:37, Eric Kinzie wrote:
> > > >
> > > > On Wed Oct 12 16:24:21 +0100 2016, Bruce Richardson wrote:
> > > > >
> > > > > On Wed, Oct 12, 2016 at 04:24:54PM +0300, Ilya Maximets wrote:
> > > > > >
> > > > > > On 07.10.2016 05:02, Eric Kinzie wrote:
> > > > > > >
> > > > > > > On Wed Sep 07 15:28:10 +0300 2016, Ilya Maximets wrote:
> > > > > > > >
> > > > > > > > This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
> > > > > > > >
> > > > > > > > It is necessary to reconfigure all queues every time because
> > > > > > > > configuration
> > > > > > > > can be changed.
> > > > > > > >
> > > > > > > > For example, if we're reconfiguring bonding device with new memory
> > > > > > > > pool,
> > > > > > > > already configured queues will still use the old one. And if the old
> > > > > > > > mempool be freed, application likely will panic in attempt to use
> > > > > > > > freed mempool.
> > > > > > > >
> > > > > > > > This happens when we use the bonding device with OVS 2.6 while MTU
> > > > > > > > reconfiguration:
> > > > > > > >
> > > > > > > > PANIC in rte_mempool_get_ops():
> > > > > > > > assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)"
> > > > > > > > failed
> > > > > > > >
> > > > > > > > Cc: <stable@dpdk.org>
> > > > > > > > Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> > > > > > > > ---
> > > > > > > > drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
> > > > > > > > 1 file changed, 2 insertions(+), 8 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
> > > > > > > > b/drivers/net/bonding/rte_eth_bond_pmd.c
> > > > > > > > index b20a272..eb5b6d1 100644
> > > > > > > > --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> > > > > > > > +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> > > > > > > > @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev
> > > > > > > > *bonded_eth_dev,
> > > > > > > > struct bond_rx_queue *bd_rx_q;
> > > > > > > > struct bond_tx_queue *bd_tx_q;
> > > > > > > >
> > > > > > > > - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
> > > > > > > > - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
> > > > > > > > int errval;
> > > > > > > > uint16_t q_id;
> > > > > > > >
> > > > > > > > @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev
> > > > > > > > *bonded_eth_dev,
> > > > > > > > }
> > > > > > > >
> > > > > > > > /* Setup Rx Queues */
> > > > > > > > - /* Use existing queues, if any */
> > > > > > > > - for (q_id = old_nb_rx_queues;
> > > > > > > > - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> > > > > > > > + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues;
> > > > > > > > q_id++) {
> > > > > > > > bd_rx_q = (struct bond_rx_queue
> > > > > > > > *)bonded_eth_dev->data->rx_queues[q_id];
> > > > > > > >
> > > > > > > > errval =
> > > > > > > > rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
> > > > > > > > @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev
> > > > > > > > *bonded_eth_dev,
> > > > > > > > }
> > > > > > > >
> > > > > > > > /* Setup Tx Queues */
> > > > > > > > - /* Use existing queues, if any */
> > > > > > > > - for (q_id = old_nb_tx_queues;
> > > > > > > > - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
> > > > > > > > + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues;
> > > > > > > > q_id++) {
> > > > > > > > bd_tx_q = (struct bond_tx_queue
> > > > > > > > *)bonded_eth_dev->data->tx_queues[q_id];
> > > > > > > >
> > > > > > > > errval =
> > > > > > > > rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
> > > > > > > > --
> > > > > > > > 2.7.4
> > > > > > > >
> > > > > > >
> > > > > > > NAK
> > > > > > >
> > > > > > > There are still some users of this code. Let's give them a chance to
> > > > > > > comment before removing it.
> > > > > >
> > > > > >
> > > > > > Hi Eric,
> > > > > >
> > > > > > Are these users in CC-list? If not, could you, please, add them?
> > > > > > This patch awaits in mail-list already more than a month. I think, it's
> > > > > > enough
> > > > > > time period for all who wants to say something. Patch fixes a real bug
> > > > > > that
> > > > > > prevent using of DPDK bonding in all applications that reconfigures
> > > > > > devices
> > > > > > in runtime including OVS.
> > > > > >
> > > > > Agreed.
> > > > >
> > > > > Eric, does reverting this patch cause you problems directly, or is your
> > > > > concern
> > > > > just with regards to potential impact to others?
> > > > >
> > > > > Thanks,
> > > > > /Bruce
> > > >
> > > >
> > > > This won't impact me directly. The users are CCed (different thread)
> > > > and I haven't seen any comment, so I no longer have any objection to
> > > > reverting this change.
> > > >
> > > > Eric
> > > >
> > >
> > > As there has been no further objections and this reinstates the original
> > > expected behavior of the bonding driver. I'm re-ack'ing for inclusion in
> > > release.
> > >
> > > Acked-by: Declan Doherty <declan.doherty@intel.com>
> >
> > Ok, I can revert the revert for us.
> >
> > Do I read this correctly that you are not interested in fixing this properly?!
> >
> > Thanks,
> > Jan
> >
>
> Jan, sorry I missed the replies from last week due to the way my mail client
> was filtering the conversation. Let me have another look at this and I'll
> come back to the list.
>
> Thanks
> Declan
While this patch has already been applied to dpdk-next-net tree, it
appears that there is still some ongoing discussion about it. I'm
therefore planning to pull it back out of the tree for rc2. If a
subsequent consensus is reached we can see about including it in rc3.
Declan, as maintainer, does this seem reasonable to you.
Regards,
/Bruce
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-10-25 12:57 ` Bruce Richardson
@ 2016-10-25 13:48 ` Declan Doherty
2016-10-25 14:00 ` Bruce Richardson
0 siblings, 1 reply; 32+ messages in thread
From: Declan Doherty @ 2016-10-25 13:48 UTC (permalink / raw)
To: Bruce Richardson
Cc: Jan Blunck, Eric Kinzie, Ilya Maximets, dev, Heetae Ahn,
Yuanhan Liu, Bernard Iremonger, stable, Thomas Monjalon
On 25/10/16 13:57, Bruce Richardson wrote:
> On Mon, Oct 24, 2016 at 04:07:17PM +0100, Declan Doherty wrote:
>> On 24/10/16 15:51, Jan Blunck wrote:
>>> On Mon, Oct 24, 2016 at 7:02 AM, Declan Doherty
>>> <declan.doherty@intel.com> wrote:
>>>> On 14/10/16 00:37, Eric Kinzie wrote:
>>>>>
>>>>> On Wed Oct 12 16:24:21 +0100 2016, Bruce Richardson wrote:
>>>>>>
>>>>>> On Wed, Oct 12, 2016 at 04:24:54PM +0300, Ilya Maximets wrote:
>>>>>>>
>>>>>>> On 07.10.2016 05:02, Eric Kinzie wrote:
>>>>>>>>
>>>>>>>> On Wed Sep 07 15:28:10 +0300 2016, Ilya Maximets wrote:
>>>>>>>>>
>>>>>>>>> This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
>>>>>>>>>
>>>>>>>>> It is necessary to reconfigure all queues every time because
>>>>>>>>> configuration
>>>>>>>>> can be changed.
>>>>>>>>>
>>>>>>>>> For example, if we're reconfiguring bonding device with new memory
>>>>>>>>> pool,
>>>>>>>>> already configured queues will still use the old one. And if the old
>>>>>>>>> mempool be freed, application likely will panic in attempt to use
>>>>>>>>> freed mempool.
>>>>>>>>>
>>>>>>>>> This happens when we use the bonding device with OVS 2.6 while MTU
>>>>>>>>> reconfiguration:
>>>>>>>>>
>>>>>>>>> PANIC in rte_mempool_get_ops():
>>>>>>>>> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)"
>>>>>>>>> failed
>>>>>>>>>
>>>>>>>>> Cc: <stable@dpdk.org>
>>>>>>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>>>>>>>> ---
>>>>>>>>> drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
>>>>>>>>> 1 file changed, 2 insertions(+), 8 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>>>> index b20a272..eb5b6d1 100644
>>>>>>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>>>> @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev
>>>>>>>>> *bonded_eth_dev,
>>>>>>>>> struct bond_rx_queue *bd_rx_q;
>>>>>>>>> struct bond_tx_queue *bd_tx_q;
>>>>>>>>>
>>>>>>>>> - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
>>>>>>>>> - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
>>>>>>>>> int errval;
>>>>>>>>> uint16_t q_id;
>>>>>>>>>
>>>>>>>>> @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev
>>>>>>>>> *bonded_eth_dev,
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> /* Setup Rx Queues */
>>>>>>>>> - /* Use existing queues, if any */
>>>>>>>>> - for (q_id = old_nb_rx_queues;
>>>>>>>>> - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
>>>>>>>>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues;
>>>>>>>>> q_id++) {
>>>>>>>>> bd_rx_q = (struct bond_rx_queue
>>>>>>>>> *)bonded_eth_dev->data->rx_queues[q_id];
>>>>>>>>>
>>>>>>>>> errval =
>>>>>>>>> rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>>>>>>>> @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev
>>>>>>>>> *bonded_eth_dev,
>>>>>>>>> }
>>>>>>>>>
>>>>>>>>> /* Setup Tx Queues */
>>>>>>>>> - /* Use existing queues, if any */
>>>>>>>>> - for (q_id = old_nb_tx_queues;
>>>>>>>>> - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
>>>>>>>>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues;
>>>>>>>>> q_id++) {
>>>>>>>>> bd_tx_q = (struct bond_tx_queue
>>>>>>>>> *)bonded_eth_dev->data->tx_queues[q_id];
>>>>>>>>>
>>>>>>>>> errval =
>>>>>>>>> rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>>>>>>>> --
>>>>>>>>> 2.7.4
>>>>>>>>>
>>>>>>>>
>>>>>>>> NAK
>>>>>>>>
>>>>>>>> There are still some users of this code. Let's give them a chance to
>>>>>>>> comment before removing it.
>>>>>>>
>>>>>>>
>>>>>>> Hi Eric,
>>>>>>>
>>>>>>> Are these users in CC-list? If not, could you, please, add them?
>>>>>>> This patch awaits in mail-list already more than a month. I think, it's
>>>>>>> enough
>>>>>>> time period for all who wants to say something. Patch fixes a real bug
>>>>>>> that
>>>>>>> prevent using of DPDK bonding in all applications that reconfigures
>>>>>>> devices
>>>>>>> in runtime including OVS.
>>>>>>>
>>>>>> Agreed.
>>>>>>
>>>>>> Eric, does reverting this patch cause you problems directly, or is your
>>>>>> concern
>>>>>> just with regards to potential impact to others?
>>>>>>
>>>>>> Thanks,
>>>>>> /Bruce
>>>>>
>>>>>
>>>>> This won't impact me directly. The users are CCed (different thread)
>>>>> and I haven't seen any comment, so I no longer have any objection to
>>>>> reverting this change.
>>>>>
>>>>> Eric
>>>>>
>>>>
>>>> As there has been no further objections and this reinstates the original
>>>> expected behavior of the bonding driver. I'm re-ack'ing for inclusion in
>>>> release.
>>>>
>>>> Acked-by: Declan Doherty <declan.doherty@intel.com>
>>>
>>> Ok, I can revert the revert for us.
>>>
>>> Do I read this correctly that you are not interested in fixing this properly?!
>>>
>>> Thanks,
>>> Jan
>>>
>>
>> Jan, sorry I missed the replies from last week due to the way my mail client
>> was filtering the conversation. Let me have another look at this and I'll
>> come back to the list.
>>
>> Thanks
>> Declan
>
> While this patch has already been applied to dpdk-next-net tree, it
> appears that there is still some ongoing discussion about it. I'm
> therefore planning to pull it back out of the tree for rc2. If a
> subsequent consensus is reached we can see about including it in rc3.
>
> Declan, as maintainer, does this seem reasonable to you.
>
> Regards,
> /Bruce
>
Hey Bruce, that seems reasonable, I would like to discuss this further
with Jan and Ilya.
Declan
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-10-19 9:55 ` Bruce Richardson
@ 2016-10-25 13:59 ` Bruce Richardson
0 siblings, 0 replies; 32+ messages in thread
From: Bruce Richardson @ 2016-10-25 13:59 UTC (permalink / raw)
To: Declan Doherty
Cc: Ilya Maximets, dev, Heetae Ahn, Yuanhan Liu, Eric Kinzie,
Bernard Iremonger, stable
On Wed, Oct 19, 2016 at 10:55:25AM +0100, Bruce Richardson wrote:
> On Thu, Oct 06, 2016 at 03:32:36PM +0100, Declan Doherty wrote:
> > On 07/09/16 13:28, Ilya Maximets wrote:
> > > This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
> > >
> > > It is necessary to reconfigure all queues every time because configuration
> > > can be changed.
> > >
> >
> > Hey Ilya, this makes sense. I guess this was my original intention but I
> > missed this case in my review of the change.
> >
> > > For example, if we're reconfiguring bonding device with new memory pool,
> > > already configured queues will still use the old one. And if the old
> > > mempool be freed, application likely will panic in attempt to use
> > > freed mempool.
> > >
> > > This happens when we use the bonding device with OVS 2.6 while MTU
> > > reconfiguration:
> > >
> > > PANIC in rte_mempool_get_ops():
> > > assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
> > >
> > > Cc: <stable@dpdk.org>
> > > Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> > > ---
> >
> > Acked-by: Declan Doherty <declan.doherty@intel.com>
>
> Applied to dpdk-next-net/rel_16_11
>
Patch taken out of branch due to on-going discussion in this thread. It
can be re-applied later if consensus is reached that it is ok.
Apologies for any confusion caused by this, but after discussion with
the driver maintainer, we feel this is the safest course for now.
/Bruce
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-10-25 13:48 ` Declan Doherty
@ 2016-10-25 14:00 ` Bruce Richardson
2016-11-21 11:30 ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
0 siblings, 1 reply; 32+ messages in thread
From: Bruce Richardson @ 2016-10-25 14:00 UTC (permalink / raw)
To: Declan Doherty
Cc: Jan Blunck, Eric Kinzie, Ilya Maximets, dev, Heetae Ahn,
Yuanhan Liu, Bernard Iremonger, stable, Thomas Monjalon
On Tue, Oct 25, 2016 at 02:48:04PM +0100, Declan Doherty wrote:
> On 25/10/16 13:57, Bruce Richardson wrote:
> > On Mon, Oct 24, 2016 at 04:07:17PM +0100, Declan Doherty wrote:
> > > On 24/10/16 15:51, Jan Blunck wrote:
> > > > On Mon, Oct 24, 2016 at 7:02 AM, Declan Doherty
> > > > <declan.doherty@intel.com> wrote:
> > > > > On 14/10/16 00:37, Eric Kinzie wrote:
> > > > > >
> > > > > > On Wed Oct 12 16:24:21 +0100 2016, Bruce Richardson wrote:
> > > > > > >
> > > > > > > On Wed, Oct 12, 2016 at 04:24:54PM +0300, Ilya Maximets wrote:
> > > > > > > >
> > > > > > > > On 07.10.2016 05:02, Eric Kinzie wrote:
> > > > > > > > >
> > > > > > > > > On Wed Sep 07 15:28:10 +0300 2016, Ilya Maximets wrote:
> > > > > > > > > >
> > > > > > > > > > This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
> > > > > > > > > >
> > > > > > > > > > It is necessary to reconfigure all queues every time because
> > > > > > > > > > configuration
> > > > > > > > > > can be changed.
> > > > > > > > > >
> > > > > > > > > > For example, if we're reconfiguring bonding device with new memory
> > > > > > > > > > pool,
> > > > > > > > > > already configured queues will still use the old one. And if the old
> > > > > > > > > > mempool be freed, application likely will panic in attempt to use
> > > > > > > > > > freed mempool.
> > > > > > > > > >
> > > > > > > > > > This happens when we use the bonding device with OVS 2.6 while MTU
> > > > > > > > > > reconfiguration:
> > > > > > > > > >
> > > > > > > > > > PANIC in rte_mempool_get_ops():
> > > > > > > > > > assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)"
> > > > > > > > > > failed
> > > > > > > > > >
> > > > > > > > > > Cc: <stable@dpdk.org>
> > > > > > > > > > Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> > > > > > > > > > ---
> > > > > > > > > > drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
> > > > > > > > > > 1 file changed, 2 insertions(+), 8 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
> > > > > > > > > > b/drivers/net/bonding/rte_eth_bond_pmd.c
> > > > > > > > > > index b20a272..eb5b6d1 100644
> > > > > > > > > > --- a/drivers/net/bonding/rte_eth_bond_pmd.c
> > > > > > > > > > +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
> > > > > > > > > > @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev
> > > > > > > > > > *bonded_eth_dev,
> > > > > > > > > > struct bond_rx_queue *bd_rx_q;
> > > > > > > > > > struct bond_tx_queue *bd_tx_q;
> > > > > > > > > >
> > > > > > > > > > - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
> > > > > > > > > > - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
> > > > > > > > > > int errval;
> > > > > > > > > > uint16_t q_id;
> > > > > > > > > >
> > > > > > > > > > @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev
> > > > > > > > > > *bonded_eth_dev,
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > /* Setup Rx Queues */
> > > > > > > > > > - /* Use existing queues, if any */
> > > > > > > > > > - for (q_id = old_nb_rx_queues;
> > > > > > > > > > - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
> > > > > > > > > > + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues;
> > > > > > > > > > q_id++) {
> > > > > > > > > > bd_rx_q = (struct bond_rx_queue
> > > > > > > > > > *)bonded_eth_dev->data->rx_queues[q_id];
> > > > > > > > > >
> > > > > > > > > > errval =
> > > > > > > > > > rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
> > > > > > > > > > @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev
> > > > > > > > > > *bonded_eth_dev,
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > /* Setup Tx Queues */
> > > > > > > > > > - /* Use existing queues, if any */
> > > > > > > > > > - for (q_id = old_nb_tx_queues;
> > > > > > > > > > - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
> > > > > > > > > > + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues;
> > > > > > > > > > q_id++) {
> > > > > > > > > > bd_tx_q = (struct bond_tx_queue
> > > > > > > > > > *)bonded_eth_dev->data->tx_queues[q_id];
> > > > > > > > > >
> > > > > > > > > > errval =
> > > > > > > > > > rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
> > > > > > > > > > --
> > > > > > > > > > 2.7.4
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > NAK
> > > > > > > > >
> > > > > > > > > There are still some users of this code. Let's give them a chance to
> > > > > > > > > comment before removing it.
> > > > > > > >
> > > > > > > >
> > > > > > > > Hi Eric,
> > > > > > > >
> > > > > > > > Are these users in CC-list? If not, could you, please, add them?
> > > > > > > > This patch awaits in mail-list already more than a month. I think, it's
> > > > > > > > enough
> > > > > > > > time period for all who wants to say something. Patch fixes a real bug
> > > > > > > > that
> > > > > > > > prevent using of DPDK bonding in all applications that reconfigures
> > > > > > > > devices
> > > > > > > > in runtime including OVS.
> > > > > > > >
> > > > > > > Agreed.
> > > > > > >
> > > > > > > Eric, does reverting this patch cause you problems directly, or is your
> > > > > > > concern
> > > > > > > just with regards to potential impact to others?
> > > > > > >
> > > > > > > Thanks,
> > > > > > > /Bruce
> > > > > >
> > > > > >
> > > > > > This won't impact me directly. The users are CCed (different thread)
> > > > > > and I haven't seen any comment, so I no longer have any objection to
> > > > > > reverting this change.
> > > > > >
> > > > > > Eric
> > > > > >
> > > > >
> > > > > As there has been no further objections and this reinstates the original
> > > > > expected behavior of the bonding driver. I'm re-ack'ing for inclusion in
> > > > > release.
> > > > >
> > > > > Acked-by: Declan Doherty <declan.doherty@intel.com>
> > > >
> > > > Ok, I can revert the revert for us.
> > > >
> > > > Do I read this correctly that you are not interested in fixing this properly?!
> > > >
> > > > Thanks,
> > > > Jan
> > > >
> > >
> > > Jan, sorry I missed the replies from last week due to the way my mail client
> > > was filtering the conversation. Let me have another look at this and I'll
> > > come back to the list.
> > >
> > > Thanks
> > > Declan
> >
> > While this patch has already been applied to dpdk-next-net tree, it
> > appears that there is still some ongoing discussion about it. I'm
> > therefore planning to pull it back out of the tree for rc2. If a
> > subsequent consensus is reached we can see about including it in rc3.
> >
> > Declan, as maintainer, does this seem reasonable to you.
> >
> > Regards,
> > /Bruce
> >
>
>
> Hey Bruce, that seems reasonable, I would like to discuss this further with
> Jan and Ilya.
>
Done. Hopefully consensus on a correct solution for this driver can be
reached soon.
Regards,
/Bruce
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-10-25 6:26 ` Ilya Maximets
@ 2016-10-28 6:14 ` Ilya Maximets
2016-11-11 9:16 ` Ilya Maximets
0 siblings, 1 reply; 32+ messages in thread
From: Ilya Maximets @ 2016-10-28 6:14 UTC (permalink / raw)
To: Jan Blunck; +Cc: Dyasly Sergey, stable, dev
On 25.10.2016 09:26, Ilya Maximets wrote:
> On 24.10.2016 17:54, Jan Blunck wrote:
>> On Wed, Oct 19, 2016 at 5:47 AM, Ilya Maximets <i.maximets@samsung.com> wrote:
>>> On 18.10.2016 18:19, Jan Blunck wrote:
>>>> On Tue, Oct 18, 2016 at 2:49 PM, Ilya Maximets <i.maximets@samsung.com> wrote:
>>>>> On 18.10.2016 15:28, Jan Blunck wrote:
>>>>>> If the application already configured queues the PMD should not
>>>>>> silently claim ownership and reset them.
>>>>>>
>>>>>> What exactly is the problem when changing MTU? This works fine from
>>>>>> what I can tell.
>>>>>
>>>>> Following scenario leads to APP PANIC:
>>>>>
>>>>> 1. mempool_1 = rte_mempool_create()
>>>>> 2. rte_eth_rx_queue_setup(bond0, ..., mempool_1);
>>>>> 3. rte_eth_dev_start(bond0);
>>>>> 4. mempool_2 = rte_mempool_create();
>>>>> 5. rte_eth_dev_stop(bond0);
>>>>> 6. rte_eth_rx_queue_setup(bond0, ..., mempool_2);
>>>>> 7. rte_eth_dev_start(bond0);
>>>>> * RX queues still use 'mempool_1' because reconfiguration doesn't affect them. *
>>>>> 8. rte_mempool_free(mempool_1);
>>>>> 9. On any rx operation we'll get PANIC because of using freed 'mempool_1':
>>>>> PANIC in rte_mempool_get_ops():
>>>>> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
>>>>>
>>>>> You may just start OVS 2.6 with DPDK bonding device and attempt to change MTU via 'mtu_request'.
>>>>> Bug is easily reproducible.
>>>>>
>>>>
>>>> I see. I'm not 100% that this is expected to work without leaking the
>>>> driver's queues though. The driver is allowed to do allocations in
>>>> its rx_queue_setup() function that are being freed via
>>>> rx_queue_release() later. But rx_queue_release() is only called if you
>>>> reconfigure the
>>>> device with 0 queues.
>
> It's not true. Drivers usually calls 'rx_queue_release()' inside
> 'rx_queue_setup()' function while reallocating the already allocated
> queue. (See ixgbe driver for example). Also all HW queues are
> usually destroyed inside 'eth_dev_stop()' and reallocated in
> 'eth_dev_start()' or '{rx,tx}_queue_setup()'.
> So, there is no leaks at all.
>
>>>> From what I understand there is no other way to
>>>> reconfigure a device to use another mempool.
>>>>
>>>> But ... even that wouldn't work with the bonding driver right now: the
>>>> bonding master only configures the slaves during startup. I can put
>>>> that on my todo list.
>
> No, bonding master reconfigures new slaves in 'rte_eth_bond_slave_add()'
> if needed.
>
>>>> Coming back to your original problem: changing the MTU for the bond
>>>> does work through rte_eth_dev_set_mtu() for slaves supporting that. In
>>>> any other case you could (re-)configure rxmode.max_rx_pkt_len (and
>>>> jumbo_frame / enable_scatter accordingly). This does work without a
>>>> call to rte_eth_rx_queue_setup().
>>>
>>> Thanks for suggestion, but using of rte_eth_dev_set_mtu() without
>>> reconfiguration will require to have mempools with huge mbufs (9KB)
>>> for all ports from the start. This is unacceptable because leads to
>>> significant performance regressions because of fast cache exhausting.
>>> Also this will require big work to rewrite OVS reconfiguration code
>>> this way.
>>> Anyway, it isn't the MTU only problem. Number of rx/tx descriptors
>>> also can't be changed in runtime.
>>>
>>>
>>> I'm not fully understand what is the use case for this 'reusing' code.
>>> Could you, please, describe situation where this behaviour is necessary?
>>
>> The device that is added to the bond was used before and therefore
>> already has allocated queues. Therefore we reuse the existing queues
>> of the devices instead of borrowing the queues of the bond device. If
>> the slave is removed from the bond again there is no need to allocate
>> the queues again.
>>
>> Hope that clarifies the usecase
>
> So, As I see, this is just an optimization that leads to differently
> configured queues of same device and possible application crash if the
> old configuration doesn't valid any more.
>
> With revert applied in your usecase while adding the device to bond
> it's queues will be automatically reconfigured according to configuration
> of the bond device. If the slave is removed from the bond all its'
> queues will remain as configured by bond. You can reconfigure them if
> needed. I guess, that in your case configuration of slave devices,
> actually, matches configuration of bond device. In that case slave
> will remain in the same state after removing from bond as it was
> before adding.
So, Jan, Declan, what do you think about this?
Best regards, Ilya Maximets.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-10-28 6:14 ` Ilya Maximets
@ 2016-11-11 9:16 ` Ilya Maximets
0 siblings, 0 replies; 32+ messages in thread
From: Ilya Maximets @ 2016-11-11 9:16 UTC (permalink / raw)
To: Jan Blunck, Declan Doherty, bruce.richardson
Cc: dev, Heetae Ahn, Yuanhan Liu, Eric Kinzie, Bernard Iremonger,
stable, Dyasly Sergey, Thomas Monjalon
Ping.
On 28.10.2016 09:14, Ilya Maximets wrote:
> On 25.10.2016 09:26, Ilya Maximets wrote:
>> On 24.10.2016 17:54, Jan Blunck wrote:
>>> On Wed, Oct 19, 2016 at 5:47 AM, Ilya Maximets <i.maximets@samsung.com> wrote:
>>>> On 18.10.2016 18:19, Jan Blunck wrote:
>>>>> On Tue, Oct 18, 2016 at 2:49 PM, Ilya Maximets <i.maximets@samsung.com> wrote:
>>>>>> On 18.10.2016 15:28, Jan Blunck wrote:
>>>>>>> If the application already configured queues the PMD should not
>>>>>>> silently claim ownership and reset them.
>>>>>>>
>>>>>>> What exactly is the problem when changing MTU? This works fine from
>>>>>>> what I can tell.
>>>>>>
>>>>>> Following scenario leads to APP PANIC:
>>>>>>
>>>>>> 1. mempool_1 = rte_mempool_create()
>>>>>> 2. rte_eth_rx_queue_setup(bond0, ..., mempool_1);
>>>>>> 3. rte_eth_dev_start(bond0);
>>>>>> 4. mempool_2 = rte_mempool_create();
>>>>>> 5. rte_eth_dev_stop(bond0);
>>>>>> 6. rte_eth_rx_queue_setup(bond0, ..., mempool_2);
>>>>>> 7. rte_eth_dev_start(bond0);
>>>>>> * RX queues still use 'mempool_1' because reconfiguration doesn't affect them. *
>>>>>> 8. rte_mempool_free(mempool_1);
>>>>>> 9. On any rx operation we'll get PANIC because of using freed 'mempool_1':
>>>>>> PANIC in rte_mempool_get_ops():
>>>>>> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)" failed
>>>>>>
>>>>>> You may just start OVS 2.6 with DPDK bonding device and attempt to change MTU via 'mtu_request'.
>>>>>> Bug is easily reproducible.
>>>>>>
>>>>>
>>>>> I see. I'm not 100% that this is expected to work without leaking the
>>>>> driver's queues though. The driver is allowed to do allocations in
>>>>> its rx_queue_setup() function that are being freed via
>>>>> rx_queue_release() later. But rx_queue_release() is only called if you
>>>>> reconfigure the
>>>>> device with 0 queues.
>>
>> It's not true. Drivers usually calls 'rx_queue_release()' inside
>> 'rx_queue_setup()' function while reallocating the already allocated
>> queue. (See ixgbe driver for example). Also all HW queues are
>> usually destroyed inside 'eth_dev_stop()' and reallocated in
>> 'eth_dev_start()' or '{rx,tx}_queue_setup()'.
>> So, there is no leaks at all.
>>
>>>>> From what I understand there is no other way to
>>>>> reconfigure a device to use another mempool.
>>>>>
>>>>> But ... even that wouldn't work with the bonding driver right now: the
>>>>> bonding master only configures the slaves during startup. I can put
>>>>> that on my todo list.
>>
>> No, bonding master reconfigures new slaves in 'rte_eth_bond_slave_add()'
>> if needed.
>>
>>>>> Coming back to your original problem: changing the MTU for the bond
>>>>> does work through rte_eth_dev_set_mtu() for slaves supporting that. In
>>>>> any other case you could (re-)configure rxmode.max_rx_pkt_len (and
>>>>> jumbo_frame / enable_scatter accordingly). This does work without a
>>>>> call to rte_eth_rx_queue_setup().
>>>>
>>>> Thanks for suggestion, but using of rte_eth_dev_set_mtu() without
>>>> reconfiguration will require to have mempools with huge mbufs (9KB)
>>>> for all ports from the start. This is unacceptable because leads to
>>>> significant performance regressions because of fast cache exhausting.
>>>> Also this will require big work to rewrite OVS reconfiguration code
>>>> this way.
>>>> Anyway, it isn't the MTU only problem. Number of rx/tx descriptors
>>>> also can't be changed in runtime.
>>>>
>>>>
>>>> I'm not fully understand what is the use case for this 'reusing' code.
>>>> Could you, please, describe situation where this behaviour is necessary?
>>>
>>> The device that is added to the bond was used before and therefore
>>> already has allocated queues. Therefore we reuse the existing queues
>>> of the devices instead of borrowing the queues of the bond device. If
>>> the slave is removed from the bond again there is no need to allocate
>>> the queues again.
>>>
>>> Hope that clarifies the usecase
>>
>> So, As I see, this is just an optimization that leads to differently
>> configured queues of same device and possible application crash if the
>> old configuration doesn't valid any more.
>>
>> With revert applied in your usecase while adding the device to bond
>> it's queues will be automatically reconfigured according to configuration
>> of the bond device. If the slave is removed from the bond all its'
>> queues will remain as configured by bond. You can reconfigure them if
>> needed. I guess, that in your case configuration of slave devices,
>> actually, matches configuration of bond device. In that case slave
>> will remain in the same state after removing from bond as it was
>> before adding.
>
> So, Jan, Declan, what do you think about this?
>
> Best regards, Ilya Maximets.
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [dpdk-stable] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-10-25 14:00 ` Bruce Richardson
@ 2016-11-21 11:30 ` Ferruh Yigit
2016-11-21 11:39 ` Jan Blunck
0 siblings, 1 reply; 32+ messages in thread
From: Ferruh Yigit @ 2016-11-21 11:30 UTC (permalink / raw)
To: Bruce Richardson, Declan Doherty
Cc: Jan Blunck, Eric Kinzie, Ilya Maximets, dev, Heetae Ahn,
Yuanhan Liu, Bernard Iremonger, stable, Thomas Monjalon
On 10/25/2016 3:00 PM, Bruce Richardson wrote:
> On Tue, Oct 25, 2016 at 02:48:04PM +0100, Declan Doherty wrote:
>> On 25/10/16 13:57, Bruce Richardson wrote:
>>> On Mon, Oct 24, 2016 at 04:07:17PM +0100, Declan Doherty wrote:
>>>> On 24/10/16 15:51, Jan Blunck wrote:
>>>>> On Mon, Oct 24, 2016 at 7:02 AM, Declan Doherty
>>>>> <declan.doherty@intel.com> wrote:
>>>>>> On 14/10/16 00:37, Eric Kinzie wrote:
>>>>>>>
>>>>>>> On Wed Oct 12 16:24:21 +0100 2016, Bruce Richardson wrote:
>>>>>>>>
>>>>>>>> On Wed, Oct 12, 2016 at 04:24:54PM +0300, Ilya Maximets wrote:
>>>>>>>>>
>>>>>>>>> On 07.10.2016 05:02, Eric Kinzie wrote:
>>>>>>>>>>
>>>>>>>>>> On Wed Sep 07 15:28:10 +0300 2016, Ilya Maximets wrote:
>>>>>>>>>>>
>>>>>>>>>>> This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
>>>>>>>>>>>
>>>>>>>>>>> It is necessary to reconfigure all queues every time because
>>>>>>>>>>> configuration
>>>>>>>>>>> can be changed.
>>>>>>>>>>>
>>>>>>>>>>> For example, if we're reconfiguring bonding device with new memory
>>>>>>>>>>> pool,
>>>>>>>>>>> already configured queues will still use the old one. And if the old
>>>>>>>>>>> mempool be freed, application likely will panic in attempt to use
>>>>>>>>>>> freed mempool.
>>>>>>>>>>>
>>>>>>>>>>> This happens when we use the bonding device with OVS 2.6 while MTU
>>>>>>>>>>> reconfiguration:
>>>>>>>>>>>
>>>>>>>>>>> PANIC in rte_mempool_get_ops():
>>>>>>>>>>> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)"
>>>>>>>>>>> failed
>>>>>>>>>>>
>>>>>>>>>>> Cc: <stable@dpdk.org>
>>>>>>>>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>>>>>>>>>> ---
>>>>>>>>>>> drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
>>>>>>>>>>> 1 file changed, 2 insertions(+), 8 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>>>>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>>>>>> index b20a272..eb5b6d1 100644
>>>>>>>>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>>>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>>>>>> @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev
>>>>>>>>>>> *bonded_eth_dev,
>>>>>>>>>>> struct bond_rx_queue *bd_rx_q;
>>>>>>>>>>> struct bond_tx_queue *bd_tx_q;
>>>>>>>>>>>
>>>>>>>>>>> - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
>>>>>>>>>>> - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
>>>>>>>>>>> int errval;
>>>>>>>>>>> uint16_t q_id;
>>>>>>>>>>>
>>>>>>>>>>> @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev
>>>>>>>>>>> *bonded_eth_dev,
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> /* Setup Rx Queues */
>>>>>>>>>>> - /* Use existing queues, if any */
>>>>>>>>>>> - for (q_id = old_nb_rx_queues;
>>>>>>>>>>> - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
>>>>>>>>>>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues;
>>>>>>>>>>> q_id++) {
>>>>>>>>>>> bd_rx_q = (struct bond_rx_queue
>>>>>>>>>>> *)bonded_eth_dev->data->rx_queues[q_id];
>>>>>>>>>>>
>>>>>>>>>>> errval =
>>>>>>>>>>> rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>>>>>>>>>> @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev
>>>>>>>>>>> *bonded_eth_dev,
>>>>>>>>>>> }
>>>>>>>>>>>
>>>>>>>>>>> /* Setup Tx Queues */
>>>>>>>>>>> - /* Use existing queues, if any */
>>>>>>>>>>> - for (q_id = old_nb_tx_queues;
>>>>>>>>>>> - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
>>>>>>>>>>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues;
>>>>>>>>>>> q_id++) {
>>>>>>>>>>> bd_tx_q = (struct bond_tx_queue
>>>>>>>>>>> *)bonded_eth_dev->data->tx_queues[q_id];
>>>>>>>>>>>
>>>>>>>>>>> errval =
>>>>>>>>>>> rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>>>>>>>>>> --
>>>>>>>>>>> 2.7.4
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> NAK
>>>>>>>>>>
>>>>>>>>>> There are still some users of this code. Let's give them a chance to
>>>>>>>>>> comment before removing it.
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hi Eric,
>>>>>>>>>
>>>>>>>>> Are these users in CC-list? If not, could you, please, add them?
>>>>>>>>> This patch awaits in mail-list already more than a month. I think, it's
>>>>>>>>> enough
>>>>>>>>> time period for all who wants to say something. Patch fixes a real bug
>>>>>>>>> that
>>>>>>>>> prevent using of DPDK bonding in all applications that reconfigures
>>>>>>>>> devices
>>>>>>>>> in runtime including OVS.
>>>>>>>>>
>>>>>>>> Agreed.
>>>>>>>>
>>>>>>>> Eric, does reverting this patch cause you problems directly, or is your
>>>>>>>> concern
>>>>>>>> just with regards to potential impact to others?
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> /Bruce
>>>>>>>
>>>>>>>
>>>>>>> This won't impact me directly. The users are CCed (different thread)
>>>>>>> and I haven't seen any comment, so I no longer have any objection to
>>>>>>> reverting this change.
>>>>>>>
>>>>>>> Eric
>>>>>>>
>>>>>>
>>>>>> As there has been no further objections and this reinstates the original
>>>>>> expected behavior of the bonding driver. I'm re-ack'ing for inclusion in
>>>>>> release.
>>>>>>
>>>>>> Acked-by: Declan Doherty <declan.doherty@intel.com>
>>>>>
>>>>> Ok, I can revert the revert for us.
>>>>>
>>>>> Do I read this correctly that you are not interested in fixing this properly?!
>>>>>
>>>>> Thanks,
>>>>> Jan
>>>>>
>>>>
>>>> Jan, sorry I missed the replies from last week due to the way my mail client
>>>> was filtering the conversation. Let me have another look at this and I'll
>>>> come back to the list.
>>>>
>>>> Thanks
>>>> Declan
>>>
>>> While this patch has already been applied to dpdk-next-net tree, it
>>> appears that there is still some ongoing discussion about it. I'm
>>> therefore planning to pull it back out of the tree for rc2. If a
>>> subsequent consensus is reached we can see about including it in rc3.
>>>
>>> Declan, as maintainer, does this seem reasonable to you.
>>>
>>> Regards,
>>> /Bruce
>>>
>>
>>
>> Hey Bruce, that seems reasonable, I would like to discuss this further with
>> Jan and Ilya.
>>
>
> Done. Hopefully consensus on a correct solution for this driver can be
> reached soon.
>
Is there an update for this patch? Is a consensus reached?
Thanks,
ferruh
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [dpdk-stable] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-11-21 11:30 ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
@ 2016-11-21 11:39 ` Jan Blunck
2016-11-21 12:49 ` Ilya Maximets
2016-11-23 19:38 ` [dpdk-dev] [PATCH 1/4] ethdev: Call rx/tx_queue_release before rx/tx_queue_setup Jan Blunck
0 siblings, 2 replies; 32+ messages in thread
From: Jan Blunck @ 2016-11-21 11:39 UTC (permalink / raw)
To: Ferruh Yigit
Cc: Bruce Richardson, Declan Doherty, Eric Kinzie, Ilya Maximets,
dev, Heetae Ahn, Yuanhan Liu, Bernard Iremonger, stable,
Thomas Monjalon
Ferruh,
I've been working on a patch but was distracted by other stuff and
therefore haven't tested it yet.
Stay tuned,
Jan
On Mon, Nov 21, 2016 at 12:30 PM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> On 10/25/2016 3:00 PM, Bruce Richardson wrote:
>> On Tue, Oct 25, 2016 at 02:48:04PM +0100, Declan Doherty wrote:
>>> On 25/10/16 13:57, Bruce Richardson wrote:
>>>> On Mon, Oct 24, 2016 at 04:07:17PM +0100, Declan Doherty wrote:
>>>>> On 24/10/16 15:51, Jan Blunck wrote:
>>>>>> On Mon, Oct 24, 2016 at 7:02 AM, Declan Doherty
>>>>>> <declan.doherty@intel.com> wrote:
>>>>>>> On 14/10/16 00:37, Eric Kinzie wrote:
>>>>>>>>
>>>>>>>> On Wed Oct 12 16:24:21 +0100 2016, Bruce Richardson wrote:
>>>>>>>>>
>>>>>>>>> On Wed, Oct 12, 2016 at 04:24:54PM +0300, Ilya Maximets wrote:
>>>>>>>>>>
>>>>>>>>>> On 07.10.2016 05:02, Eric Kinzie wrote:
>>>>>>>>>>>
>>>>>>>>>>> On Wed Sep 07 15:28:10 +0300 2016, Ilya Maximets wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> This reverts commit 5b7bb2bda5519b7800f814df64d4e015282140e5.
>>>>>>>>>>>>
>>>>>>>>>>>> It is necessary to reconfigure all queues every time because
>>>>>>>>>>>> configuration
>>>>>>>>>>>> can be changed.
>>>>>>>>>>>>
>>>>>>>>>>>> For example, if we're reconfiguring bonding device with new memory
>>>>>>>>>>>> pool,
>>>>>>>>>>>> already configured queues will still use the old one. And if the old
>>>>>>>>>>>> mempool be freed, application likely will panic in attempt to use
>>>>>>>>>>>> freed mempool.
>>>>>>>>>>>>
>>>>>>>>>>>> This happens when we use the bonding device with OVS 2.6 while MTU
>>>>>>>>>>>> reconfiguration:
>>>>>>>>>>>>
>>>>>>>>>>>> PANIC in rte_mempool_get_ops():
>>>>>>>>>>>> assert "(ops_index >= 0) && (ops_index < RTE_MEMPOOL_MAX_OPS_IDX)"
>>>>>>>>>>>> failed
>>>>>>>>>>>>
>>>>>>>>>>>> Cc: <stable@dpdk.org>
>>>>>>>>>>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>> drivers/net/bonding/rte_eth_bond_pmd.c | 10 ++--------
>>>>>>>>>>>> 1 file changed, 2 insertions(+), 8 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>>>>>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>>>>>>> index b20a272..eb5b6d1 100644
>>>>>>>>>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>>>>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>>>>>>>>>> @@ -1305,8 +1305,6 @@ slave_configure(struct rte_eth_dev
>>>>>>>>>>>> *bonded_eth_dev,
>>>>>>>>>>>> struct bond_rx_queue *bd_rx_q;
>>>>>>>>>>>> struct bond_tx_queue *bd_tx_q;
>>>>>>>>>>>>
>>>>>>>>>>>> - uint16_t old_nb_tx_queues = slave_eth_dev->data->nb_tx_queues;
>>>>>>>>>>>> - uint16_t old_nb_rx_queues = slave_eth_dev->data->nb_rx_queues;
>>>>>>>>>>>> int errval;
>>>>>>>>>>>> uint16_t q_id;
>>>>>>>>>>>>
>>>>>>>>>>>> @@ -1347,9 +1345,7 @@ slave_configure(struct rte_eth_dev
>>>>>>>>>>>> *bonded_eth_dev,
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> /* Setup Rx Queues */
>>>>>>>>>>>> - /* Use existing queues, if any */
>>>>>>>>>>>> - for (q_id = old_nb_rx_queues;
>>>>>>>>>>>> - q_id < bonded_eth_dev->data->nb_rx_queues; q_id++) {
>>>>>>>>>>>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_rx_queues;
>>>>>>>>>>>> q_id++) {
>>>>>>>>>>>> bd_rx_q = (struct bond_rx_queue
>>>>>>>>>>>> *)bonded_eth_dev->data->rx_queues[q_id];
>>>>>>>>>>>>
>>>>>>>>>>>> errval =
>>>>>>>>>>>> rte_eth_rx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>>>>>>>>>>> @@ -1365,9 +1361,7 @@ slave_configure(struct rte_eth_dev
>>>>>>>>>>>> *bonded_eth_dev,
>>>>>>>>>>>> }
>>>>>>>>>>>>
>>>>>>>>>>>> /* Setup Tx Queues */
>>>>>>>>>>>> - /* Use existing queues, if any */
>>>>>>>>>>>> - for (q_id = old_nb_tx_queues;
>>>>>>>>>>>> - q_id < bonded_eth_dev->data->nb_tx_queues; q_id++) {
>>>>>>>>>>>> + for (q_id = 0; q_id < bonded_eth_dev->data->nb_tx_queues;
>>>>>>>>>>>> q_id++) {
>>>>>>>>>>>> bd_tx_q = (struct bond_tx_queue
>>>>>>>>>>>> *)bonded_eth_dev->data->tx_queues[q_id];
>>>>>>>>>>>>
>>>>>>>>>>>> errval =
>>>>>>>>>>>> rte_eth_tx_queue_setup(slave_eth_dev->data->port_id, q_id,
>>>>>>>>>>>> --
>>>>>>>>>>>> 2.7.4
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> NAK
>>>>>>>>>>>
>>>>>>>>>>> There are still some users of this code. Let's give them a chance to
>>>>>>>>>>> comment before removing it.
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Hi Eric,
>>>>>>>>>>
>>>>>>>>>> Are these users in CC-list? If not, could you, please, add them?
>>>>>>>>>> This patch awaits in mail-list already more than a month. I think, it's
>>>>>>>>>> enough
>>>>>>>>>> time period for all who wants to say something. Patch fixes a real bug
>>>>>>>>>> that
>>>>>>>>>> prevent using of DPDK bonding in all applications that reconfigures
>>>>>>>>>> devices
>>>>>>>>>> in runtime including OVS.
>>>>>>>>>>
>>>>>>>>> Agreed.
>>>>>>>>>
>>>>>>>>> Eric, does reverting this patch cause you problems directly, or is your
>>>>>>>>> concern
>>>>>>>>> just with regards to potential impact to others?
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> /Bruce
>>>>>>>>
>>>>>>>>
>>>>>>>> This won't impact me directly. The users are CCed (different thread)
>>>>>>>> and I haven't seen any comment, so I no longer have any objection to
>>>>>>>> reverting this change.
>>>>>>>>
>>>>>>>> Eric
>>>>>>>>
>>>>>>>
>>>>>>> As there has been no further objections and this reinstates the original
>>>>>>> expected behavior of the bonding driver. I'm re-ack'ing for inclusion in
>>>>>>> release.
>>>>>>>
>>>>>>> Acked-by: Declan Doherty <declan.doherty@intel.com>
>>>>>>
>>>>>> Ok, I can revert the revert for us.
>>>>>>
>>>>>> Do I read this correctly that you are not interested in fixing this properly?!
>>>>>>
>>>>>> Thanks,
>>>>>> Jan
>>>>>>
>>>>>
>>>>> Jan, sorry I missed the replies from last week due to the way my mail client
>>>>> was filtering the conversation. Let me have another look at this and I'll
>>>>> come back to the list.
>>>>>
>>>>> Thanks
>>>>> Declan
>>>>
>>>> While this patch has already been applied to dpdk-next-net tree, it
>>>> appears that there is still some ongoing discussion about it. I'm
>>>> therefore planning to pull it back out of the tree for rc2. If a
>>>> subsequent consensus is reached we can see about including it in rc3.
>>>>
>>>> Declan, as maintainer, does this seem reasonable to you.
>>>>
>>>> Regards,
>>>> /Bruce
>>>>
>>>
>>>
>>> Hey Bruce, that seems reasonable, I would like to discuss this further with
>>> Jan and Ilya.
>>>
>>
>> Done. Hopefully consensus on a correct solution for this driver can be
>> reached soon.
>>
>
> Is there an update for this patch? Is a consensus reached?
>
> Thanks,
> ferruh
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [dpdk-stable] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-11-21 11:39 ` Jan Blunck
@ 2016-11-21 12:49 ` Ilya Maximets
2016-11-21 13:11 ` Ilya Maximets
2016-11-23 19:38 ` [dpdk-dev] [PATCH 1/4] ethdev: Call rx/tx_queue_release before rx/tx_queue_setup Jan Blunck
1 sibling, 1 reply; 32+ messages in thread
From: Ilya Maximets @ 2016-11-21 12:49 UTC (permalink / raw)
To: Jan Blunck, Ferruh Yigit
Cc: Bruce Richardson, Declan Doherty, Eric Kinzie, dev, Heetae Ahn,
Yuanhan Liu, Bernard Iremonger, stable, Thomas Monjalon
On 21.11.2016 14:39, Jan Blunck wrote:
> Ferruh,
>
> I've been working on a patch but was distracted by other stuff and
> therefore haven't tested it yet.
Jan, on what patch are you working? I don't understand.
>
> Stay tuned,
> Jan
>
> On Mon, Nov 21, 2016 at 12:30 PM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>> Is there an update for this patch? Is a consensus reached?
Ferruh, I didn't receive any response on my e-mails. I've pinged this thread
few times, but didn't receive any feedback and I have no idea what patch Jan
talking about.
Best regards, Ilya Maximets.
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [dpdk-stable] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-11-21 12:49 ` Ilya Maximets
@ 2016-11-21 13:11 ` Ilya Maximets
2016-11-23 20:35 ` Jan Blunck
0 siblings, 1 reply; 32+ messages in thread
From: Ilya Maximets @ 2016-11-21 13:11 UTC (permalink / raw)
To: Jan Blunck, Ferruh Yigit
Cc: Bruce Richardson, Declan Doherty, Eric Kinzie, dev, Heetae Ahn,
Yuanhan Liu, Bernard Iremonger, stable, Thomas Monjalon
To be clear, I still insist on applying this path as is.
Best regards, Ilya Maximets.
On 21.11.2016 15:49, Ilya Maximets wrote:
> On 21.11.2016 14:39, Jan Blunck wrote:
>> Ferruh,
>>
>> I've been working on a patch but was distracted by other stuff and
>> therefore haven't tested it yet.
>
> Jan, on what patch are you working? I don't understand.
>
>>
>> Stay tuned,
>> Jan
>>
>> On Mon, Nov 21, 2016 at 12:30 PM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>>
>>> Is there an update for this patch? Is a consensus reached?
>
> Ferruh, I didn't receive any response on my e-mails. I've pinged this thread
> few times, but didn't receive any feedback and I have no idea what patch Jan
> talking about.
>
> Best regards, Ilya Maximets.
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH 1/4] ethdev: Call rx/tx_queue_release before rx/tx_queue_setup
2016-11-21 11:39 ` Jan Blunck
2016-11-21 12:49 ` Ilya Maximets
@ 2016-11-23 19:38 ` Jan Blunck
2016-11-23 19:38 ` [dpdk-dev] [PATCH 2/4] ethdev: Free rx/tx_queues after releasing all queues Jan Blunck
` (2 more replies)
1 sibling, 3 replies; 32+ messages in thread
From: Jan Blunck @ 2016-11-23 19:38 UTC (permalink / raw)
To: dev; +Cc: ferruh.yigit, i.maximets, bruce.richardson, declan.doherty
If a queue has been setup before lets release it before we setup.
Otherwise we might leak resources.
Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
lib/librte_ether/rte_ethdev.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index fde8112..8c4b6cd 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1010,6 +1010,7 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
uint32_t mbp_buf_size;
struct rte_eth_dev *dev;
struct rte_eth_dev_info dev_info;
+ void **rxq;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
@@ -1068,6 +1069,14 @@ rte_eth_rx_queue_setup(uint8_t port_id, uint16_t rx_queue_id,
return -EINVAL;
}
+ rxq = dev->data->rx_queues;
+ if (rxq[rx_queue_id]) {
+ RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->rx_queue_release,
+ -ENOTSUP);
+ (*dev->dev_ops->rx_queue_release)(rxq[rx_queue_id]);
+ rxq[rx_queue_id] = NULL;
+ }
+
if (rx_conf == NULL)
rx_conf = &dev_info.default_rxconf;
@@ -1089,6 +1098,7 @@ rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
{
struct rte_eth_dev *dev;
struct rte_eth_dev_info dev_info;
+ void **txq;
RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
@@ -1121,6 +1131,14 @@ rte_eth_tx_queue_setup(uint8_t port_id, uint16_t tx_queue_id,
return -EINVAL;
}
+ txq = dev->data->tx_queues;
+ if (txq[tx_queue_id]) {
+ RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_queue_release,
+ -ENOTSUP);
+ (*dev->dev_ops->tx_queue_release)(txq[tx_queue_id]);
+ txq[tx_queue_id] = NULL;
+ }
+
if (tx_conf == NULL)
tx_conf = &dev_info.default_txconf;
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH 2/4] ethdev: Free rx/tx_queues after releasing all queues
2016-11-23 19:38 ` [dpdk-dev] [PATCH 1/4] ethdev: Call rx/tx_queue_release before rx/tx_queue_setup Jan Blunck
@ 2016-11-23 19:38 ` Jan Blunck
2016-11-23 19:38 ` [dpdk-dev] [PATCH 3/4] ethdev: Add DPDK internal _rte_eth_dev_reset() Jan Blunck
2016-11-23 19:38 ` [dpdk-dev] [PATCH 4/4] bond: Force reconfiguration of removed slave interfaces Jan Blunck
2 siblings, 0 replies; 32+ messages in thread
From: Jan Blunck @ 2016-11-23 19:38 UTC (permalink / raw)
To: dev; +Cc: ferruh.yigit, i.maximets, bruce.richardson, declan.doherty
If all queues are released lets also free up the dev->data->rx/tx_queues
to be able to properly reinitialize.
Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
lib/librte_ether/rte_ethdev.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 8c4b6cd..a3986ad 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -531,6 +531,9 @@ rte_eth_dev_rx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
for (i = nb_queues; i < old_nb_queues; i++)
(*dev->dev_ops->rx_queue_release)(rxq[i]);
+
+ rte_free(dev->data->rx_queues);
+ dev->data->rx_queues = NULL;
}
dev->data->nb_rx_queues = nb_queues;
return 0;
@@ -682,6 +685,9 @@ rte_eth_dev_tx_queue_config(struct rte_eth_dev *dev, uint16_t nb_queues)
for (i = nb_queues; i < old_nb_queues; i++)
(*dev->dev_ops->tx_queue_release)(txq[i]);
+
+ rte_free(dev->data->tx_queues);
+ dev->data->tx_queues = NULL;
}
dev->data->nb_tx_queues = nb_queues;
return 0;
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH 3/4] ethdev: Add DPDK internal _rte_eth_dev_reset()
2016-11-23 19:38 ` [dpdk-dev] [PATCH 1/4] ethdev: Call rx/tx_queue_release before rx/tx_queue_setup Jan Blunck
2016-11-23 19:38 ` [dpdk-dev] [PATCH 2/4] ethdev: Free rx/tx_queues after releasing all queues Jan Blunck
@ 2016-11-23 19:38 ` Jan Blunck
2016-11-23 19:38 ` [dpdk-dev] [PATCH 4/4] bond: Force reconfiguration of removed slave interfaces Jan Blunck
2 siblings, 0 replies; 32+ messages in thread
From: Jan Blunck @ 2016-11-23 19:38 UTC (permalink / raw)
To: dev; +Cc: ferruh.yigit, i.maximets, bruce.richardson, declan.doherty
This is a helper for DPDK internal users to force a reconfiguration of a
device.
Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
lib/librte_ether/rte_ethdev.c | 15 +++++++++++++++
lib/librte_ether/rte_ethdev.h | 13 +++++++++++++
lib/librte_ether/rte_ether_version.map | 6 ++++++
3 files changed, 34 insertions(+)
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index a3986ad..bd5787b 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -858,6 +858,21 @@ rte_eth_dev_configure(uint8_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
return 0;
}
+void
+_rte_eth_dev_reset(struct rte_eth_dev *dev)
+{
+ if (dev->data->dev_started) {
+ RTE_PMD_DEBUG_TRACE(
+ "port %d must be stopped to allow reset\n", port_id);
+ return;
+ }
+
+ rte_eth_dev_rx_queue_config(dev, 0);
+ rte_eth_dev_tx_queue_config(dev, 0);
+
+ memset(&dev->data->dev_conf, 0, sizeof(dev->data->dev_conf));
+}
+
static void
rte_eth_dev_config_restore(uint8_t port_id)
{
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 9678179..e0740db 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1914,6 +1914,19 @@ int rte_eth_dev_configure(uint8_t port_id, uint16_t nb_rx_queue,
uint16_t nb_tx_queue, const struct rte_eth_conf *eth_conf);
/**
+ * @internal Release a devices rx/tx queues and clear its configuration to
+ * force the user application to reconfigure it. It is for DPDK internal user
+ * only.
+ *
+ * @param dev
+ * Pointer to struct rte_eth_dev.
+ *
+ * @return
+ * void
+ */
+void _rte_eth_dev_reset(struct rte_eth_dev *dev);
+
+/**
* Allocate and set up a receive queue for an Ethernet device.
*
* The function allocates a contiguous block of memory for *nb_rx_desc*
diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
index 72be66d..0c31c5d 100644
--- a/lib/librte_ether/rte_ether_version.map
+++ b/lib/librte_ether/rte_ether_version.map
@@ -147,3 +147,9 @@ DPDK_16.11 {
rte_eth_dev_pci_remove;
} DPDK_16.07;
+
+DPDK_17.02 {
+ global:
+
+ _rte_eth_dev_reset;
+} DPDK_16.11;
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* [dpdk-dev] [PATCH 4/4] bond: Force reconfiguration of removed slave interfaces
2016-11-23 19:38 ` [dpdk-dev] [PATCH 1/4] ethdev: Call rx/tx_queue_release before rx/tx_queue_setup Jan Blunck
2016-11-23 19:38 ` [dpdk-dev] [PATCH 2/4] ethdev: Free rx/tx_queues after releasing all queues Jan Blunck
2016-11-23 19:38 ` [dpdk-dev] [PATCH 3/4] ethdev: Add DPDK internal _rte_eth_dev_reset() Jan Blunck
@ 2016-11-23 19:38 ` Jan Blunck
2 siblings, 0 replies; 32+ messages in thread
From: Jan Blunck @ 2016-11-23 19:38 UTC (permalink / raw)
To: dev; +Cc: ferruh.yigit, i.maximets, bruce.richardson, declan.doherty
After a slave interface is removed from a bond group it still has the
configuration of the bond interface. Lets enforce that the slave interface
is reconfigured after removal by resetting it.
Signed-off-by: Jan Blunck <jblunck@infradead.org>
---
drivers/net/bonding/rte_eth_bond_pmd.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c b/drivers/net/bonding/rte_eth_bond_pmd.c
index a80b6fa..e61afc9 100644
--- a/drivers/net/bonding/rte_eth_bond_pmd.c
+++ b/drivers/net/bonding/rte_eth_bond_pmd.c
@@ -1454,6 +1454,9 @@ slave_remove(struct bond_dev_private *internals,
(internals->slave_count - i - 1));
internals->slave_count--;
+
+ /* force reconfiguration of slave interfaces */
+ _rte_eth_dev_reset(slave_eth_dev);
}
static void
--
2.7.4
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [dpdk-dev] [dpdk-stable] [PATCH] Revert "bonding: use existing enslaved device queues"
2016-11-21 13:11 ` Ilya Maximets
@ 2016-11-23 20:35 ` Jan Blunck
0 siblings, 0 replies; 32+ messages in thread
From: Jan Blunck @ 2016-11-23 20:35 UTC (permalink / raw)
To: Ilya Maximets
Cc: Ferruh Yigit, Bruce Richardson, Declan Doherty, Eric Kinzie, dev,
Heetae Ahn, Yuanhan Liu, Bernard Iremonger, stable,
Thomas Monjalon
On Mon, Nov 21, 2016 at 2:11 PM, Ilya Maximets <i.maximets@samsung.com> wrote:
> To be clear, I still insist on applying this path as is.
>
That is very kind of you. Please see the patches that I've send in
reply to this thread which are required additionally to the revert you
send.
Happy Thanksgiving,
Jan
> Best regards, Ilya Maximets.
>
> On 21.11.2016 15:49, Ilya Maximets wrote:
>> On 21.11.2016 14:39, Jan Blunck wrote:
>>> Ferruh,
>>>
>>> I've been working on a patch but was distracted by other stuff and
>>> therefore haven't tested it yet.
>>
>> Jan, on what patch are you working? I don't understand.
>>
>>>
>>> Stay tuned,
>>> Jan
>>>
>>> On Mon, Nov 21, 2016 at 12:30 PM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>>>
>>>> Is there an update for this patch? Is a consensus reached?
>>
>> Ferruh, I didn't receive any response on my e-mails. I've pinged this thread
>> few times, but didn't receive any feedback and I have no idea what patch Jan
>> talking about.
>>
>> Best regards, Ilya Maximets.
>>
^ permalink raw reply [flat|nested] 32+ messages in thread
end of thread, other threads:[~2016-11-23 20:35 UTC | newest]
Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-07 12:28 [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues" Ilya Maximets
[not found] ` <CGME20160916050359eucas1p22998d07e190781e165082cdd9c917470@eucas1p2.samsung.com>
2016-09-16 5:03 ` Ilya Maximets
2016-10-06 14:32 ` Declan Doherty
2016-10-19 9:55 ` Bruce Richardson
2016-10-25 13:59 ` Bruce Richardson
2016-10-07 2:02 ` Eric Kinzie
2016-10-12 13:24 ` Ilya Maximets
2016-10-12 15:24 ` Bruce Richardson
2016-10-13 23:37 ` Eric Kinzie
2016-10-24 11:02 ` Declan Doherty
2016-10-24 14:51 ` Jan Blunck
2016-10-24 15:07 ` Declan Doherty
2016-10-25 12:57 ` Bruce Richardson
2016-10-25 13:48 ` Declan Doherty
2016-10-25 14:00 ` Bruce Richardson
2016-11-21 11:30 ` [dpdk-dev] [dpdk-stable] " Ferruh Yigit
2016-11-21 11:39 ` Jan Blunck
2016-11-21 12:49 ` Ilya Maximets
2016-11-21 13:11 ` Ilya Maximets
2016-11-23 20:35 ` Jan Blunck
2016-11-23 19:38 ` [dpdk-dev] [PATCH 1/4] ethdev: Call rx/tx_queue_release before rx/tx_queue_setup Jan Blunck
2016-11-23 19:38 ` [dpdk-dev] [PATCH 2/4] ethdev: Free rx/tx_queues after releasing all queues Jan Blunck
2016-11-23 19:38 ` [dpdk-dev] [PATCH 3/4] ethdev: Add DPDK internal _rte_eth_dev_reset() Jan Blunck
2016-11-23 19:38 ` [dpdk-dev] [PATCH 4/4] bond: Force reconfiguration of removed slave interfaces Jan Blunck
2016-10-18 12:28 ` [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues" Jan Blunck
2016-10-18 12:49 ` Ilya Maximets
2016-10-18 15:19 ` Jan Blunck
2016-10-19 9:47 ` Ilya Maximets
2016-10-24 14:54 ` Jan Blunck
2016-10-25 6:26 ` Ilya Maximets
2016-10-28 6:14 ` Ilya Maximets
2016-11-11 9:16 ` Ilya Maximets
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).