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?
> Signed-off-by: Hanumanth Pothula <hpoth...@marvell.com>
Please put the changelog after '---', which than git will take it as note.
> 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.
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.
