Re: [PATCH v7 2/4] ethdev: support mulitiple mbuf pools per Rx queue

[Thomas Monjalon <thomas@monjalon.net>]

07/10/2022 16:37, Andrew Rybchenko:
> From: Hanumanth Pothula <hpothula@marvell.com>
> 
> Some of the HW has support for choosing memory pools based on the
> packet's size. The capability allows to choose a memory pool based
> on the packet's length.

The second sentence is redundant.

> This is often useful for saving the memory where the application
> can create a different pool to steer the specific size of the
> packet, thus enabling more efficient usage of memory.
[...]
> +* **Added support for mulitiple mbuf pools per ethdev Rx queue.**

mulitiple -> multiple

> +
> +  * Added support for multiple mbuf pools per Rx queue. The capability allows

No need to repeat the title.

> +    application to provide many mempools of different size and PMD to choose
> +    a memory pool based on the packet's length and/or Rx buffers availability.
[...]
> +	/* Ensure that we have one and only one source of Rx buffers */
> +	if ((mp != NULL) +

+ operator?
Are we sure a boolean is always translated as 1?

> +	    (rx_conf != NULL && rx_conf->rx_nseg > 0) +
> +	    (rx_conf != NULL && rx_conf->rx_nmempool > 0) != 1) {
> +		RTE_ETHDEV_LOG(ERR,
> +			       "Ambiguous Rx mempools configuration\n");
> +		return -EINVAL;
> +	}
[...]
> @@ -1067,6 +1067,24 @@ struct rte_eth_rxconf {
>  	 */
>  	union rte_eth_rxseg *rx_seg;
>  
> +	/**
> +	 * Array of mempools to allocate Rx buffers from.
> +	 *
> +	 * This provides support for multiple mbuf pools per Rx queue.
> +	 * The capability is reported in device info via positive
> +	 * max_rx_mempools.
> +	 *
> +	 * It could be useful for more efficient usage of memory when an
> +	 * application creates different mempools to steer the specific
> +	 * size of the packet.
> +	 *
> +	 * Note that if Rx scatter is enabled, a packet may be delivered using
> +	 * a chain of mbufs obtained from single mempool or multiple mempools
> +	 * based on the NIC implementation.
> +	 */
> +	struct rte_mempool **rx_mempools;
> +	uint16_t rx_nmempool; /** < Number of Rx mempools */

The commit message suggests a configuration per packet size.
I guess it is not configurable in ethdev API?
If it is hard-configured in the HW or the driver only,
it should be specified here.

[...]
> +	/**
> +	 * Maximum number of Rx mempools supported per Rx queue.
> +	 *
> +	 * Value greater than 0 means that the driver supports Rx queue
> +	 * mempools specification via rx_conf->rx_mempools.
> +	 */
> +	uint16_t max_rx_mempools;