RE: [EXT] Re: [PATCH v4 1/1] app/testpmd: add valid check to verify multi mempool feature

[Hanumanth Reddy Pothula <hpothula@marvell.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@amd.com>
> Sent: Monday, November 21, 2022 3:38 PM
> To: Hanumanth Reddy Pothula <hpothula@marvell.com>;
> thomas@monjalon.net; andrew.rybchenko@oktetlabs.ru; Nithin Kumar
> Dabilpuram <ndabilpuram@marvell.com>
> Cc: dev@dpdk.org; yux.jiang@intel.com; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Aman Singh <aman.deep.singh@intel.com>; Yuying
> Zhang <yuying.zhang@intel.com>
> Subject: Re: [EXT] Re: [PATCH v4 1/1] app/testpmd: add valid check to
> verify multi mempool feature
> 
> On 11/19/2022 12:00 AM, Hanumanth Reddy Pothula wrote:
> >
> >
> >> -----Original Message-----
> >> From: Ferruh Yigit <ferruh.yigit@amd.com>
> >> Sent: Saturday, November 19, 2022 2:26 AM
> >> To: Hanumanth Reddy Pothula <hpothula@marvell.com>;
> >> thomas@monjalon.net; andrew.rybchenko@oktetlabs.ru; Nithin Kumar
> >> Dabilpuram <ndabilpuram@marvell.com>
> >> Cc: dev@dpdk.org; yux.jiang@intel.com; Jerin Jacob Kollanukkaran
> >> <jerinj@marvell.com>; Aman Singh <aman.deep.singh@intel.com>;
> Yuying
> >> Zhang <yuying.zhang@intel.com>
> >> Subject: [EXT] Re: [PATCH v4 1/1] app/testpmd: add valid check to
> >> verify multi mempool feature
> >>
> >> External Email
> >>
> >> ---------------------------------------------------------------------
> >> - On 11/18/2022 2:13 PM, Hanumanth Pothula wrote:
> >>> Validate ethdev parameter 'max_rx_mempools' to know whether
> device
> >>> supports multi-mempool feature or not.
> >>>
> >>
> >> My preference would be revert the testpmd patch [1] that adds this
> >> new feature after -rc2, and add it back next release with new testpmd
> >> argument and below mentioned changes in setup function.
> >>
> >> @Andrew, @Thomas, @Jerin, what do you think?
> >>
> >>
> >> [1]
> >> 4f04edcda769 ("app/testpmd: support multiple mbuf pools per Rx
> >> queue")
> >>
> >>> Bugzilla ID: 1128
> >>>
> >>
> >> Can you please add fixes line?
> >>
> > Ack
> >>> Signed-off-by: Hanumanth Pothula <hpothula@marvell.com>
> >>
> >> Please put the changelog after '---', which than git will take it as note.
> >>
> > Ack
> >>> v4:
> >>>  - updated if condition.
> >>> v3:
> >>>  - Simplified conditional check.
> >>>  - Corrected spell, whether.
> >>> v2:
> >>>  - Rebased on tip of next-net/main.
> >>> ---
> >>>  app/test-pmd/testpmd.c | 10 ++++++++--
> >>>  1 file changed, 8 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> >>> 4e25f77c6a..c1b4dbd716 100644
> >>> --- a/app/test-pmd/testpmd.c
> >>> +++ b/app/test-pmd/testpmd.c
> >>> @@ -2655,17 +2655,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))) {
> >>> +	if ((dev_info.max_rx_mempools == 0) && (rx_pkt_nb_segs <= 1 ||
> >>
> >> Using `dev_info.max_rx_mempools` for check means if device supports
> >> multiple mempool, multiple mempool will be configured independent
> >> from user configuration. But user may prefer singe mempool or buffer
> split.
> >>
> > Please find my suggested logic.
> >
> >> Right now only PMD support multiple mempool is 'cnxk', so this
> >> doesn't impact others but I think this is not correct.
> >>
> >> Instead of re-using testpmd "mbuf-size" parameter (it is already used
> >> for two other features, and this is the reason of the defect) it
> >> would be better to have an explicit parameter for multiple mempool
> feature.
> >>
> >>
> >>> +	    ((rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT) ==
> >> 0))) {
> >>>  		/* Single pool/segment configuration */
> >>>  		rx_conf->rx_seg = NULL;
> >>>  		rx_conf->rx_nseg = 0;
> >>
> >>
> >> Logic seems correct, although I have not tested.
> >>
> >> Current functions tries to detect the requested feature and setup
> >> queues accordingly, features are:
> >> - single mempool
> >> - packet split (to multiple mempool)
> >> - multiple mempool (various size)
> >>
> >> And the logic in the function is:
> >> ``
> >> if ( (! multiple mempool) && (! packet split))
> >> 	setup for single mempool
> >> 	exit
> >>
> >> if (packet split)
> >> 	setup packet split
> >> else
> >> 	setup multiple mempool
> >> ``
> >>
> >> What do you think to
> >> a) simplify logic by making single mempool as fallback and last
> >> option, instead of detecting non existence of other configs
> >> b) have explicit check for multiple mempool
> >>
> >> Like:
> >>
> >> ``
> >> if (packet split)
> >> 	setup packet split
> >> 	exit
> >> else if (multiple mempool)
> >> 	setup multiple mempool
> >> 	exit
> >>
> >> setup for single mempool
> >> ``
> >>
> >> I think this both solves the defect and simplifies the code.
> >
> > Yes Ferruh your suggested logic simplifies the code.
> >
> > In the lines of your proposed logic,  below if conditions might work
> > fine for all features(buffer-split/multi-mempool) supported by PMD and
> > user preference,
> >
> > if (rx_pkt_nb_segs > 1 ||
> >             rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT) {
> > 	/*multi-segment (buffer split)*/
> > } else if (mbuf_data_size_n > 1 && dev_info.max_rx_mempools > 1) {
> > 	/*multi-mempool*/
> > } else {
> > 	/* single pool and segment */
> > }
> >
> 
> `mbuf_data_size_n > 1` may mean user is requesting multiple segment, or
> buffer split, so I am not sure about using this value to decide on
> multiple mempool feature, it can create side effect as Bug 1128 does.
> 
> > Or  adding new Rx offload parameter for multi_mempool feature, I think it
> might not be required, using dev_info.max_rx_mempools works fine.
> >
> 
> In ethdev level, we don't need an offload flag, since separated config
> options clarify the intention there.
> What is needed is a way to understand users intention, for application
> (testpmd) and configure device accordingly.
> That is why I think 'dev_info.max_rx_mempools' is not working fine,
> because that is a way for device to say multiple mempool is supported,
> it is not to get user intention.
> In your logic when device supports multiple mempool, use it independent
> from what user request.
> 
> I suggest having a testpmd argument explicitly to say multiple mempool
> feature is requested, this will help to distinguish buffer split,
> multiple mempool, single mempool features.
> 
> Thanks.

Sure, will upload new patch-set with testpmd argument, which tells multiple mempool feature is enabled/disabled.

> 
> > if (rx_pkt_nb_segs > 1 ||
> >             rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT) {
> > 	/*multi-segment (buffer split)*/
> > } else if (mbuf_data_size_n > 1 && rx_conf->offloads &
> RTE_ETH_RX_OFFLOAD_MULTI_MEMPOOL ) {
> > 	/*multi-mempool*/
> > } else {
> > 	/* single pool and segment */
> > }
> >
> > Please let me know your inputs on above logic.
> >