[dpdk-dev] [PATCH 0/2] examples/ethtool: remove useless device info query

[dpdk-dev] [PATCH 0/2] examples/ethtool: remove useless device info query

[Thomas Monjalon]

Thomas Monjalon (2):
  examples/ethtool: remove query of default config
  examples/ethtool: allocate only one mempool

 examples/ethtool/ethtool-app/main.c | 36 ++++++++---------------------
 1 file changed, 10 insertions(+), 26 deletions(-)

-- 
2.20.1

[dpdk-dev] [PATCH 1/2] examples/ethtool: remove query of default config

[Thomas Monjalon]

The default config is used if the setup parameter is NULL.
No need to query the default config with rte_eth_dev_info_get().
The function call will be removed with another useless info.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 examples/ethtool/ethtool-app/main.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/examples/ethtool/ethtool-app/main.c b/examples/ethtool/ethtool-app/main.c
index dc93adfe3..e23d3afd2 100644
--- a/examples/ethtool/ethtool-app/main.c
+++ b/examples/ethtool/ethtool-app/main.c
@@ -95,7 +95,6 @@ static void setup_ports(struct app_config *app_cfg, int cnt_ports)
 	char str_name[16];
 	uint16_t nb_rxd = PORT_RX_QUEUE_SIZE;
 	uint16_t nb_txd = PORT_TX_QUEUE_SIZE;
-	struct rte_eth_txconf txconf;
 
 	memset(&cfg_port, 0, sizeof(cfg_port));
 	cfg_port.txmode.mq_mode = ETH_MQ_TX_NONE;
@@ -140,10 +139,9 @@ static void setup_ports(struct app_config *app_cfg, int cnt_ports)
 			rte_exit(EXIT_FAILURE,
 				 "rte_eth_rx_queue_setup failed"
 				);
-		txconf = dev_info.default_txconf;
 		if (rte_eth_tx_queue_setup(
 			    idx_port, 0, nb_txd,
-			    rte_eth_dev_socket_id(idx_port), &txconf) < 0)
+			    rte_eth_dev_socket_id(idx_port), NULL) < 0)
 			rte_exit(EXIT_FAILURE,
 				 "rte_eth_tx_queue_setup failed"
 				);
-- 
2.20.1

[dpdk-dev] [PATCH 2/2] examples/ethtool: allocate only one mempool

[Thomas Monjalon]

No need to allocate one mempool per port.
The number of mbufs is fixed for simplicity.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 examples/ethtool/ethtool-app/main.c | 32 ++++++++---------------------
 1 file changed, 9 insertions(+), 23 deletions(-)

diff --git a/examples/ethtool/ethtool-app/main.c b/examples/ethtool/ethtool-app/main.c
index e23d3afd2..e7185ca79 100644
--- a/examples/ethtool/ethtool-app/main.c
+++ b/examples/ethtool/ethtool-app/main.c
@@ -22,7 +22,7 @@
 #define MAX_BURST_LENGTH 32
 #define PORT_RX_QUEUE_SIZE 1024
 #define PORT_TX_QUEUE_SIZE 1024
-#define PKTPOOL_EXTRA_SIZE 512
+#define NUM_MBUFS 8191
 #define PKTPOOL_CACHE 32
 
 
