From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 47E6311DE; Mon, 24 Oct 2016 17:14:06 +0200 (CEST) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga101.fm.intel.com with ESMTP; 24 Oct 2016 08:13:46 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,542,1473145200"; d="scan'208";a="23032607" Received: from dwdohert-dpdk.ir.intel.com ([163.33.210.152]) by fmsmga005.fm.intel.com with ESMTP; 24 Oct 2016 08:13:44 -0700 To: Jan Blunck References: <1473251290-22053-1-git-send-email-i.maximets@samsung.com> <20161007020225.GA22829@roosta.home> <1854c9f5-eedf-fc7b-a786-7526b80b6efa@samsung.com> <20161012152421.GC104428@bricha3-MOBL3> <20161013233714.GC17047@roosta> <61df7d78-c57a-d379-252a-aa7128e7e62e@intel.com> Cc: Eric Kinzie , Bruce Richardson , Ilya Maximets , dev@dpdk.org, Heetae Ahn , Yuanhan Liu , Bernard Iremonger , stable@dpdk.org, Thomas Monjalon From: Declan Doherty Message-ID: Date: Mon, 24 Oct 2016 16:07:17 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit 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: Mon, 24 Oct 2016 15:14:07 -0000 On 24/10/16 15:51, Jan Blunck wrote: > On Mon, Oct 24, 2016 at 7:02 AM, Declan Doherty > 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: >>>>>>> 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 >>>>>>> >>>>>> >>>>>> 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 > > 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