From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <3chas3@gmail.com> Received: from mail-io0-f182.google.com (mail-io0-f182.google.com [209.85.223.182]) by dpdk.org (Postfix) with ESMTP id 14E751BA8C for ; Mon, 29 Jan 2018 19:30:16 +0100 (CET) Received: by mail-io0-f182.google.com with SMTP id f34so8637710ioi.13 for ; Mon, 29 Jan 2018 10:30:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=OmNe1lcbihRnFpZDWDERc0uSVw+hJECzd97eptFmdRs=; b=Ge4XJ1XFQKGwMthHLFKYAal7Vs42kHA0wFDSV8ENkygFsKE2ZQvQD+AgD0fBzDLNUg pDPlXHMiRxAQ4RTiTyx251POxJXYE6fzQHwq+omDeFh32fEXY35W29eYHtcn2rWvTJ+q UV5bssWI1VAl8wpdL57kLhQmK1p60rASBg5m6EOCxEz7ZGmXUVkxL5BntL1/195rtsQ+ f0iSffPdeWGWK+isLyT/7J9oEZJ+8iWmpQQbe9dlpoQhKdV1YlDv1UdXUpGw15O9d8fN S7QDdINw7mvHSUX32rKHwhRH7jkq0nk2CKS8TQq4trQFfJnTOWViwbDWqqL82oSJ5oNZ Y46w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=OmNe1lcbihRnFpZDWDERc0uSVw+hJECzd97eptFmdRs=; b=KZVbn4fHyZe6kmU+6UXFpYJsw86/FPK1ECd0asLmHdZ06cnxCDQUY6zbf/qJ5y0NXp JOnbgIsVzh33eFEvQg7agpiDai/+dj1K7c78OpYuHtWctDUvA3lj4OoYjuEhYvAa5PDd OuqsMW+j9Src8gLgeoWBWAS+vK5LjNlpmKRLN4bp0kljbncKJk/0sSH+wfbKxqUSrfpP pgTKEULI/adznXQ/bAIhlenMZis+btvqAdjGAT30KhLgIrECPk2Jwis6bKO1wShz4sQ3 2gGRDVm5N1NUKecj1Bwy/3MZzQLFvX7PfGmS7fSn0WSxr3RJJx/X3JLzwOoiPA+8bBlP xUGg== X-Gm-Message-State: AKwxytcw38mHBNI//EDm4h/kmGTvFkM0mAF7pbN+ExMB6lNl4RmTk4jm WjxuanI8G7Hqq7XkhhFbE5pHknQ24tPlblesTIv2RQ== X-Google-Smtp-Source: AH8x2249MYQ6EitXQe38Cx9iMJve/CjhPPR5kWhXYMpzbyiAOC1644XyFhNVB2jcCleYzx88J4K1To7Fq5gHZXTs3f4= X-Received: by 10.107.19.140 with SMTP id 12mr28668734iot.31.1517250615408; Mon, 29 Jan 2018 10:30:15 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.200.5 with HTTP; Mon, 29 Jan 2018 10:30:14 -0800 (PST) In-Reply-To: <2601191342CEEE43887BDE71AB97725890565566@IRSMSX103.ger.corp.intel.com> References: <20180129152115.26359-1-3chas3@gmail.com> <2601191342CEEE43887BDE71AB977258905653D7@IRSMSX103.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725890565545@IRSMSX103.ger.corp.intel.com> <2601191342CEEE43887BDE71AB97725890565566@IRSMSX103.ger.corp.intel.com> From: Chas Williams <3chas3@gmail.com> Date: Mon, 29 Jan 2018 13:30:14 -0500 Message-ID: To: "Ananyev, Konstantin" Cc: "dev@dpdk.org" , "Charles (Chas) Williams" , "Lu, Wenzhuo" Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.15 Subject: Re: [dpdk-dev] [PATCH] net/ixgbe: fix reconfiguration of rx queues X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 29 Jan 2018 18:30:16 -0000 On Mon, Jan 29, 2018 at 1:05 PM, Ananyev, Konstantin < konstantin.ananyev@intel.com> wrote: > > > > -----Original Message----- > > From: Ananyev, Konstantin > > Sent: Monday, January 29, 2018 5:51 PM > > To: Ananyev, Konstantin > > Subject: [PATCH] net/ixgbe: fix reconfiguration of rx queues > > > > > > > > From: Chas Williams [mailto:3chas3@gmail.com] > > Sent: Monday, January 29, 2018 4:49 PM > > To: Ananyev, Konstantin > > Cc: dev@dpdk.org; Lu, Wenzhuo ; Charles (Chas) > Williams > > Subject: Re: [PATCH] net/ixgbe: fix reconfiguration of rx queues > > > > > > > > On Mon, Jan 29, 2018 at 11:25 AM, Ananyev, Konstantin < > konstantin.ananyev@intel.com> wrote: > > > > > -----Original Message----- > > > From: Chas Williams [mailto:3chas3@gmail.com] > > > Sent: Monday, January 29, 2018 3:21 PM > > > To: dev@dpdk.org > > > Cc: Lu, Wenzhuo ; Ananyev, Konstantin < > konstantin.ananyev@intel.com>; Charles (Chas) Williams > > > > > > Subject: [PATCH] net/ixgbe: fix reconfiguration of rx queues > > > > > > From: "Charles (Chas) Williams" > > > > > > .dev_configure() may be called again after RX queues have been setup. > > > This has the effect of clearing whatever setting the RX queues made for > > > rx_bulk_alloc_allowed or rx_vec_allowed. Only reset this configuration > > > is there aren't any currently allocated queues. > > > > > > Fixes: 01fa1d6215fa ("ixgbe: unify Rx setup") > > > > > > Signed-off-by: Chas Williams > > > --- > > > drivers/net/ixgbe/ixgbe_ethdev.c | 18 ++++++++++++++++-- > > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/net/ixgbe/ixgbe_ethdev.c > b/drivers/net/ixgbe/ixgbe_ethdev.c > > > index 37eb668..b39249a 100644 > > > --- a/drivers/net/ixgbe/ixgbe_ethdev.c > > > +++ b/drivers/net/ixgbe/ixgbe_ethdev.c > > > @@ -2348,6 +2348,7 @@ ixgbe_dev_configure(struct rte_eth_dev *dev) > > > struct ixgbe_adapter *adapter = > > > (struct ixgbe_adapter *)dev->data->dev_private; > > > int ret; > > > + uint16_t i; > > > > > > PMD_INIT_FUNC_TRACE(); > > > /* multipe queue mode checking */ > > > @@ -2363,11 +2364,17 @@ ixgbe_dev_configure(struct rte_eth_dev *dev) > > > > > > /* > > > * Initialize to TRUE. If any of Rx queues doesn't meet the bulk > > > - * allocation or vector Rx preconditions we will reset it. > > > + * allocation or vector Rx preconditions we will reset it. We > > > + * can only do this is there aren't any existing RX queues. > > > */ > > > + for (i = 0; i < dev->data->nb_rx_queues; i++) { > > > + if (dev->data->rx_queues[i]) > > > + goto out; > > > + } > > I don't see why this is needed. > > It surely should be possible to reconfigure device with different > > number of queues. > > Konstantin > > > > Yes, you can add new queues but you shouldn't reset the bulk and vec > settings > > that have already been chosen by the previously allocated queues. > > Why is that? Might be in new queue setup user will change settings? > There is no requirement that the user allocates all the RX queues in the same way. Some could have a different numbers of descs which is one of the checks in check_rx_burst_bulk_alloc_preconditions() > > > If those queues > > set rx_bulk_alloc_allowed to be false, then this is going to cause an > issue with queue > > release later on. > > Could you be a bit more specific here: > What you think will be broken in ixgbe_rx_queue_release() in that case? > Sorry, I mispoke. It's this function, ixgbe_reset_rx_queue(), /* * By default, the Rx queue setup function allocates enough memory for * IXGBE_MAX_RING_DESC. The Rx Burst bulk allocation function requires * extra memory at the end of the descriptor ring to be zero'd out. */ if (adapter->rx_bulk_alloc_allowed) /* zero out extra memory */ len += RTE_PMD_IXGBE_RX_MAX_BURST; /* * Zero out HW ring memory. Zero out extra memory at the end of * the H/W ring so look-ahead logic in Rx Burst bulk alloc function * reads extra memory as zeros. */ for (i = 0; i < len; i++) { rxq->rx_ring[i] = zeroed_desc; } So you potentially write past the rx_ring[] you allocated. You can't change rx_bulk_alloc_allowed once it has been set and you have allocated queues. In fact, you can't really let some later queue change this setting after the first queue decides what itshould be. > > > This breaks: > > > > rte_eth_dev_configure(..., 1, 1, ...); > > rx_queue_setup(1) > > [rx_queue_setup decides that it can't support rx_bulk_alloc_allowed] > > .. > > > > Later, you want to add some more queues, you call > > > > eth_eth_dev_configure(..., 2, 2, ...); > > After you call dev_configure, you'll have to do queue_setup() for all your > queues. > dev_configure() can changes some global device settings, so each queue has > to be > reconfigured. > In your example It should be: > eth_eth_dev_configure(..., 2, 2, ...); > rx_queue_setup(...,0, ...); > rx_queue_setup(...,1, ...); > > Konstantin > Nothing in the API says this must happen. If there where true, shouldn't rte_eth_dev_configure() automatically drop any existing queues? There is existing code that doesn't do this. Show me in the API where I am required to setup all my queues _again_ after calling rte_eth_dev_configure() > > > rx_queue_setup(2) > > [rx_queue_setup hopefully makes the same choice as rxqid = 1?] > > ... > > > > Is one supposed to release all queues before calling > rte_eth_dev_configure()? If > > that is true, it seems like the change_mtu examples I see are possibly > wrong. As > > suggested in kenrel_nic_interface.rst: > > > > > > ret = rte_eth_dev_configure(port_id, 1, 1, &conf); > > if (ret < 0) { > > RTE_LOG(ERR, APP, "Fail to reconfigure port %d\n", port_id); > > return ret; > > } > > > > /* Restart specific port */ > > > > ret = rte_eth_dev_start(port_id); > > if (ret < 0) { > > RTE_LOG(ERR, APP, "Fail to restart port %d\n", port_id); > > return ret; > > } > > > > This is will obviously reset the rx_bulk_alloc_allowed and not > reallocated the RX queues. > > > > > > > adapter->rx_bulk_alloc_allowed = true; > > > adapter->rx_vec_allowed = true; > > > > > > +out: > > > return 0; > > > } > > > > > > @@ -4959,6 +4966,7 @@ ixgbevf_dev_configure(struct rte_eth_dev *dev) > > > struct rte_eth_conf *conf = &dev->data->dev_conf; > > > struct ixgbe_adapter *adapter = > > > (struct ixgbe_adapter *)dev->data->dev_private; > > > + uint16_t i; > > > > > > PMD_INIT_LOG(DEBUG, "Configured Virtual Function port id: %d", > > > dev->data->port_id); > > > @@ -4981,11 +4989,17 @@ ixgbevf_dev_configure(struct rte_eth_dev *dev) > > > > > > /* > > > * Initialize to TRUE. If any of Rx queues doesn't meet the bulk > > > - * allocation or vector Rx preconditions we will reset it. > > > + * allocation or vector Rx preconditions we will reset it. We > > > + * can only do this is there aren't any existing RX queues. > > > */ > > > + for (i = 0; i < dev->data->nb_rx_queues; i++) { > > > + if (dev->data->rx_queues[i]) > > > + goto out; > > > + } > > > adapter->rx_bulk_alloc_allowed = true; > > > adapter->rx_vec_allowed = true; > > > > > > +out: > > > return 0; > > > } > > > > > > -- > > > 2.9.5 > >