@@ -38,7 +38,6 @@ struct app_port {
 	int port_active;
 	int port_dirty;
 	int idx_port;
-	struct rte_mempool *pkt_pool;
 };
 
 struct app_config {
@@ -89,36 +88,23 @@ void mark_port_newmac(int idx_port)
 static void setup_ports(struct app_config *app_cfg, int cnt_ports)
 {
 	int idx_port;
-	int size_pktpool;
 	struct rte_eth_conf cfg_port;
-	struct rte_eth_dev_info dev_info;
-	char str_name[16];
 	uint16_t nb_rxd = PORT_RX_QUEUE_SIZE;
 	uint16_t nb_txd = PORT_TX_QUEUE_SIZE;
+	struct rte_mempool *pkt_pool;
 
 	memset(&cfg_port, 0, sizeof(cfg_port));
 	cfg_port.txmode.mq_mode = ETH_MQ_TX_NONE;
 
+	pkt_pool = rte_pktmbuf_pool_create("mbuf pool",
+		NUM_MBUFS, PKTPOOL_CACHE, 0, RTE_MBUF_DEFAULT_BUF_SIZE,
+		rte_socket_id());
+	if (pkt_pool == NULL)
+		rte_exit(EXIT_FAILURE, "rte_pktmbuf_pool_create failed");
+
 	for (idx_port = 0; idx_port < cnt_ports; idx_port++) {
 		struct app_port *ptr_port = &app_cfg->ports[idx_port];
 
-		rte_eth_dev_info_get(idx_port, &dev_info);
-		size_pktpool = dev_info.rx_desc_lim.nb_max +
-			dev_info.tx_desc_lim.nb_max + PKTPOOL_EXTRA_SIZE;
-
-		snprintf(str_name, 16, "pkt_pool%i", idx_port);
-		ptr_port->pkt_pool = rte_pktmbuf_pool_create(
-			str_name,
-			size_pktpool, PKTPOOL_CACHE,
-			0,
-			RTE_MBUF_DEFAULT_BUF_SIZE,
-			rte_socket_id()
-			);
-		if (ptr_port->pkt_pool == NULL)
-			rte_exit(EXIT_FAILURE,
-				"rte_pktmbuf_pool_create failed"
-				);
-
 		printf("Init port %i..\n", idx_port);
 		ptr_port->port_active = 1;
 		ptr_port->port_dirty = 0;
@@ -135,7 +121,7 @@ static void setup_ports(struct app_config *app_cfg, int cnt_ports)
 		if (rte_eth_rx_queue_setup(
 			    idx_port, 0, nb_rxd,
 			    rte_eth_dev_socket_id(idx_port), NULL,
-			    ptr_port->pkt_pool) < 0)
+			    pkt_pool) < 0)
 			rte_exit(EXIT_FAILURE,
 				 "rte_eth_rx_queue_setup failed"
 				);
-- 
2.20.1

Re: [dpdk-dev] [PATCH 1/2] examples/ethtool: remove query of default config

[Bruce Richardson]

On Wed, Mar 13, 2019 at 11:09:09AM +0100, Thomas Monjalon wrote:
> The default config is used if the setup parameter is NULL.
> No need to query the default config with rte_eth_dev_info_get().
> The function call will be removed with another useless info.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [dpdk-dev] [PATCH 2/2] examples/ethtool: allocate only one mempool

[Bruce Richardson]

On Wed, Mar 13, 2019 at 11:09:10AM +0100, Thomas Monjalon wrote:
> No need to allocate one mempool per port.
> The number of mbufs is fixed for simplicity.
>
While it's simpler to use a fixed number of mbufs, having a fixed number
of mbufs multiplied by the number of ports is safer IMHO, and not really that
much more complicated.

Otherwise, fully agree with the principle of the change.

/Bruce

Re: [dpdk-dev] [PATCH 1/2] examples/ethtool: remove query of default config

[Rami Rosen]

On Wed, Mar 13, 2019 at 11:09:09AM +0100, Thomas Monjalon wrote:
> > The default config is used if the setup parameter is NULL.
> > No need to query the default config with rte_eth_dev_info_get().
> > The function call will be removed with another useless info.
> >
> > Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>

Reviewed-by: Rami Rosen <ramirose@gmail.com>

Re: [dpdk-dev] [PATCH 1/2] examples/ethtool: remove query of default config

[Ferruh Yigit]

On 3/13/2019 5:41 PM, Rami Rosen wrote:
> On Wed, Mar 13, 2019 at 11:09:09AM +0100, Thomas Monjalon wrote:
>>> The default config is used if the setup parameter is NULL.
>>> No need to query the default config with rte_eth_dev_info_get().
>>> The function call will be removed with another useless info.
>>>
>>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>>
>> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>>
> 
> Reviewed-by: Rami Rosen <ramirose@gmail.com>
> 

Applied to dpdk-next-net/master, thanks.

(only this patch merged, not series. Other patch seems waiting for comment.)

Re: [dpdk-dev] [PATCH 1/2] examples/ethtool: remove query of default config

[Ferruh Yigit]

On 3/13/2019 5:41 PM, Rami Rosen wrote:
> On Wed, Mar 13, 2019 at 11:09:09AM +0100, Thomas Monjalon wrote:
>>> The default config is used if the setup parameter is NULL.
>>> No need to query the default config with rte_eth_dev_info_get().
>>> The function call will be removed with another useless info.
>>>
>>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>>
>> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>>
> 
> Reviewed-by: Rami Rosen <ramirose@gmail.com>
> 

Applied to dpdk-next-net/master, thanks.

(only this patch merged, not series. Other patch seems waiting for comment.)

Re: [dpdk-dev] [PATCH 1/2] examples/ethtool: remove query of default config

[Thomas Monjalon]

20/03/2019 14:46, Ferruh Yigit:
> Applied to dpdk-next-net/master, thanks.
> 
> (only this patch merged, not series. Other patch seems waiting for comment.)

Applying half of patchset makes tracking of patches less obvious.
I prefer we avoid such practice.

Re: [dpdk-dev] [PATCH 1/2] examples/ethtool: remove query of default config

[Thomas Monjalon]

20/03/2019 14:46, Ferruh Yigit:
> Applied to dpdk-next-net/master, thanks.
> 
> (only this patch merged, not series. Other patch seems waiting for comment.)

Applying half of patchset makes tracking of patches less obvious.
I prefer we avoid such practice.

Re: [dpdk-dev] [PATCH 1/2] examples/ethtool: remove query of default config

[Ferruh Yigit]

On 3/20/2019 1:51 PM, Thomas Monjalon wrote:
> 20/03/2019 14:46, Ferruh Yigit:
>> Applied to dpdk-next-net/master, thanks.
>>
>> (only this patch merged, not series. Other patch seems waiting for comment.)
> 
> Applying half of patchset makes tracking of patches less obvious.
> I prefer we avoid such practice.

Generally I agree, this case the patches in patchset looks can be independent
and merged one was trivial, I tend to this for similar cases.
Do you prefer me drop the applied patch?

Re: [dpdk-dev] [PATCH 1/2] examples/ethtool: remove query of default config

[Ferruh Yigit]

On 3/20/2019 1:51 PM, Thomas Monjalon wrote:
> 20/03/2019 14:46, Ferruh Yigit:
>> Applied to dpdk-next-net/master, thanks.
>>
>> (only this patch merged, not series. Other patch seems waiting for comment.)
> 
> Applying half of patchset makes tracking of patches less obvious.
> I prefer we avoid such practice.

Generally I agree, this case the patches in patchset looks can be independent
and merged one was trivial, I tend to this for similar cases.
Do you prefer me drop the applied patch?

Re: [dpdk-dev] [PATCH 1/2] examples/ethtool: remove query of default config

[Thomas Monjalon]

20/03/2019 14:56, Ferruh Yigit:
> On 3/20/2019 1:51 PM, Thomas Monjalon wrote:
> > 20/03/2019 14:46, Ferruh Yigit:
> >> Applied to dpdk-next-net/master, thanks.
> >>
> >> (only this patch merged, not series. Other patch seems waiting for comment.)
> > 
> > Applying half of patchset makes tracking of patches less obvious.
> > I prefer we avoid such practice.
> 
> Generally I agree, this case the patches in patchset looks can be independent
> and merged one was trivial, I tend to this for similar cases.

Even if it is independent (it is not really in this case),
it looks weird to see half of patchset pending in patchwork.
We may spend some time to understand what happened.

> Do you prefer me drop the applied patch?

You can keep it now it's applied.

Re: [dpdk-dev] [PATCH 1/2] examples/ethtool: remove query of default config

[Thomas Monjalon]

20/03/2019 14:56, Ferruh Yigit:
> On 3/20/2019 1:51 PM, Thomas Monjalon wrote:
> > 20/03/2019 14:46, Ferruh Yigit:
> >> Applied to dpdk-next-net/master, thanks.
> >>
> >> (only this patch merged, not series. Other patch seems waiting for comment.)
> > 
> > Applying half of patchset makes tracking of patches less obvious.
> > I prefer we avoid such practice.
> 
> Generally I agree, this case the patches in patchset looks can be independent
> and merged one was trivial, I tend to this for similar cases.

Even if it is independent (it is not really in this case),
it looks weird to see half of patchset pending in patchwork.
We may spend some time to understand what happened.

> Do you prefer me drop the applied patch?

You can keep it now it's applied.