[v5,1/1] app/testpmd: add valid check to verify multi mempool feature
Checks
Commit Message
Validate ethdev parameter 'max_rx_mempools' to know whether
device supports multi-mempool feature or not.
Also, add new testpmd command line argument, multi-mempool,
to control multi-mempool feature. By default its disabled.
Bugzilla ID: 1128
Fixes: 4f04edcda769 ("app/testpmd: support multiple mbuf pools per Rx queue")
Signed-off-by: Hanumanth Pothula <hpothula@marvell.com>
---
v5:
- Added testpmd argument to enable multi-mempool feature.
- Simplified logic to distinguish between multi-mempool,
multi-segment and single pool/segment.
v4:
- updated if condition.
v3:
- Simplified conditional check.
- Corrected spell, whether.
v2:
- Rebased on tip of next-net/main.
---
app/test-pmd/parameters.c | 3 ++
app/test-pmd/testpmd.c | 58 +++++++++++++++++++++++++--------------
app/test-pmd/testpmd.h | 1 +
3 files changed, 41 insertions(+), 21 deletions(-)
Comments
On 11/21/2022 12:45 PM, Hanumanth Pothula wrote:
> Validate ethdev parameter 'max_rx_mempools' to know whether
> device supports multi-mempool feature or not.
>
> Also, add new testpmd command line argument, multi-mempool,
> to control multi-mempool feature. By default its disabled.
>
> Bugzilla ID: 1128
> Fixes: 4f04edcda769 ("app/testpmd: support multiple mbuf pools per Rx queue")
>
> Signed-off-by: Hanumanth Pothula <hpothula@marvell.com>
>
> ---
> v5:
> - Added testpmd argument to enable multi-mempool feature.
> - Simplified logic to distinguish between multi-mempool,
> multi-segment and single pool/segment.
> v4:
> - updated if condition.
> v3:
> - Simplified conditional check.
> - Corrected spell, whether.
> v2:
> - Rebased on tip of next-net/main.
> ---
> app/test-pmd/parameters.c | 3 ++
> app/test-pmd/testpmd.c | 58 +++++++++++++++++++++++++--------------
> app/test-pmd/testpmd.h | 1 +
> 3 files changed, 41 insertions(+), 21 deletions(-)
>
> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> index aed4cdcb84..26d6450db4 100644
> --- a/app/test-pmd/parameters.c
> +++ b/app/test-pmd/parameters.c
> @@ -700,6 +700,7 @@ launch_args_parse(int argc, char** argv)
> { "rx-mq-mode", 1, 0, 0 },
> { "record-core-cycles", 0, 0, 0 },
> { "record-burst-stats", 0, 0, 0 },
> + { "multi-mempool", 0, 0, 0 },
Can you please group with relatet paramters, instead of appending end,
after 'rxpkts' related parameters group (so after 'txpkts') can be good
location since it is used for buffer split.
need to document new argument on 'doc/guides/testpmd_app_ug/run_app.rst'
Also need to add help string in 'usage()' function, again grouped in
related paramters.
> { PARAM_NUM_PROCS, 1, 0, 0 },
> { PARAM_PROC_ID, 1, 0, 0 },
> { 0, 0, 0, 0 },
> @@ -1449,6 +1450,8 @@ launch_args_parse(int argc, char** argv)
> record_core_cycles = 1;
> if (!strcmp(lgopts[opt_idx].name, "record-burst-stats"))
> record_burst_stats = 1;
> + if (!strcmp(lgopts[opt_idx].name, "multi-mempool"))
> + multi_mempool = 1;
Can you group with related paramters, same as above mentioned location?
> if (!strcmp(lgopts[opt_idx].name, PARAM_NUM_PROCS))
> num_procs = atoi(optarg);
> if (!strcmp(lgopts[opt_idx].name, PARAM_PROC_ID))
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 4e25f77c6a..9dfc4c9d0e 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -497,6 +497,11 @@ uint8_t record_burst_stats;
> */
> uint32_t rxq_share;
>
> +/*
> + * Multi-mempool support, disabled by default.
> + */
> +uint8_t multi_mempool;
Can you put this after 'rx_pkt_nb_segs' related group.
> +
> unsigned int num_sockets = 0;
> unsigned int socket_ids[RTE_MAX_NUMA_NODES];
>
> @@ -2655,28 +2660,23 @@ rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
> union rte_eth_rxseg rx_useg[MAX_SEGS_BUFFER_SPLIT] = {};
> struct rte_mempool *rx_mempool[MAX_MEMPOOL] = {};
> struct rte_mempool *mpx;
> + struct rte_eth_dev_info dev_info;
> unsigned int i, mp_n;
> uint32_t prev_hdrs = 0;
> int ret;
>
> + ret = rte_eth_dev_info_get(port_id, &dev_info);
> + if (ret != 0)
> + return ret;
> +
> /* Verify Rx queue configuration is single pool and segment or
> * multiple pool/segment.
> + * @see rte_eth_dev_info::max_rx_mempools
> * @see rte_eth_rxconf::rx_mempools
> * @see rte_eth_rxconf::rx_seg
> */
Is above comment block still valid?
> - if (!(mbuf_data_size_n > 1) && !(rx_pkt_nb_segs > 1 ||
> - ((rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT) != 0))) {
> - /* Single pool/segment configuration */
> - rx_conf->rx_seg = NULL;
> - rx_conf->rx_nseg = 0;
> - ret = rte_eth_rx_queue_setup(port_id, rx_queue_id,
> - nb_rx_desc, socket_id,
> - rx_conf, mp);
> - goto exit;
> - }
> -
> - if (rx_pkt_nb_segs > 1 ||
> - rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT) {
> + if ((rx_pkt_nb_segs > 1) &&
> + (rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT)) {
> /* multi-segment configuration */
> for (i = 0; i < rx_pkt_nb_segs; i++) {
> struct rte_eth_rxseg_split *rx_seg = &rx_useg[i].split;
> @@ -2701,7 +2701,14 @@ rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
> }
> rx_conf->rx_nseg = rx_pkt_nb_segs;
> rx_conf->rx_seg = rx_useg;
> - } else {
> + rx_conf->rx_mempools = NULL;
> + rx_conf->rx_nmempool = 0;
> + ret = rte_eth_rx_queue_setup(port_id, rx_queue_id, nb_rx_desc,
> + socket_id, rx_conf, NULL);
> + rx_conf->rx_seg = NULL;
> + rx_conf->rx_nseg = 0;
> + } else if ((multi_mempool == 1) && (dev_info.max_rx_mempools != 0) &&
> + (mbuf_data_size_n > 1)) {
What do you think to move 'rte_eth_dev_info_get()' within this if block,
and reduce 'dev_info' scope, like
else if (multi_mempool == 1)
if (mbuf_data_size_n <= 1))
log(describe problem)
return
struct rte_eth_dev_info dev_info;
ret = rte_eth_dev_info_get(port_id, &dev_info);
if (dev_info.max_rx_mempools == 0)
log("device doesn't support requested config"
return
<multi-pool configuration>
else
> /* multi-pool configuration */
> for (i = 0; i < mbuf_data_size_n; i++) {
> mpx = mbuf_pool_find(socket_id, i);
Where the mempools are created? Is that code also needs to be updated to
use/check 'multi_mempool' variable/config?
> @@ -2709,14 +2716,23 @@ rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
> }
> rx_conf->rx_mempools = rx_mempool;
> rx_conf->rx_nmempool = mbuf_data_size_n;
> - }
> - ret = rte_eth_rx_queue_setup(port_id, rx_queue_id, nb_rx_desc,
> + rx_conf->rx_seg = NULL;
> + rx_conf->rx_nseg = 0;
> + ret = rte_eth_rx_queue_setup(port_id, rx_queue_id, nb_rx_desc,
> socket_id, rx_conf, NULL);
> - rx_conf->rx_seg = NULL;
> - rx_conf->rx_nseg = 0;
> - rx_conf->rx_mempools = NULL;
> - rx_conf->rx_nmempool = 0;
> -exit:
> + rx_conf->rx_mempools = NULL;
> + rx_conf->rx_nmempool = 0;
> + } else {
> + /* Single pool/segment configuration */
> + rx_conf->rx_seg = NULL;
> + rx_conf->rx_nseg = 0;
> + rx_conf->rx_mempools = NULL;
> + rx_conf->rx_nmempool = 0;
> + ret = rte_eth_rx_queue_setup(port_id, rx_queue_id, nb_rx_desc,
> + socket_id, rx_conf, mp);
> + }
> +
> +
> ports[port_id].rxq[rx_queue_id].state = rx_conf->rx_deferred_start ?
> RTE_ETH_QUEUE_STATE_STOPPED :
> RTE_ETH_QUEUE_STATE_STARTED;
> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h
> index aaf69c349a..9472a2cb19 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -464,6 +464,7 @@ enum dcb_mode_enable
> extern uint8_t xstats_hide_zero; /**< Hide zero values for xstats display */
>
> /* globals used for configuration */
> +extern uint8_t multi_mempool; /**< Enables multi-mempool feature. */
Again please group this same location as done in .c file
> extern uint8_t record_core_cycles; /**< Enables measurement of CPU cycles */
> extern uint8_t record_burst_stats; /**< Enables display of RX and TX bursts */
> extern uint16_t verbose_level; /**< Drives messages being displayed, if any. */
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Monday, November 21, 2022 6:53 PM
> To: Hanumanth Reddy Pothula <hpothula@marvell.com>; Aman Singh
> <aman.deep.singh@intel.com>; Yuying Zhang <yuying.zhang@intel.com>
> Cc: dev@dpdk.org; andrew.rybchenko@oktetlabs.ru;
> thomas@monjalon.net; yux.jiang@intel.com; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Nithin Kumar Dabilpuram
> <ndabilpuram@marvell.com>
> Subject: [EXT] Re: [PATCH v5 1/1] app/testpmd: add valid check to verify
> multi mempool feature
>
> External Email
>
> ----------------------------------------------------------------------
> On 11/21/2022 12:45 PM, Hanumanth Pothula wrote:
> > Validate ethdev parameter 'max_rx_mempools' to know whether device
> > supports multi-mempool feature or not.
> >
> > Also, add new testpmd command line argument, multi-mempool, to
> control
> > multi-mempool feature. By default its disabled.
> >
> > Bugzilla ID: 1128
> > Fixes: 4f04edcda769 ("app/testpmd: support multiple mbuf pools per Rx
> > queue")
> >
> > Signed-off-by: Hanumanth Pothula <hpothula@marvell.com>
> >
> > ---
> > v5:
> > - Added testpmd argument to enable multi-mempool feature.
> > - Simplified logic to distinguish between multi-mempool,
> > multi-segment and single pool/segment.
> > v4:
> > - updated if condition.
> > v3:
> > - Simplified conditional check.
> > - Corrected spell, whether.
> > v2:
> > - Rebased on tip of next-net/main.
> > ---
> > app/test-pmd/parameters.c | 3 ++
> > app/test-pmd/testpmd.c | 58 +++++++++++++++++++++++++------------
> --
> > app/test-pmd/testpmd.h | 1 +
> > 3 files changed, 41 insertions(+), 21 deletions(-)
> >
> > diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> > index aed4cdcb84..26d6450db4 100644
> > --- a/app/test-pmd/parameters.c
> > +++ b/app/test-pmd/parameters.c
> > @@ -700,6 +700,7 @@ launch_args_parse(int argc, char** argv)
> > { "rx-mq-mode", 1, 0, 0 },
> > { "record-core-cycles", 0, 0, 0 },
> > { "record-burst-stats", 0, 0, 0 },
> > + { "multi-mempool", 0, 0, 0 },
>
> Can you please group with relatet paramters, instead of appending end,
> after 'rxpkts' related parameters group (so after 'txpkts') can be good
> location since it is used for buffer split.
>
Ack
> need to document new argument on
> 'doc/guides/testpmd_app_ug/run_app.rst'
>
Ack
> Also need to add help string in 'usage()' function, again grouped in related
> paramters.
Sure, will add help string
>
> > { PARAM_NUM_PROCS, 1, 0, 0 },
> > { PARAM_PROC_ID, 1, 0, 0 },
> > { 0, 0, 0, 0 },
> > @@ -1449,6 +1450,8 @@ launch_args_parse(int argc, char** argv)
> > record_core_cycles = 1;
> > if (!strcmp(lgopts[opt_idx].name, "record-burst-
> stats"))
> > record_burst_stats = 1;
> > + if (!strcmp(lgopts[opt_idx].name, "multi-
> mempool"))
> > + multi_mempool = 1;
>
> Can you group with related paramters, same as above mentioned location?
>
Ack
> > if (!strcmp(lgopts[opt_idx].name,
> PARAM_NUM_PROCS))
> > num_procs = atoi(optarg);
> > if (!strcmp(lgopts[opt_idx].name,
> PARAM_PROC_ID)) diff --git
> > a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > 4e25f77c6a..9dfc4c9d0e 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -497,6 +497,11 @@ uint8_t record_burst_stats;
> > */
> > uint32_t rxq_share;
> >
> > +/*
> > + * Multi-mempool support, disabled by default.
> > + */
> > +uint8_t multi_mempool;
>
> Can you put this after 'rx_pkt_nb_segs' related group.
>
Ack
> > +
> > unsigned int num_sockets = 0;
> > unsigned int socket_ids[RTE_MAX_NUMA_NODES];
> >
> > @@ -2655,28 +2660,23 @@ rx_queue_setup(uint16_t port_id, uint16_t
> rx_queue_id,
> > union rte_eth_rxseg rx_useg[MAX_SEGS_BUFFER_SPLIT] = {};
> > struct rte_mempool *rx_mempool[MAX_MEMPOOL] = {};
> > struct rte_mempool *mpx;
> > + struct rte_eth_dev_info dev_info;
> > unsigned int i, mp_n;
> > uint32_t prev_hdrs = 0;
> > int ret;
> >
> > + ret = rte_eth_dev_info_get(port_id, &dev_info);
> > + if (ret != 0)
> > + return ret;
> > +
> > /* Verify Rx queue configuration is single pool and segment or
> > * multiple pool/segment.
> > + * @see rte_eth_dev_info::max_rx_mempools
> > * @see rte_eth_rxconf::rx_mempools
> > * @see rte_eth_rxconf::rx_seg
> > */
>
> Is above comment block still valid?
Will remove
>
> > - if (!(mbuf_data_size_n > 1) && !(rx_pkt_nb_segs > 1 ||
> > - ((rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT) !=
> 0))) {
> > - /* Single pool/segment configuration */
> > - rx_conf->rx_seg = NULL;
> > - rx_conf->rx_nseg = 0;
> > - ret = rte_eth_rx_queue_setup(port_id, rx_queue_id,
> > - nb_rx_desc, socket_id,
> > - rx_conf, mp);
> > - goto exit;
> > - }
> > -
> > - if (rx_pkt_nb_segs > 1 ||
> > - rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT) {
> > + if ((rx_pkt_nb_segs > 1) &&
> > + (rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT)) {
> > /* multi-segment configuration */
> > for (i = 0; i < rx_pkt_nb_segs; i++) {
> > struct rte_eth_rxseg_split *rx_seg =
> &rx_useg[i].split; @@ -2701,7
> > +2701,14 @@ rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
> > }
> > rx_conf->rx_nseg = rx_pkt_nb_segs;
> > rx_conf->rx_seg = rx_useg;
> > - } else {
> > + rx_conf->rx_mempools = NULL;
> > + rx_conf->rx_nmempool = 0;
> > + ret = rte_eth_rx_queue_setup(port_id, rx_queue_id,
> nb_rx_desc,
> > + socket_id, rx_conf, NULL);
> > + rx_conf->rx_seg = NULL;
> > + rx_conf->rx_nseg = 0;
> > + } else if ((multi_mempool == 1) && (dev_info.max_rx_mempools !=
> 0) &&
> > + (mbuf_data_size_n > 1)) {
>
> What do you think to move 'rte_eth_dev_info_get()' within this if block,
> and reduce 'dev_info' scope, like
Ack
>
> else if (multi_mempool == 1)
> if (mbuf_data_size_n <= 1))
> log(describe problem)
> return
> struct rte_eth_dev_info dev_info;
> ret = rte_eth_dev_info_get(port_id, &dev_info);
> if (dev_info.max_rx_mempools == 0)
> log("device doesn't support requested config"
> return
> <multi-pool configuration>
> else
>
> > /* multi-pool configuration */
> > for (i = 0; i < mbuf_data_size_n; i++) {
> > mpx = mbuf_pool_find(socket_id, i);
>
> Where the mempools are created? Is that code also needs to be updated to
> use/check 'multi_mempool' variable/config?
I think it's not required, as user might create multiple pools for other scenarios as well, for example as part of buzilla id: 1128, user creating two pools but not for multi-mempool feature.
./x86_64-native-linuxapp-gcc/app/dpdk-testpmd -l 5,6 -n 8 --force-max-simd-bitwidth=64 -- -i --portmask=0x3 --rxq=1 --txq=1 --txd=1024 --rxd=1024 --nb-cores=1 --mbuf-size=2048,2048
>
> > @@ -2709,14 +2716,23 @@ rx_queue_setup(uint16_t port_id, uint16_t
> rx_queue_id,
> > }
> > rx_conf->rx_mempools = rx_mempool;
> > rx_conf->rx_nmempool = mbuf_data_size_n;
> > - }
> > - ret = rte_eth_rx_queue_setup(port_id, rx_queue_id, nb_rx_desc,
> > + rx_conf->rx_seg = NULL;
> > + rx_conf->rx_nseg = 0;
> > + ret = rte_eth_rx_queue_setup(port_id, rx_queue_id,
> nb_rx_desc,
> > socket_id, rx_conf, NULL);
> > - rx_conf->rx_seg = NULL;
> > - rx_conf->rx_nseg = 0;
> > - rx_conf->rx_mempools = NULL;
> > - rx_conf->rx_nmempool = 0;
> > -exit:
> > + rx_conf->rx_mempools = NULL;
> > + rx_conf->rx_nmempool = 0;
> > + } else {
> > + /* Single pool/segment configuration */
> > + rx_conf->rx_seg = NULL;
> > + rx_conf->rx_nseg = 0;
> > + rx_conf->rx_mempools = NULL;
> > + rx_conf->rx_nmempool = 0;
> > + ret = rte_eth_rx_queue_setup(port_id, rx_queue_id,
> nb_rx_desc,
> > + socket_id, rx_conf, mp);
> > + }
> > +
> > +
> > ports[port_id].rxq[rx_queue_id].state = rx_conf->rx_deferred_start
> ?
> >
> RTE_ETH_QUEUE_STATE_STOPPED :
> >
> RTE_ETH_QUEUE_STATE_STARTED;
> > diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index
> > aaf69c349a..9472a2cb19 100644
> > --- a/app/test-pmd/testpmd.h
> > +++ b/app/test-pmd/testpmd.h
> > @@ -464,6 +464,7 @@ enum dcb_mode_enable extern uint8_t
> > xstats_hide_zero; /**< Hide zero values for xstats display */
> >
> > /* globals used for configuration */
> > +extern uint8_t multi_mempool; /**< Enables multi-mempool feature.
> */
>
> Again please group this same location as done in .c file
Ack.
>
> > extern uint8_t record_core_cycles; /**< Enables measurement of CPU
> > cycles */ extern uint8_t record_burst_stats; /**< Enables display of
> > RX and TX bursts */ extern uint16_t verbose_level; /**< Drives messages
> being displayed, if any. */
On 11/21/2022 1:36 PM, Hanumanth Reddy Pothula wrote:
>
>
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>> Sent: Monday, November 21, 2022 6:53 PM
>> To: Hanumanth Reddy Pothula <hpothula@marvell.com>; Aman Singh
>> <aman.deep.singh@intel.com>; Yuying Zhang <yuying.zhang@intel.com>
>> Cc: dev@dpdk.org; andrew.rybchenko@oktetlabs.ru;
>> thomas@monjalon.net; yux.jiang@intel.com; Jerin Jacob Kollanukkaran
>> <jerinj@marvell.com>; Nithin Kumar Dabilpuram
>> <ndabilpuram@marvell.com>
>> Subject: [EXT] Re: [PATCH v5 1/1] app/testpmd: add valid check to verify
>> multi mempool feature
>>
>> External Email
>>
>> ----------------------------------------------------------------------
>> On 11/21/2022 12:45 PM, Hanumanth Pothula wrote:
>>> Validate ethdev parameter 'max_rx_mempools' to know whether device
>>> supports multi-mempool feature or not.
>>>
>>> Also, add new testpmd command line argument, multi-mempool, to
>> control
>>> multi-mempool feature. By default its disabled.
>>>
>>> Bugzilla ID: 1128
>>> Fixes: 4f04edcda769 ("app/testpmd: support multiple mbuf pools per Rx
>>> queue")
>>>
>>> Signed-off-by: Hanumanth Pothula <hpothula@marvell.com>
>>>
>>> ---
>>> v5:
>>> - Added testpmd argument to enable multi-mempool feature.
>>> - Simplified logic to distinguish between multi-mempool,
>>> multi-segment and single pool/segment.
>>> v4:
>>> - updated if condition.
>>> v3:
>>> - Simplified conditional check.
>>> - Corrected spell, whether.
>>> v2:
>>> - Rebased on tip of next-net/main.
>>> ---
>>> app/test-pmd/parameters.c | 3 ++
>>> app/test-pmd/testpmd.c | 58 +++++++++++++++++++++++++------------
>> --
>>> app/test-pmd/testpmd.h | 1 +
>>> 3 files changed, 41 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
>>> index aed4cdcb84..26d6450db4 100644
>>> --- a/app/test-pmd/parameters.c
>>> +++ b/app/test-pmd/parameters.c
>>> @@ -700,6 +700,7 @@ launch_args_parse(int argc, char** argv)
>>> { "rx-mq-mode", 1, 0, 0 },
>>> { "record-core-cycles", 0, 0, 0 },
>>> { "record-burst-stats", 0, 0, 0 },
>>> + { "multi-mempool", 0, 0, 0 },
>>
>> Can you please group with relatet paramters, instead of appending end,
>> after 'rxpkts' related parameters group (so after 'txpkts') can be good
>> location since it is used for buffer split.
>>
> Ack
>
>> need to document new argument on
>> 'doc/guides/testpmd_app_ug/run_app.rst'
>>
> Ack
>
>> Also need to add help string in 'usage()' function, again grouped in related
>> paramters.
> Sure, will add help string
>>
>>> { PARAM_NUM_PROCS, 1, 0, 0 },
>>> { PARAM_PROC_ID, 1, 0, 0 },
>>> { 0, 0, 0, 0 },
>>> @@ -1449,6 +1450,8 @@ launch_args_parse(int argc, char** argv)
>>> record_core_cycles = 1;
>>> if (!strcmp(lgopts[opt_idx].name, "record-burst-
>> stats"))
>>> record_burst_stats = 1;
>>> + if (!strcmp(lgopts[opt_idx].name, "multi-
>> mempool"))
>>> + multi_mempool = 1;
>>
>> Can you group with related paramters, same as above mentioned location?
>>
> Ack
>>> if (!strcmp(lgopts[opt_idx].name,
>> PARAM_NUM_PROCS))
>>> num_procs = atoi(optarg);
>>> if (!strcmp(lgopts[opt_idx].name,
>> PARAM_PROC_ID)) diff --git
>>> a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>> 4e25f77c6a..9dfc4c9d0e 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -497,6 +497,11 @@ uint8_t record_burst_stats;
>>> */
>>> uint32_t rxq_share;
>>>
>>> +/*
>>> + * Multi-mempool support, disabled by default.
>>> + */
>>> +uint8_t multi_mempool;
>>
>> Can you put this after 'rx_pkt_nb_segs' related group.
>>
> Ack
>>> +
>>> unsigned int num_sockets = 0;
>>> unsigned int socket_ids[RTE_MAX_NUMA_NODES];
>>>
>>> @@ -2655,28 +2660,23 @@ rx_queue_setup(uint16_t port_id, uint16_t
>> rx_queue_id,
>>> union rte_eth_rxseg rx_useg[MAX_SEGS_BUFFER_SPLIT] = {};
>>> struct rte_mempool *rx_mempool[MAX_MEMPOOL] = {};
>>> struct rte_mempool *mpx;
>>> + struct rte_eth_dev_info dev_info;
>>> unsigned int i, mp_n;
>>> uint32_t prev_hdrs = 0;
>>> int ret;
>>>
>>> + ret = rte_eth_dev_info_get(port_id, &dev_info);
>>> + if (ret != 0)
>>> + return ret;
>>> +
>>> /* Verify Rx queue configuration is single pool and segment or
>>> * multiple pool/segment.
>>> + * @see rte_eth_dev_info::max_rx_mempools
>>> * @see rte_eth_rxconf::rx_mempools
>>> * @see rte_eth_rxconf::rx_seg
>>> */
>>
>> Is above comment block still valid?
> Will remove
>>
>>> - if (!(mbuf_data_size_n > 1) && !(rx_pkt_nb_segs > 1 ||
>>> - ((rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT) !=
>> 0))) {
>>> - /* Single pool/segment configuration */
>>> - rx_conf->rx_seg = NULL;
>>> - rx_conf->rx_nseg = 0;
>>> - ret = rte_eth_rx_queue_setup(port_id, rx_queue_id,
>>> - nb_rx_desc, socket_id,
>>> - rx_conf, mp);
>>> - goto exit;
>>> - }
>>> -
>>> - if (rx_pkt_nb_segs > 1 ||
>>> - rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT) {
>>> + if ((rx_pkt_nb_segs > 1) &&
>>> + (rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT)) {
>>> /* multi-segment configuration */
>>> for (i = 0; i < rx_pkt_nb_segs; i++) {
>>> struct rte_eth_rxseg_split *rx_seg =
>> &rx_useg[i].split; @@ -2701,7
>>> +2701,14 @@ rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
>>> }
>>> rx_conf->rx_nseg = rx_pkt_nb_segs;
>>> rx_conf->rx_seg = rx_useg;
>>> - } else {
>>> + rx_conf->rx_mempools = NULL;
>>> + rx_conf->rx_nmempool = 0;
>>> + ret = rte_eth_rx_queue_setup(port_id, rx_queue_id,
>> nb_rx_desc,
>>> + socket_id, rx_conf, NULL);
>>> + rx_conf->rx_seg = NULL;
>>> + rx_conf->rx_nseg = 0;
>>> + } else if ((multi_mempool == 1) && (dev_info.max_rx_mempools !=
>> 0) &&
>>> + (mbuf_data_size_n > 1)) {
>>
>> What do you think to move 'rte_eth_dev_info_get()' within this if block,
>> and reduce 'dev_info' scope, like
> Ack
>>
>> else if (multi_mempool == 1)
>> if (mbuf_data_size_n <= 1))
>> log(describe problem)
>> return
>> struct rte_eth_dev_info dev_info;
>> ret = rte_eth_dev_info_get(port_id, &dev_info);
>> if (dev_info.max_rx_mempools == 0)
>> log("device doesn't support requested config"
>> return
>> <multi-pool configuration>
>> else
>>
>>> /* multi-pool configuration */
>>> for (i = 0; i < mbuf_data_size_n; i++) {
>>> mpx = mbuf_pool_find(socket_id, i);
>>
>> Where the mempools are created? Is that code also needs to be updated to
>> use/check 'multi_mempool' variable/config?
> I think it's not required, as user might create multiple pools for other scenarios as well, for example as part of buzilla id: 1128, user creating two pools but not for multi-mempool feature.
> ./x86_64-native-linuxapp-gcc/app/dpdk-testpmd -l 5,6 -n 8 --force-max-simd-bitwidth=64 -- -i --portmask=0x3 --rxq=1 --txq=1 --txd=1024 --rxd=1024 --nb-cores=1 --mbuf-size=2048,2048
If they are not created explicit for multiple pool, agree to not change
that code, thanks.
>>
>>> @@ -2709,14 +2716,23 @@ rx_queue_setup(uint16_t port_id, uint16_t
>> rx_queue_id,
>>> }
>>> rx_conf->rx_mempools = rx_mempool;
>>> rx_conf->rx_nmempool = mbuf_data_size_n;
>>> - }
>>> - ret = rte_eth_rx_queue_setup(port_id, rx_queue_id, nb_rx_desc,
>>> + rx_conf->rx_seg = NULL;
>>> + rx_conf->rx_nseg = 0;
>>> + ret = rte_eth_rx_queue_setup(port_id, rx_queue_id,
>> nb_rx_desc,
>>> socket_id, rx_conf, NULL);
>>> - rx_conf->rx_seg = NULL;
>>> - rx_conf->rx_nseg = 0;
>>> - rx_conf->rx_mempools = NULL;
>>> - rx_conf->rx_nmempool = 0;
>>> -exit:
>>> + rx_conf->rx_mempools = NULL;
>>> + rx_conf->rx_nmempool = 0;
>>> + } else {
>>> + /* Single pool/segment configuration */
>>> + rx_conf->rx_seg = NULL;
>>> + rx_conf->rx_nseg = 0;
>>> + rx_conf->rx_mempools = NULL;
>>> + rx_conf->rx_nmempool = 0;
>>> + ret = rte_eth_rx_queue_setup(port_id, rx_queue_id,
>> nb_rx_desc,
>>> + socket_id, rx_conf, mp);
>>> + }
>>> +
>>> +
>>> ports[port_id].rxq[rx_queue_id].state = rx_conf->rx_deferred_start
>> ?
>>>
>> RTE_ETH_QUEUE_STATE_STOPPED :
>>>
>> RTE_ETH_QUEUE_STATE_STARTED;
>>> diff --git a/app/test-pmd/testpmd.h b/app/test-pmd/testpmd.h index
>>> aaf69c349a..9472a2cb19 100644
>>> --- a/app/test-pmd/testpmd.h
>>> +++ b/app/test-pmd/testpmd.h
>>> @@ -464,6 +464,7 @@ enum dcb_mode_enable extern uint8_t
>>> xstats_hide_zero; /**< Hide zero values for xstats display */
>>>
>>> /* globals used for configuration */
>>> +extern uint8_t multi_mempool; /**< Enables multi-mempool feature.
>> */
>>
>> Again please group this same location as done in .c file
> Ack.
>>
>>> extern uint8_t record_core_cycles; /**< Enables measurement of CPU
>>> cycles */ extern uint8_t record_burst_stats; /**< Enables display of
>>> RX and TX bursts */ extern uint16_t verbose_level; /**< Drives messages
>> being displayed, if any. */
>
@@ -700,6 +700,7 @@ launch_args_parse(int argc, char** argv)
{ "rx-mq-mode", 1, 0, 0 },
{ "record-core-cycles", 0, 0, 0 },
{ "record-burst-stats", 0, 0, 0 },
+ { "multi-mempool", 0, 0, 0 },
{ PARAM_NUM_PROCS, 1, 0, 0 },
{ PARAM_PROC_ID, 1, 0, 0 },
{ 0, 0, 0, 0 },
@@ -1449,6 +1450,8 @@ launch_args_parse(int argc, char** argv)
record_core_cycles = 1;
if (!strcmp(lgopts[opt_idx].name, "record-burst-stats"))
record_burst_stats = 1;
+ if (!strcmp(lgopts[opt_idx].name, "multi-mempool"))
+ multi_mempool = 1;
if (!strcmp(lgopts[opt_idx].name, PARAM_NUM_PROCS))
num_procs = atoi(optarg);
if (!strcmp(lgopts[opt_idx].name, PARAM_PROC_ID))
@@ -497,6 +497,11 @@ uint8_t record_burst_stats;
*/
uint32_t rxq_share;
+/*
+ * Multi-mempool support, disabled by default.
+ */
+uint8_t multi_mempool;
+
unsigned int num_sockets = 0;
unsigned int socket_ids[RTE_MAX_NUMA_NODES];
@@ -2655,28 +2660,23 @@ rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
union rte_eth_rxseg rx_useg[MAX_SEGS_BUFFER_SPLIT] = {};
struct rte_mempool *rx_mempool[MAX_MEMPOOL] = {};
struct rte_mempool *mpx;
+ struct rte_eth_dev_info dev_info;
unsigned int i, mp_n;
uint32_t prev_hdrs = 0;
int ret;
+ ret = rte_eth_dev_info_get(port_id, &dev_info);
+ if (ret != 0)
+ return ret;
+
/* Verify Rx queue configuration is single pool and segment or
* multiple pool/segment.
+ * @see rte_eth_dev_info::max_rx_mempools
* @see rte_eth_rxconf::rx_mempools
* @see rte_eth_rxconf::rx_seg
*/
- if (!(mbuf_data_size_n > 1) && !(rx_pkt_nb_segs > 1 ||
- ((rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT) != 0))) {
- /* Single pool/segment configuration */
- rx_conf->rx_seg = NULL;
- rx_conf->rx_nseg = 0;
- ret = rte_eth_rx_queue_setup(port_id, rx_queue_id,
- nb_rx_desc, socket_id,
- rx_conf, mp);
- goto exit;
- }
-
- if (rx_pkt_nb_segs > 1 ||
- rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT) {
+ if ((rx_pkt_nb_segs > 1) &&
+ (rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT)) {
/* multi-segment configuration */
for (i = 0; i < rx_pkt_nb_segs; i++) {
struct rte_eth_rxseg_split *rx_seg = &rx_useg[i].split;
@@ -2701,7 +2701,14 @@ rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
}
rx_conf->rx_nseg = rx_pkt_nb_segs;
rx_conf->rx_seg = rx_useg;
- } else {
+ rx_conf->rx_mempools = NULL;
+ rx_conf->rx_nmempool = 0;
+ ret = rte_eth_rx_queue_setup(port_id, rx_queue_id, nb_rx_desc,
+ socket_id, rx_conf, NULL);
+ rx_conf->rx_seg = NULL;
+ rx_conf->rx_nseg = 0;
+ } else if ((multi_mempool == 1) && (dev_info.max_rx_mempools != 0) &&
+ (mbuf_data_size_n > 1)) {
/* multi-pool configuration */
for (i = 0; i < mbuf_data_size_n; i++) {
mpx = mbuf_pool_find(socket_id, i);
@@ -2709,14 +2716,23 @@ rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
}
rx_conf->rx_mempools = rx_mempool;
rx_conf->rx_nmempool = mbuf_data_size_n;
- }
- ret = rte_eth_rx_queue_setup(port_id, rx_queue_id, nb_rx_desc,
+ rx_conf->rx_seg = NULL;
+ rx_conf->rx_nseg = 0;
+ ret = rte_eth_rx_queue_setup(port_id, rx_queue_id, nb_rx_desc,
socket_id, rx_conf, NULL);
- rx_conf->rx_seg = NULL;
- rx_conf->rx_nseg = 0;
- rx_conf->rx_mempools = NULL;
- rx_conf->rx_nmempool = 0;
-exit:
+ rx_conf->rx_mempools = NULL;
+ rx_conf->rx_nmempool = 0;
+ } else {
+ /* Single pool/segment configuration */
+ rx_conf->rx_seg = NULL;
+ rx_conf->rx_nseg = 0;
+ rx_conf->rx_mempools = NULL;
+ rx_conf->rx_nmempool = 0;
+ ret = rte_eth_rx_queue_setup(port_id, rx_queue_id, nb_rx_desc,
+ socket_id, rx_conf, mp);
+ }
+
+
ports[port_id].rxq[rx_queue_id].state = rx_conf->rx_deferred_start ?
RTE_ETH_QUEUE_STATE_STOPPED :
RTE_ETH_QUEUE_STATE_STARTED;
@@ -464,6 +464,7 @@ enum dcb_mode_enable
extern uint8_t xstats_hide_zero; /**< Hide zero values for xstats display */
/* globals used for configuration */
+extern uint8_t multi_mempool; /**< Enables multi-mempool feature. */
extern uint8_t record_core_cycles; /**< Enables measurement of CPU cycles */
extern uint8_t record_burst_stats; /**< Enables display of RX and TX bursts */
extern uint16_t verbose_level; /**< Drives messages being displayed, if any. */