On Tue, Oct 18, 2016 at 2:49 PM, Ilya Maximets [off-list ref] wrote:

On 18.10.2016 15:28, Jan Blunck wrote:

quoted

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.

quoted

On Wed, Sep 7, 2016 at 2:28 PM, Ilya Maximets [off-list ref] 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