[v2] app/testpmd: reduce memory consumption
diff mbox series

Message ID 20191122104323.28992-1-david.marchand@redhat.com
State Accepted, archived
Delegated to: Ferruh Yigit
Headers show
Series
  • [v2] app/testpmd: reduce memory consumption
Related show

Checks

Context Check Description
ci/Intel-compilation fail apply issues
ci/travis-robot success Travis build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-compilation success Compile Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/checkpatch warning coding style issues

Commit Message

David Marchand Nov. 22, 2019, 10:43 a.m. UTC
Following [1], testpmd memory consumption has skyrocketted.
The rte_port structure has gotten quite fat.

struct rte_port {
[...]
  struct rte_eth_rxconf rx_conf[65536];            /* 266280 3145728 */
  /* --- cacheline 53312 boundary (3411968 bytes) was 40 bytes ago --- */
  struct rte_eth_txconf tx_conf[65536];            /* 3412008 3670016 */
  /* --- cacheline 110656 boundary (7081984 bytes) was 40 bytes ago --- */
[...]
  /* size: 8654936, cachelines: 135234, members: 31 */
[...]

testpmd handles RTE_MAX_ETHPORTS ports (32 by default) which means that it
needs ~256MB just for this internal representation.

The reason is that a testpmd rte_port (the name is quite confusing, as
it is a local type) maintains configurations for all queues of a port.
But where you would expect testpmd to use RTE_MAX_QUEUES_PER_PORT as the
maximum queue count, the rte_port uses MAX_QUEUE_ID set to 64k.

Prefer the ethdev maximum value.

After this patch:
struct rte_port {
[...]
  struct rte_eth_rxconf      rx_conf[1025];        /*  8240 49200 */
  /* --- cacheline 897 boundary (57408 bytes) was 32 bytes ago --- */
  struct rte_eth_txconf      tx_conf[1025];        /* 57440 57400 */
  /* --- cacheline 1794 boundary (114816 bytes) was 24 bytes ago --- */
[...]
  /* size: 139488, cachelines: 2180, members: 31 */
[...]

With this, we can ask for less memory in test-null.sh.

[1]: https://git.dpdk.org/dpdk/commit/?id=436b3a6b6e62

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
Acked-by: Thomas Monjalon <thomas@monjalon.net>
---
Changelog since v1:
- updated test-null.sh

---
 app/test-pmd/testpmd.c |  6 +++---
 app/test-pmd/testpmd.h | 16 +++++++---------
 devtools/test-null.sh  |  2 +-
 3 files changed, 11 insertions(+), 13 deletions(-)

Comments

Ferruh Yigit Nov. 22, 2019, 12:24 p.m. UTC | #1
On 11/22/2019 10:43 AM, David Marchand wrote:
> diff --git a/devtools/test-null.sh b/devtools/test-null.sh
> index 9f9a459f76..6e5b1ad529 100755
> --- a/devtools/test-null.sh
> +++ b/devtools/test-null.sh
> @@ -25,6 +25,6 @@ else
>  fi
>  
>  (sleep 1 && echo stop) |
> -$testpmd -c $coremask --no-huge -m 150 \
> +$testpmd -c $coremask --no-huge -m 20 \
>  	$libs --vdev net_null1 --vdev net_null2 $eal_options -- \
>  	--no-mlockall --total-num-mbufs=2048 $testpmd_options -ia

What do you think to separate this part, and go with first version of the patchset?

And I am not sure if we should update at all, what is the benefit?

Also script fails after update, because of the additional physical devices and
their memory requirement, it is possible to make it run with additional testpmd
argument but fails by default.
Thomas Monjalon Nov. 22, 2019, 1:12 p.m. UTC | #2
22/11/2019 13:24, Ferruh Yigit:
> On 11/22/2019 10:43 AM, David Marchand wrote:
> > diff --git a/devtools/test-null.sh b/devtools/test-null.sh
> > index 9f9a459f76..6e5b1ad529 100755
> > --- a/devtools/test-null.sh
> > +++ b/devtools/test-null.sh
> > @@ -25,6 +25,6 @@ else
> >  fi
> >  
> >  (sleep 1 && echo stop) |
> > -$testpmd -c $coremask --no-huge -m 150 \
> > +$testpmd -c $coremask --no-huge -m 20 \
> >  	$libs --vdev net_null1 --vdev net_null2 $eal_options -- \
> >  	--no-mlockall --total-num-mbufs=2048 $testpmd_options -ia
> 
> What do you think to separate this part, and go with first version of the patchset?
> 
> And I am not sure if we should update at all, what is the benefit?

The benefit it to test that there is no anormal memory needs.

> Also script fails after update, because of the additional physical devices and
> their memory requirement, it is possible to make it run with additional testpmd
> argument but fails by default.

This test is supposed to be for null devices only.
Why are you trying to add physical devices?
Oh wait, we miss an option -w 0 to be in whitelist mode?
Ferruh Yigit Nov. 22, 2019, 1:14 p.m. UTC | #3
On 11/22/2019 1:12 PM, Thomas Monjalon wrote:
> 22/11/2019 13:24, Ferruh Yigit:
>> On 11/22/2019 10:43 AM, David Marchand wrote:
>>> diff --git a/devtools/test-null.sh b/devtools/test-null.sh
>>> index 9f9a459f76..6e5b1ad529 100755
>>> --- a/devtools/test-null.sh
>>> +++ b/devtools/test-null.sh
>>> @@ -25,6 +25,6 @@ else
>>>  fi
>>>  
>>>  (sleep 1 && echo stop) |
>>> -$testpmd -c $coremask --no-huge -m 150 \
>>> +$testpmd -c $coremask --no-huge -m 20 \
>>>  	$libs --vdev net_null1 --vdev net_null2 $eal_options -- \
>>>  	--no-mlockall --total-num-mbufs=2048 $testpmd_options -ia
>>
>> What do you think to separate this part, and go with first version of the patchset?
>>
>> And I am not sure if we should update at all, what is the benefit?

Fair enough.

> 
> The benefit it to test that there is no anormal memory needs.
> 
>> Also script fails after update, because of the additional physical devices and
>> their memory requirement, it is possible to make it run with additional testpmd
>> argument but fails by default.
> 
> This test is supposed to be for null devices only.
> Why are you trying to add physical devices?
> Oh wait, we miss an option -w 0 to be in whitelist mode?
> 
I am not trying to add physical devices, they are already there in my
environment, yes adding "-w" can solve it.
Ferruh Yigit Nov. 22, 2019, 2:14 p.m. UTC | #4
On 11/22/2019 10:43 AM, David Marchand wrote:
> Following [1], testpmd memory consumption has skyrocketted.
> The rte_port structure has gotten quite fat.
> 
> struct rte_port {
> [...]
>   struct rte_eth_rxconf rx_conf[65536];            /* 266280 3145728 */
>   /* --- cacheline 53312 boundary (3411968 bytes) was 40 bytes ago --- */
>   struct rte_eth_txconf tx_conf[65536];            /* 3412008 3670016 */
>   /* --- cacheline 110656 boundary (7081984 bytes) was 40 bytes ago --- */
> [...]
>   /* size: 8654936, cachelines: 135234, members: 31 */
> [...]
> 
> testpmd handles RTE_MAX_ETHPORTS ports (32 by default) which means that it
> needs ~256MB just for this internal representation.
> 
> The reason is that a testpmd rte_port (the name is quite confusing, as
> it is a local type) maintains configurations for all queues of a port.
> But where you would expect testpmd to use RTE_MAX_QUEUES_PER_PORT as the
> maximum queue count, the rte_port uses MAX_QUEUE_ID set to 64k.
> 
> Prefer the ethdev maximum value.
> 
> After this patch:
> struct rte_port {
> [...]
>   struct rte_eth_rxconf      rx_conf[1025];        /*  8240 49200 */
>   /* --- cacheline 897 boundary (57408 bytes) was 32 bytes ago --- */
>   struct rte_eth_txconf      tx_conf[1025];        /* 57440 57400 */
>   /* --- cacheline 1794 boundary (114816 bytes) was 24 bytes ago --- */
> [...]
>   /* size: 139488, cachelines: 2180, members: 31 */
> [...]
> 
> With this, we can ask for less memory in test-null.sh.
> 
> [1]: https://git.dpdk.org/dpdk/commit/?id=436b3a6b6e62
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Ferruh Yigit <ferruh.yigit@intel.com>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>

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

Patch
diff mbox series

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 73ebf37aae..d28211ba52 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -979,7 +979,7 @@  check_socket_id(const unsigned int socket_id)
 queueid_t
 get_allowed_max_nb_rxq(portid_t *pid)
 {
-	queueid_t allowed_max_rxq = MAX_QUEUE_ID;
+	queueid_t allowed_max_rxq = RTE_MAX_QUEUES_PER_PORT;
 	bool max_rxq_valid = false;
 	portid_t pi;
 	struct rte_eth_dev_info dev_info;
@@ -1029,7 +1029,7 @@  check_nb_rxq(queueid_t rxq)
 queueid_t
 get_allowed_max_nb_txq(portid_t *pid)
 {
-	queueid_t allowed_max_txq = MAX_QUEUE_ID;
+	queueid_t allowed_max_txq = RTE_MAX_QUEUES_PER_PORT;
 	bool max_txq_valid = false;
 	portid_t pi;
 	struct rte_eth_dev_info dev_info;
@@ -1079,7 +1079,7 @@  check_nb_txq(queueid_t txq)
 queueid_t
 get_allowed_max_nb_hairpinq(portid_t *pid)
 {
-	queueid_t allowed_max_hairpinq = MAX_QUEUE_ID;
+	queueid_t allowed_max_hairpinq = RTE_MAX_QUEUES_PER_PORT;
 	portid_t pi;
 	struct rte_eth_hairpin_cap cap;
 
diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
index 90694a3309..217d577018 100644
--- a/app/test-pmd/testpmd.h
+++ b/app/test-pmd/testpmd.h
@@ -58,8 +58,6 @@  typedef uint16_t portid_t;
 typedef uint16_t queueid_t;
 typedef uint16_t streamid_t;
 
-#define MAX_QUEUE_ID ((1 << (sizeof(queueid_t) * 8)) - 1)
-
 #if defined RTE_LIBRTE_PMD_SOFTNIC
 #define SOFTNIC			1
 #else
@@ -179,22 +177,22 @@  struct rte_port {
 	uint8_t                 need_reconfig_queues; /**< need reconfiguring queues or not */
 	uint8_t                 rss_flag;   /**< enable rss or not */
 	uint8_t                 dcb_flag;   /**< enable dcb */
-	uint16_t                nb_rx_desc[MAX_QUEUE_ID+1]; /**< per queue rx desc number */
-	uint16_t                nb_tx_desc[MAX_QUEUE_ID+1]; /**< per queue tx desc number */
-	struct rte_eth_rxconf   rx_conf[MAX_QUEUE_ID+1]; /**< per queue rx configuration */
-	struct rte_eth_txconf   tx_conf[MAX_QUEUE_ID+1]; /**< per queue tx configuration */
+	uint16_t                nb_rx_desc[RTE_MAX_QUEUES_PER_PORT+1]; /**< per queue rx desc number */
+	uint16_t                nb_tx_desc[RTE_MAX_QUEUES_PER_PORT+1]; /**< per queue tx desc number */
+	struct rte_eth_rxconf   rx_conf[RTE_MAX_QUEUES_PER_PORT+1]; /**< per queue rx configuration */
+	struct rte_eth_txconf   tx_conf[RTE_MAX_QUEUES_PER_PORT+1]; /**< per queue tx configuration */
 	struct rte_ether_addr   *mc_addr_pool; /**< pool of multicast addrs */
 	uint32_t                mc_addr_nb; /**< nb. of addr. in mc_addr_pool */
 	uint8_t                 slave_flag; /**< bonding slave port */
 	struct port_flow        *flow_list; /**< Associated flows. */
-	const struct rte_eth_rxtx_callback *rx_dump_cb[MAX_QUEUE_ID+1];
-	const struct rte_eth_rxtx_callback *tx_dump_cb[MAX_QUEUE_ID+1];
+	const struct rte_eth_rxtx_callback *rx_dump_cb[RTE_MAX_QUEUES_PER_PORT+1];
+	const struct rte_eth_rxtx_callback *tx_dump_cb[RTE_MAX_QUEUES_PER_PORT+1];
 #ifdef SOFTNIC
 	struct softnic_port     softport;  /**< softnic params */
 #endif
 	/**< metadata value to insert in Tx packets. */
 	uint32_t		tx_metadata;
-	const struct rte_eth_rxtx_callback *tx_set_md_cb[MAX_QUEUE_ID+1];
+	const struct rte_eth_rxtx_callback *tx_set_md_cb[RTE_MAX_QUEUES_PER_PORT+1];
 };
 
 /**
diff --git a/devtools/test-null.sh b/devtools/test-null.sh
index 9f9a459f76..6e5b1ad529 100755
--- a/devtools/test-null.sh
+++ b/devtools/test-null.sh
@@ -25,6 +25,6 @@  else
 fi
 
 (sleep 1 && echo stop) |
-$testpmd -c $coremask --no-huge -m 150 \
+$testpmd -c $coremask --no-huge -m 20 \
 	$libs --vdev net_null1 --vdev net_null2 $eal_options -- \
 	--no-mlockall --total-num-mbufs=2048 $testpmd_options -ia