On 10/25/2016 3:00 PM, Bruce Richardson wrote:

On Tue, Oct 25, 2016 at 02:48:04PM +0100, Declan Doherty wrote:

quoted

On 25/10/16 13:57, Bruce Richardson wrote:

quoted

On Mon, Oct 24, 2016 at 04:07:17PM +0100, Declan Doherty wrote:

quoted

On 24/10/16 15:51, Jan Blunck wrote:

quoted

On Mon, Oct 24, 2016 at 7:02 AM, Declan Doherty
[off-list ref] wrote:

quoted

On 14/10/16 00:37, Eric Kinzie wrote:

quoted

On Wed Oct 12 16:24:21 +0100 2016, Bruce Richardson wrote:

quoted

On Wed, Oct 12, 2016 at 04:24:54PM +0300, Ilya Maximets wrote:

quoted

On 07.10.2016 05:02, Eric Kinzie wrote:

quoted

On Wed Sep 07 15:28:10 +0300 2016, Ilya Maximets wrote:

quoted

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: <redacted>
Signed-off-by: Ilya Maximets <redacted>
---
 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 <redacted>

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