| Message ID | 20160516113349.7d2a992f@miho (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Delegated to: | Thomas Monjalon |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id E5FBC8E82; Mon, 16 May 2016 11:33:57 +0200 (CEST) Received: from ernst.netinsight.se (ernst.netinsight.se [194.16.221.21]) by dpdk.org (Postfix) with SMTP id AAAAA8DB3 for <dev@dpdk.org>; Mon, 16 May 2016 11:33:56 +0200 (CEST) Received: from miho (unverified [10.100.1.152]) by ernst.netinsight.se (EMWAC SMTPRS 0.83) with SMTP id <B0033487890@ernst.netinsight.se>; Mon, 16 May 2016 11:34:12 +0200 Date: Mon, 16 May 2016 11:33:49 +0200 From: Simon Kagstrom <simon.kagstrom@netinsight.net> To: dev@dpdk.org, thomas.monjalon@6wind.com, reshma.pattan@intel.com Message-ID: <20160516113349.7d2a992f@miho> X-Mailer: Claws Mail 3.9.3 (GTK+ 2.24.27; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: [dpdk-dev] [PATCH / RFC ] ethdev: Allow rte_eth_dev_configure with zero RX/TX queues X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Simon Kagstrom
May 16, 2016, 9:33 a.m. UTC
This allows releasing RX/TX queue memory. --- We're using DPDK 16.04 and have a test suite which performs a sequence of separate tests of the type allocate mempool rte_eth_dev_configure(port, n_rxq, n_txq, ...) setup rx/tx queues rte_eth_dev_start(port) <perform actual test> stop rx/tx queues rte_eth_dev_stop(port) -> rte_eth_dev_configure(port, 0, 0, ...) check that there are no leaks from the mempool The crucial point is the marked line above. This is done so that the rx_queue_release/tx_queue_release callbacks in the PMD is called, so that mbufs allocated by the driver is released. Without this patch, this explicitly isn't allowed. Is there a particular reason why it shouldn't? It was introduced in d505ba80a165a9735f3d9d3c6ab68a7bd85f271b "ethdev: support unidirectional configuration" lib/librte_ether/rte_ethdev.c | 5 ----- 1 file changed, 5 deletions(-)
Comments
> -----Original Message----- > From: Simon Kagstrom [mailto:simon.kagstrom@netinsight.net] > Sent: Monday, May 16, 2016 10:34 AM > To: dev@dpdk.org; thomas.monjalon@6wind.com; Pattan, Reshma > <reshma.pattan@intel.com> > Subject: [PATCH / RFC ] ethdev: Allow rte_eth_dev_configure with zero RX/TX > queues > > This allows releasing RX/TX queue memory. > --- > > Without this patch, this explicitly isn't allowed. Is there a particular reason why it > shouldn't? It was introduced in > > d505ba80a165a9735f3d9d3c6ab68a7bd85f271b > > "ethdev: support unidirectional configuration" > > lib/librte_ether/rte_ethdev.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c index > a31018e..5481d45 100644 > --- a/lib/librte_ether/rte_ethdev.c > +++ b/lib/librte_ether/rte_ethdev.c > @@ -944,11 +944,6 @@ rte_eth_dev_configure(uint8_t port_id, uint16_t > nb_rx_q, uint16_t nb_tx_q, > */ > (*dev->dev_ops->dev_infos_get)(dev, &dev_info); > > - if (nb_rx_q == 0 && nb_tx_q == 0) { > - RTE_PMD_DEBUG_TRACE("ethdev port_id=%d both rx and tx > queue cannot be 0\n", port_id); > - return -EINVAL; > - } > - This was added to allow devices, at least with one direction (RX/TX) supported. As, devices with both directions disabled doesn't make sense right? Thanks, Reshma
On 2016-05-16 12:24, Pattan, Reshma wrote: >> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c index >> a31018e..5481d45 100644 >> --- a/lib/librte_ether/rte_ethdev.c >> +++ b/lib/librte_ether/rte_ethdev.c >> @@ -944,11 +944,6 @@ rte_eth_dev_configure(uint8_t port_id, uint16_t >> nb_rx_q, uint16_t nb_tx_q, >> */ >> (*dev->dev_ops->dev_infos_get)(dev, &dev_info); >> >> - if (nb_rx_q == 0 && nb_tx_q == 0) { >> - RTE_PMD_DEBUG_TRACE("ethdev port_id=%d both rx and tx >> queue cannot be 0\n", port_id); >> - return -EINVAL; >> - } > > This was added to allow devices, at least with one direction (RX/TX) supported. As, devices with both directions disabled doesn't make sense right? Well, not for running them, no. But this is a part of the shutdown procedure between tests (I should have been more clear I guess). As far as I can see in the code, rte_eth_dev_configure() is the only point which actually calls {rx,tx}_queue_release(), so without this call, we can't get the memory pool full again. So basically, our test suite looks like rte_eth_dev_configure(port, 32, 32); // For example <run a test> rte_eth_dev_configure(port, 0, 0); Check that the mempool is full again rte_eth_dev_configure(port, 32, 32); <run another test> rte_eth_dev_configure(port, 0, 0); Check that the mempool is full again ... And without this fix, the mempool check fails since a few of the buffers are tied up in the RX descriptor ring of the PMD. // Simon
> -----Original Message----- > From: Simon Kågström [mailto:simon.kagstrom@netinsight.net] > Sent: Monday, May 16, 2016 11:33 AM > To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org; > thomas.monjalon@6wind.com > Subject: Re: [PATCH / RFC ] ethdev: Allow rte_eth_dev_configure with zero > RX/TX queues > > On 2016-05-16 12:24, Pattan, Reshma wrote: > >> diff --git a/lib/librte_ether/rte_ethdev.c > >> b/lib/librte_ether/rte_ethdev.c index > >> a31018e..5481d45 100644 > >> --- a/lib/librte_ether/rte_ethdev.c > >> +++ b/lib/librte_ether/rte_ethdev.c > >> @@ -944,11 +944,6 @@ rte_eth_dev_configure(uint8_t port_id, uint16_t > >> nb_rx_q, uint16_t nb_tx_q, > >> */ > >> (*dev->dev_ops->dev_infos_get)(dev, &dev_info); > >> > >> - if (nb_rx_q == 0 && nb_tx_q == 0) { > >> - RTE_PMD_DEBUG_TRACE("ethdev port_id=%d both rx and tx > >> queue cannot be 0\n", port_id); > >> - return -EINVAL; > >> - } > > > > This was added to allow devices, at least with one direction (RX/TX) > supported. As, devices with both directions disabled doesn't make sense right? > > Well, not for running them, no. But this is a part of the shutdown procedure > between tests (I should have been more clear I guess). > Yes I understood this. But I am not sure if you can use rte_eth_dev_configure(port, 0, 0) to free the resources. Can you check if you can use rte_eth_dev_rx_queue_stop/ rte_eth_dev_tx_queue_stop to achieve the same, because they do take care of releasing mbufs, but doesn't free the queue's sw-ring and queue. Thanks, Reshma
On 2016-05-16 14:43, Pattan, Reshma wrote: >>> This was added to allow devices, at least with one direction (RX/TX) >> supported. As, devices with both directions disabled doesn't make sense right? >> >> Well, not for running them, no. But this is a part of the shutdown procedure >> between tests (I should have been more clear I guess). > > Yes I understood this. But I am not sure if you can use rte_eth_dev_configure(port, 0, 0) to free the resources. > Can you check if you can use rte_eth_dev_rx_queue_stop/ rte_eth_dev_tx_queue_stop to achieve the same, because they do take care of > releasing mbufs, but doesn't free the queue's sw-ring and queue. But isn't that very strange behavior. Aren't the descriptor rings allocated in rx_queue_setup()? If so, the sequence rx_queue_stop(); // Release buffers rx_queue_start(); would leave the descriptor ring empty after start, i.e., not able to receive data. // Simon
Ping? Any more comments on this? // Simon On 2016-05-16 15:16, Simon Kågström wrote: > On 2016-05-16 14:43, Pattan, Reshma wrote: >>>> This was added to allow devices, at least with one direction (RX/TX) >>> supported. As, devices with both directions disabled doesn't make sense right? >>> >>> Well, not for running them, no. But this is a part of the shutdown procedure >>> between tests (I should have been more clear I guess). >> >> Yes I understood this. But I am not sure if you can use rte_eth_dev_configure(port, 0, 0) to free the resources. >> Can you check if you can use rte_eth_dev_rx_queue_stop/ rte_eth_dev_tx_queue_stop to achieve the same, because they do take care of >> releasing mbufs, but doesn't free the queue's sw-ring and queue. > > But isn't that very strange behavior. Aren't the descriptor rings > allocated in rx_queue_setup()? If so, the sequence > > rx_queue_stop(); // Release buffers > rx_queue_start(); > > would leave the descriptor ring empty after start, i.e., not able to > receive data. > > // Simon >
> -----Original Message----- > From: Simon Kågström [mailto:simon.kagstrom@netinsight.net] > Sent: Friday, May 20, 2016 7:30 AM > To: Pattan, Reshma <reshma.pattan@intel.com>; dev@dpdk.org; > thomas.monjalon@6wind.com > Subject: Re: [dpdk-dev] [PATCH / RFC ] ethdev: Allow rte_eth_dev_configure > with zero RX/TX queues > > Ping? Any more comments on this? > Hi, I don't have any objections, just let's wait if any other comments from committee. Thanks, Reshma > // Simon > > On 2016-05-16 15:16, Simon Kågström wrote: > > On 2016-05-16 14:43, Pattan, Reshma wrote: > >>>> This was added to allow devices, at least with one direction > >>>> (RX/TX) > >>> supported. As, devices with both directions disabled doesn't make sense > right? > >>> > >>> Well, not for running them, no. But this is a part of the shutdown > >>> procedure between tests (I should have been more clear I guess). > >> > >> Yes I understood this. But I am not sure if you can use > rte_eth_dev_configure(port, 0, 0) to free the resources. > >> Can you check if you can use rte_eth_dev_rx_queue_stop/ > >> rte_eth_dev_tx_queue_stop to achieve the same, because they do take care > of releasing mbufs, but doesn't free the queue's sw-ring and queue. > > > > But isn't that very strange behavior. Aren't the descriptor rings > > allocated in rx_queue_setup()? If so, the sequence > > > > rx_queue_stop(); // Release buffers > > rx_queue_start(); > > > > would leave the descriptor ring empty after start, i.e., not able to > > receive data. > > > > // Simon > >
2016-05-16 11:33, Simon Kagstrom: > This allows releasing RX/TX queue memory. > --- > We're using DPDK 16.04 and have a test suite which performs a sequence > of separate tests of the type > > allocate mempool > rte_eth_dev_configure(port, n_rxq, n_txq, ...) > setup rx/tx queues > rte_eth_dev_start(port) > > <perform actual test> > > stop rx/tx queues > rte_eth_dev_stop(port) > > -> rte_eth_dev_configure(port, 0, 0, ...) > > check that there are no leaks from the mempool > > The crucial point is the marked line above. This is done so that the > rx_queue_release/tx_queue_release callbacks in the PMD is called, so > that mbufs allocated by the driver is released. I think you are trying to use a side effect of rte_eth_dev_configure(). After calling rte_eth_dev_stop(), I would say the clean-up should be done by rte_eth_dev_close(). Why not using close?
On 6/23/2016 4:53 PM, thomas.monjalon at 6wind.com (Thomas Monjalon) wrote: > 2016-05-16 11:33, Simon Kagstrom: >> This allows releasing RX/TX queue memory. >> --- >> We're using DPDK 16.04 and have a test suite which performs a sequence >> of separate tests of the type >> >> allocate mempool >> rte_eth_dev_configure(port, n_rxq, n_txq, ...) >> setup rx/tx queues >> rte_eth_dev_start(port) >> >> <perform actual test> >> >> stop rx/tx queues >> rte_eth_dev_stop(port) >> >> -> rte_eth_dev_configure(port, 0, 0, ...) >> >> check that there are no leaks from the mempool >> >> The crucial point is the marked line above. This is done so that the >> rx_queue_release/tx_queue_release callbacks in the PMD is called, so >> that mbufs allocated by the driver is released. > > I think you are trying to use a side effect of rte_eth_dev_configure(). > After calling rte_eth_dev_stop(), I would say the clean-up should be done > by rte_eth_dev_close(). > Why not using close? > Hi Simon, This patch is in patchwork for a long time without response, updating its status as rejected, if it is still relevant please let us know.
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c index a31018e..5481d45 100644 --- a/lib/librte_ether/rte_ethdev.c +++ b/lib/librte_ether/rte_ethdev.c @@ -944,11 +944,6 @@ rte_eth_dev_configure(uint8_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q, */ (*dev->dev_ops->dev_infos_get)(dev, &dev_info); - if (nb_rx_q == 0 && nb_tx_q == 0) { - RTE_PMD_DEBUG_TRACE("ethdev port_id=%d both rx and tx queue cannot be 0\n", port_id); - return -EINVAL; - } - if (nb_rx_q > dev_info.max_rx_queues) { RTE_PMD_DEBUG_TRACE("ethdev port_id=%d nb_rx_queues=%d > %d\n", port_id, nb_rx_q, dev_info.max_rx_queues);