From: Ferruh Yigit <ferruh.yigit@amd.com>
To: Hanumanth 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, jerinj@marvell.com, ndabilpuram@marvell.com
Subject: Re: [PATCH v6 1/1] app/testpmd: add valid check to verify multi mempool feature
Date: Mon, 21 Nov 2022 17:31:43 +0000 [thread overview]
Message-ID: <6696ea9a-442b-a10b-0ce9-dee07d5bacb2@amd.com> (raw)
In-Reply-To: <20221121143347.3923255-1-hpothula@marvell.com>
On 11/21/2022 2:33 PM, Hanumanth Pothula wrote:
> Validate ethdev parameter 'max_rx_mempools' to know whether
> device supports multi-mempool feature or not.
>
Validation 'max_rx_mempools' is not main purpose of this patch, I would
move below paragraph up.
> 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>
>
> ---
> v6:
> - Updated run_app.rst file with multi-mempool argument.
> - defined and populated multi_mempool at related arguments.
> - invoking rte_eth_dev_info_get() withing multi-mempool condition
> 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 | 4 ++
> app/test-pmd/testpmd.c | 66 +++++++++++++++++----------
> app/test-pmd/testpmd.h | 1 +
> doc/guides/testpmd_app_ug/run_app.rst | 4 ++
> 4 files changed, 50 insertions(+), 25 deletions(-)
>
> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> index aed4cdcb84..d0f7b2f11d 100644
> --- a/app/test-pmd/parameters.c
> +++ b/app/test-pmd/parameters.c
> @@ -155,6 +155,7 @@ usage(char* progname)
> printf(" --rxhdrs=eth[,ipv4]*: set RX segment protocol to split.\n");
> printf(" --txpkts=X[,Y]*: set TX segment sizes"
> " or total packet length.\n");
> + printf(" --multi-mempool: enable multi-mempool support\n");
Indentation is wrong, one space is missing.
Can you also update the '--mbuf-size=' definition, it has:
" ... extra memory pools will be created for allocating mbufs to receive
packets with buffer splitting features.",
Now it is for both "buffer splitting and multi Rx mempool features."
Even it can be possible to reference to new argument.
> printf(" --txonly-multi-flow: generate multiple flows in txonly mode\n");
> printf(" --tx-ip=src,dst: IP addresses in Tx-only mode\n");
> printf(" --tx-udp=src[,dst]: UDP ports in Tx-only mode\n");
> @@ -669,6 +670,7 @@ launch_args_parse(int argc, char** argv)
> { "rxpkts", 1, 0, 0 },
> { "rxhdrs", 1, 0, 0 },
> { "txpkts", 1, 0, 0 },
> + { "multi-mempool", 0, 0, 0 },
Thinking twice, I am not sure about the 'multi-mempool' name, because
'mbuf-size' already cause to create multiple mempool, 'multi-mempool'
can be confusing.
As ethdev variable name is 'max_rx_mempools', what do you think to use
'multi-rx-mempools' here as argument?
> { "txonly-multi-flow", 0, 0, 0 },
> { "rxq-share", 2, 0, 0 },
> { "eth-link-speed", 1, 0, 0 },
> @@ -1295,6 +1297,8 @@ launch_args_parse(int argc, char** argv)
> else
> rte_exit(EXIT_FAILURE, "bad txpkts\n");
> }
> + if (!strcmp(lgopts[opt_idx].name, "multi-mempool"))
> + multi_mempool = 1;
> if (!strcmp(lgopts[opt_idx].name, "txonly-multi-flow"))
> txonly_multi_flow = 1;
> if (!strcmp(lgopts[opt_idx].name, "rxq-share")) {
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 4e25f77c6a..0bf2e4bd0d 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -245,6 +245,7 @@ uint32_t max_rx_pkt_len;
> */
> uint16_t rx_pkt_seg_lengths[MAX_SEGS_BUFFER_SPLIT];
> uint8_t rx_pkt_nb_segs; /**< Number of segments to split */
> +uint8_t multi_mempool; /**< Enables multi-mempool feature */
> uint16_t rx_pkt_seg_offsets[MAX_SEGS_BUFFER_SPLIT];
> uint8_t rx_pkt_nb_offs; /**< Number of specified offsets */
> uint32_t rx_pkt_hdr_protos[MAX_SEGS_BUFFER_SPLIT];
> @@ -258,6 +259,8 @@ uint16_t tx_pkt_seg_lengths[RTE_MAX_SEGS_PER_PKT] = {
> };
> uint8_t tx_pkt_nb_segs = 1; /**< Number of segments in TXONLY packets */
>
> +
> +
Unintendend change.
> enum tx_pkt_split tx_pkt_split = TX_PKT_SPLIT_OFF;
> /**< Split policy for packets to TX. */
>
> @@ -2659,24 +2662,9 @@ rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
> uint32_t prev_hdrs = 0;
> int ret;
>
> - /* Verify Rx queue configuration is single pool and segment or
> - * multiple pool/segment.
> - * @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,22 +2689,50 @@ 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) {
> /* multi-pool configuration */
> + struct rte_eth_dev_info dev_info;
> +
> + if (mbuf_data_size_n <= 1) {
> + RTE_LOG(ERR, EAL, "invalid number of mempools %u",
> + mbuf_data_size_n);
> + return -EINVAL;
> + }
> + ret = rte_eth_dev_info_get(port_id, &dev_info);
> + if (ret != 0)
> + return ret;
> + if (dev_info.max_rx_mempools == 0) {
> + RTE_LOG(ERR, EAL, "device doesn't support requested multi-mempool configuration");
> + return -ENOTSUP;
> + }
> for (i = 0; i < mbuf_data_size_n; i++) {
> mpx = mbuf_pool_find(socket_id, i);
> rx_mempool[i] = mpx ? mpx : mp;
> }
> 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);
> + }
> +
Technically execution can reach to this point without taking any of the
braches above, in that case there should be an error here instead of
silently continue.
I think either there should be a check here, not sure how to do, or
single mempool can be the default setup out of the 'else' block. What do
you think?
> 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..e4f9b142c9 100644
> --- a/app/test-pmd/testpmd.h
> +++ b/app/test-pmd/testpmd.h
> @@ -589,6 +589,7 @@ extern uint32_t max_rx_pkt_len;
> extern uint32_t rx_pkt_hdr_protos[MAX_SEGS_BUFFER_SPLIT];
> extern uint16_t rx_pkt_seg_lengths[MAX_SEGS_BUFFER_SPLIT];
> extern uint8_t rx_pkt_nb_segs; /**< Number of segments to split */
> +extern uint8_t multi_mempool; /**< Enables multi-mempool feature. */
> extern uint16_t rx_pkt_seg_offsets[MAX_SEGS_BUFFER_SPLIT];
> extern uint8_t rx_pkt_nb_offs; /**< Number of specified offsets */
>
> diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
> index 610e442924..329570e721 100644
> --- a/doc/guides/testpmd_app_ug/run_app.rst
> +++ b/doc/guides/testpmd_app_ug/run_app.rst
> @@ -365,6 +365,10 @@ The command line options are:
> Set TX segment sizes or total packet length. Valid for ``tx-only``
> and ``flowgen`` forwarding modes.
>
> +* ``--multi-mempool``
> +
> + Enable multi-mempool, multiple mbuf pools per Rx queue, support.
> +
> * ``--txonly-multi-flow``
>
> Generate multiple flows in txonly mode.
next prev parent reply other threads:[~2022-11-21 17:31 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-17 11:30 [PATCH v1 " Hanumanth Pothula
2022-11-17 12:55 ` [PATCH v2 " Hanumanth Pothula
2022-11-17 15:00 ` Singh, Aman Deep
2022-11-17 15:58 ` [EXT] " Hanumanth Reddy Pothula
2022-11-17 16:03 ` [PATCH v3 " Hanumanth Pothula
2022-11-17 23:36 ` Ferruh Yigit
2022-11-18 6:51 ` Han, YingyaX
2022-11-18 11:37 ` Hanumanth Reddy Pothula
2022-11-18 11:13 ` Hanumanth Pothula
2022-11-18 14:13 ` [PATCH v4 " Hanumanth Pothula
2022-11-18 20:56 ` Ferruh Yigit
2022-11-19 0:00 ` [EXT] " Hanumanth Reddy Pothula
2022-11-21 10:08 ` Ferruh Yigit
2022-11-21 10:44 ` Hanumanth Reddy Pothula
2022-11-21 12:45 ` [PATCH v5 " Hanumanth Pothula
2022-11-21 13:22 ` Ferruh Yigit
2022-11-21 13:36 ` [EXT] " Hanumanth Reddy Pothula
2022-11-21 14:10 ` Ferruh Yigit
2022-11-21 14:33 ` [PATCH v6 " Hanumanth Pothula
2022-11-21 17:31 ` Ferruh Yigit [this message]
2022-11-21 17:45 ` [EXT] " Hanumanth Reddy Pothula
2022-11-21 18:05 ` Ferruh Yigit
2022-11-21 18:07 ` [PATCH v7 " Hanumanth Pothula
2022-11-21 18:40 ` Ferruh Yigit
2022-11-22 6:42 ` Han, YingyaX
2022-11-22 6:52 ` Tang, Yaqi
2022-11-22 8:33 ` Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6696ea9a-442b-a10b-0ce9-dee07d5bacb2@amd.com \
--to=ferruh.yigit@amd.com \
--cc=aman.deep.singh@intel.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=dev@dpdk.org \
--cc=hpothula@marvell.com \
--cc=jerinj@marvell.com \
--cc=ndabilpuram@marvell.com \
--cc=thomas@monjalon.net \
--cc=yux.jiang@intel.com \
--cc=yuying.zhang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).