From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf0-f68.google.com (mail-lf0-f68.google.com [209.85.215.68]) by dpdk.org (Postfix) with ESMTP id 23AF02B97; Tue, 18 Oct 2016 17:19:29 +0200 (CEST) Received: by mail-lf0-f68.google.com with SMTP id b75so32945085lfg.3; Tue, 18 Oct 2016 08:19:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=pRGM+5UZe7jWFxkEtWRSjffS1SUqUik2t59MgtvjZZY=; b=fG4TGpO7A9MoZ3DpHJduWKlJy5l0eUqqowQGAlvBYVU9RU6tdA5ETWcKByz6aQOA5q 75cj01rd5zSa6OVjkM+qPURYMaFu6WWQgD7zuz5vVEsTEHgT52ibYbrq8JW1c+vxc18f EQ+7uWqGUOFUKdRlGcn10s2EyOT6H/cmggIEgnFS9tYyzjFwj4NLnFTF4Cr+k8KZwrmM 6gaeqqGuAx5ecQcH0JCfza8xBoSHn4jmkJdcOsyEl/+/WTPlR+U+cEnQI2rK1NIHnhe/ EgNdkIHYPa4ntbGUtEBbccHLFePQwAZHXjzW1Kdj7WXTisKtQF3QZiQJ0y51mfXaZSY6 LI+Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=pRGM+5UZe7jWFxkEtWRSjffS1SUqUik2t59MgtvjZZY=; b=WFqnluVtT0tEWaI07ZYm91eXBohW75WxrNyhD5cZC7Og0gz2VUfQ0eGe5/4QlhgM5V wBbElF513ly5aOXGE+qhz22yiXfoRsJyta8gK3sJUDIHX7awVzdUSfXSDtJQJGRd3tNT Q/231eCDOTGP3FhttJLyzepm7dXEK6uOUdO4otvhi85fOlWACEu+sjvAgYE8Yic4s1OX 3t2n+w3CBmAHclH6gt6O2xbkpbDjpVVCX/NU4L8yU42OAdAEtkboValy49jixXfUuljo +uKhTRbD64TEcmbC/A18BZytSjiWSxn0bEBPX7kxYvB86e4kBFI2lqiRuhtiDhyhz+HX M92g== X-Gm-Message-State: AA6/9RlZvMhULM5eAlST7DfL/o/ABvrcTVO4pbWm8fVo7Z6iTPQJOJCimaciEdjHHDKTuwTImvVIU3ryQcl/Cg== X-Received: by 10.28.73.214 with SMTP id w205mr12465463wma.86.1476803967615; Tue, 18 Oct 2016 08:19:27 -0700 (PDT) MIME-Version: 1.0 Sender: jblunck@gmail.com Received: by 10.28.216.195 with HTTP; Tue, 18 Oct 2016 08:19:26 -0700 (PDT) In-Reply-To: References: <1473251290-22053-1-git-send-email-i.maximets@samsung.com> From: Jan Blunck Date: Tue, 18 Oct 2016 17:19:26 +0200 X-Google-Sender-Auth: bXsyMAGgFaahrENt21PQVKdIVmk Message-ID: To: Ilya Maximets Cc: dev@dpdk.org, Declan Doherty , Heetae Ahn , Yuanhan Liu , Eric Kinzie , Bernard Iremonger , stable@dpdk.org Content-Type: text/plain; charset=UTF-8 Subject: Re: [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 18 Oct 2016 15:19:29 -0000 On Tue, Oct 18, 2016 at 2:49 PM, Ilya Maximets 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 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: >>> Signed-off-by: Ilya Maximets >>> --- >>> 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 >>> >> >> >>