From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wj0-f194.google.com (mail-wj0-f194.google.com [209.85.210.194]) by dpdk.org (Postfix) with ESMTP id 24AF158C3; Mon, 21 Nov 2016 12:39:59 +0100 (CET) Received: by mail-wj0-f194.google.com with SMTP id f8so3638952wje.2; Mon, 21 Nov 2016 03:39:59 -0800 (PST) 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=UFLkd/raOP4cenjhCi/bgVLPpkF46MChe/ZKb9dMlE8=; b=HFYY35c2UN2KOLbxdV+u8f5Oj8DaYxs1KFDsWXYxgJaLrxeXPU2uyo/yLn8DWWHJnE Ercc158pfiS4/X5OnGfowARyf8lXZH6126BU9btPX1UmdsI58+YRhmzw6vj91tRD+8av BKJcaYJvtrJ/mLBMG93PvuBzA9I9pZXKH58G8NGmMW1QIVltBaNW+ldPXeoKLAZh8/b7 9+Tc4W4BmYFRocCB+TNtJ/K4FhRnKqPrsLJSHGKZO8NuozCJrB1Sx5lcDG+W021IaN4M Vql/KXiMMUJn/Jl5x+eu8nJTRAuWkgFJWL792GGIfAklGZsNI39NrhfkyVagFD1dNJCf 6dWA== 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=UFLkd/raOP4cenjhCi/bgVLPpkF46MChe/ZKb9dMlE8=; b=LEUVhLnyrPSw6wxgw3sXV0Hz8dBYo4cA4PkZbm+G+50Ig0VcH9WYSAsKoD7NDwXvGU QRJsRbQgbAVygRRmc8fpW9t4rZZyCFdQ2twrk7oy4i8u8PFsEQVTL3k0m06nP8uaIHQi t5bevN/VkEum3iyciee0yZHTGMZPTNjbVbeBDDsVstluS/vKOL9+1+Fe9DVP7gUMeRq7 a1PSjUoxGY5xOFQh+IwoD7ze0mqat2WkCHCN2vRXfwfYUgVyiwQAGG8j6fzC1YEVeQHw CvlOdt8MlgjXO8IYscjwJf4cmQtbdGn6F5ex4A6Im4rdPx0vBNoQUJUQSgnqb+qE2oU9 jt1w== X-Gm-Message-State: AKaTC03clH75zE7d3w62PynDKaTL+D21DDV5IUV5pKsvvHds7gTECCsVztdizGTyygHF81ZdRluRNceTJpFFfg== X-Received: by 10.195.30.165 with SMTP id kf5mr8974640wjd.41.1479728399543; Mon, 21 Nov 2016 03:39:59 -0800 (PST) MIME-Version: 1.0 Sender: jblunck@gmail.com Received: by 10.28.191.8 with HTTP; Mon, 21 Nov 2016 03:39:58 -0800 (PST) In-Reply-To: <7c4bc057-93ee-42e1-b7ce-c87460aeee2a@intel.com> References: <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> <20161025125750.GB57276@bricha3-MOBL3.ger.corp.intel.com> <674cdf6a-7a92-8d1a-4caa-f2582cf1b733@intel.com> <20161025140048.GB10444@bricha3-MOBL3.ger.corp.intel.com> <7c4bc057-93ee-42e1-b7ce-c87460aeee2a@intel.com> From: Jan Blunck Date: Mon, 21 Nov 2016 12:39:58 +0100 X-Google-Sender-Auth: pnoxbdJRfKw7M-2P3Ramn7ZlXYM Message-ID: To: Ferruh Yigit Cc: Bruce Richardson , Declan Doherty , Eric Kinzie , Ilya Maximets , dev@dpdk.org, Heetae Ahn , Yuanhan Liu , Bernard Iremonger , stable@dpdk.org, Thomas Monjalon Content-Type: text/plain; charset=UTF-8 Subject: Re: [dpdk-dev] [dpdk-stable] [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, 21 Nov 2016 11:40:00 -0000 Ferruh, I've been working on a patch but was distracted by other stuff and therefore haven't tested it yet. Stay tuned, Jan On Mon, Nov 21, 2016 at 12:30 PM, Ferruh Yigit wrote: > On 10/25/2016 3:00 PM, Bruce Richardson wrote: >> On Tue, Oct 25, 2016 at 02:48:04PM +0100, Declan Doherty wrote: >>> On 25/10/16 13:57, Bruce Richardson wrote: >>>> On Mon, Oct 24, 2016 at 04:07:17PM +0100, Declan Doherty wrote: >>>>> 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 >>>> >>>> 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 >