From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout3.w1.samsung.com (mailout3.w1.samsung.com [210.118.77.13]) by dpdk.org (Postfix) with ESMTP id 562393978; Tue, 25 Oct 2016 08:26:44 +0200 (CEST) Received: from eucas1p1.samsung.com (unknown [182.198.249.206]) by mailout3.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0OFL00FY1B8I6040@mailout3.w1.samsung.com>; Tue, 25 Oct 2016 07:26:42 +0100 (BST) Received: from eusmges1.samsung.com (unknown [203.254.199.239]) by eucas1p1.samsung.com (KnoxPortal) with ESMTP id 20161025062641eucas1p14a556590b6e8bd408871c5940d895791~Asl_GQks51629516295eucas1p1O; Tue, 25 Oct 2016 06:26:41 +0000 (GMT) Received: from eucas1p1.samsung.com ( [182.198.249.206]) by eusmges1.samsung.com (EUCPMTA) with SMTP id 9A.40.23383.12BFE085; Tue, 25 Oct 2016 07:26:41 +0100 (BST) Received: from eusmgms1.samsung.com (unknown [182.198.249.179]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20161025062640eucas1p25ab52707a10116f242989708923f81a8~Asl9dpS4X1367313673eucas1p2L; Tue, 25 Oct 2016 06:26:40 +0000 (GMT) X-AuditID: cbfec7ef-f79e76d000005b57-52-580efb216b3d Received: from eusync4.samsung.com ( [203.254.199.214]) by eusmgms1.samsung.com (EUCPMTA) with SMTP id AE.41.07726.41BFE085; Tue, 25 Oct 2016 07:26:28 +0100 (BST) Received: from [106.109.129.180] by eusync4.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTPA id <0OFL00DKQB8FSO20@eusync4.samsung.com>; Tue, 25 Oct 2016 07:26:40 +0100 (BST) To: Jan Blunck Cc: dev@dpdk.org, Declan Doherty , Heetae Ahn , Yuanhan Liu , Eric Kinzie , Bernard Iremonger , stable@dpdk.org, Dyasly Sergey , bruce.richardson@intel.com From: Ilya Maximets Message-id: <03e11be9-45b2-e05f-99f1-8be1bcc05f19@samsung.com> Date: Tue, 25 Oct 2016 09:26:39 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.3.0 MIME-version: 1.0 In-reply-to: Content-type: text/plain; charset=utf-8 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrEKsWRmVeSWpSXmKPExsWy7djPc7qKv/kiDKbM1rHY3ChscWOVvcWb B00sFu8+bWeymHdqL7vFtM+32S2utP9kt2h92MpkMXm2lMW/jj/sFtcnXGB14Pb4tWApq8fO WXfZPTav0PJYvOclk8e8k4EefVtWMQawRXHZpKTmZJalFunbJXBl/D6fWTDbquLn/etsDYyP 9boYOTgkBEwkzu0I6WLkBDLFJC7cW8/WxcjFISSwjFHiyeaHjBDOZ0aJnx+XM8E0vDvoCld0 /swmFgjnBaPE3f6lzCCjhAXCJXZfWMgIYosIqEnMOLkMbBKzwG4miU2tl1hAEmwCOhKnVh8B K+IVsJOYda2bFcRmEVCV+NdwiAVkm6hAhET/GXWIEkGJH5PvgYU5BYIl/i3OBAkzC2hKvPgy iQXClpfYvOYtM8Q3x9gl3rXaQ9wsK7HpAFTYReL5vPMsELawxKvjW9ghbBmJzo6DTBB2tcTE rW3sIBdLCLQwSiyc+IMVImEvcermVSaIXXwSk7ZNZ4aYzyvR0SYEUeIh8f/uNqhdjhJXGj+w QoLnJrPEnJ3nmScwys9C8s0sJC/MQvLCAkbmVYwiqaXFuempxYZ6xYm5xaV56XrJ+bmbGIEp 5/S/4+93MD5tDjnEKMDBqMTDu0KXL0KINbGsuDL3EKMEB7OSCO+570Ah3pTEyqrUovz4otKc 1OJDjNIcLErivHsXXAkXEkhPLEnNTk0tSC2CyTJxcEo1MK4PvJvIKxt35W3s++ay/Ak7u8uX 396f1Riw/tpUdYFJx3qOFU2bcmCutqr6lRClJYuvFTKuvuRw3MJRL+/CLgdtD8242S7ihTfY 5sU/+qemyP7lwOmXh9viUx5483NvMmYUkwwMLFKcwiZu/L7UYJnj5zxDlaCE85412zQ4/grW TL3AM3XxaiWW4oxEQy3mouJEAK+lJ7w1AwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrCIsWRmVeSWpSXmKPExsVy+t/xa7oiv/kiDOau4rfY3ChscWOVvcWb B00sFu8+bWeymHdqL7vFtM+32S2utP9kt2h92MpkMXm2lMW/jj/sFtcnXGB14Pb4tWApq8fO WXfZPTav0PJYvOclk8e8k4EefVtWMQawRbnZZKQmpqQWKaTmJeenZOal2yqFhrjpWigp5CXm ptoqRej6hgQpKZQl5pQCeUYGaMDBOcA9WEnfLsEt4/f5zILZVhU/719na2B8rNfFyMEhIWAi 8e6gaxcjJ5ApJnHh3nq2LkYuDiGBJYwSDb3PWCGcF4wSe48cZQWpEhYIl9h9YSEjiC0ioCYx 4+QyRoiim8wShz50gHUwC+xmknhyYgVYB5uAjsSp1UfAOngF7CRmXesGi7MIqEr8azjEAmKL CkRI3HrYwQJRIyjxY/I9MJtTIFhi+6oZzCCnMguoS0yZkgsSZhaQl9i85i3zBEaBWUg6ZiFU zUJStYCReRWjSGppcW56brGhXnFibnFpXrpecn7uJkZgDG479nPzDsZLG4MPMQpwMCrx8K7Q 5YsQYk0sK67MPcQowcGsJMJ77jtQiDclsbIqtSg/vqg0J7X4EKMp0AsTmaVEk/OB6SGvJN7Q xNDc0tDI2MLC3MhISZy35MOVcCGB9MSS1OzU1ILUIpg+Jg5OqQZGqeY0Vd2rIpteSBdWe/zb EfvyEffOF/8XClv6Cu4+J3YjI3NDlpPqftkDPPnTzLqPTEl+/qtz+7p7N61TDSYc3DupdVPx Wtcz8w2TDwk731r/a/sMKesFtgI27FO2Tnn4V7Nudefa1Kdf3WNYI9w2yLKs/7leZ7UiT4yj nJOq4C+XR7XXTj0yUGIpzkg01GIuKk4EADv+Sf7XAgAA X-MTR: 20000000000000000@CPGS X-CMS-MailID: 20161025062640eucas1p25ab52707a10116f242989708923f81a8 X-Msg-Generator: CA X-Sender-IP: 182.198.249.179 X-Local-Sender: =?UTF-8?B?SWx5YSBNYXhpbWV0cxtTUlItVmlydHVhbGl6YXRpb24gTGFi?= =?UTF-8?B?G+yCvOyEseyghOyekBtFbmdpbmVlcg==?= X-Global-Sender: =?UTF-8?B?SWx5YSBNYXhpbWV0cxtTUlItVmlydHVhbGl6YXRpb24gTGFi?= =?UTF-8?B?G1NhbXN1bmcgRWxlY3Ryb25pY3MbRW5naW5lZXI=?= X-Sender-Code: =?UTF-8?B?QzEwG0NJU0hRG0MxMEdEMDFHRDAxMDE1NA==?= CMS-TYPE: 201P X-HopCount: 7 X-CMS-RootMailID: 20161018122856eucas1p22493fbd48031126de6093fd312e7a0fc X-RootMTR: 20161018122856eucas1p22493fbd48031126de6093fd312e7a0fc References: <1473251290-22053-1-git-send-email-i.maximets@samsung.com> Subject: Re: [dpdk-stable] [dpdk-dev] [PATCH] Revert "bonding: use existing enslaved device queues" X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches for stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 25 Oct 2016 06:26:44 -0000 On 24.10.2016 17:54, Jan Blunck wrote: > On Wed, Oct 19, 2016 at 5:47 AM, Ilya Maximets wrote: >> On 18.10.2016 18:19, Jan Blunck wrote: >>> 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. 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 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