Re: [PATCH v4 1/3] ethdev: Add support for mulitiple mbuf pools per Rx queue

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

"Add support for" -> "add support for" or just "support" if
line is long

On 9/15/22 10:07, Hanumanth Pothula wrote:
> This patch adds support for multiple mempool capability.

"This patch adds" -> "Add"

> Some of the HW has support for choosing memory pools based on the
> packet's size. Thiscapability allows PMD to choose a memory pool

Thiscapability -> The capability

> based on the packet's length.
> 
> 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 effective use of memory.
> 
> For example, let's say HW has a capability of three pools,
>   - pool-1 size is 2K
>   - pool-2 size is > 2K and < 4K
>   - pool-3 size is > 4K
> Here,
>          pool-1 can accommodate packets with sizes < 2K
>          pool-2 can accommodate packets with sizes > 2K and < 4K
>          pool-3 can accommodate packets with sizes > 4K
> 
> With multiple mempool capability enabled in SW, an application may
> create three pools of different sizes and send them to PMD. Allowing
> PMD to program HW based on the packet lengths. So that packets with
> less than 2K are received on pool-1, packets with lengths between 2K
> and 4K are received on pool-2 and finally packets greater than 4K
> are received on pool-3.
> 
> Signed-off-by: Hanumanth Pothula <hpothula@marvell.com>

Please, advertise the new feature in release notes.

[snip]

> diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
> index 1979dc0850..8618d6b01d 100644
> --- a/lib/ethdev/rte_ethdev.c
> +++ b/lib/ethdev/rte_ethdev.c
> @@ -1634,6 +1634,45 @@ rte_eth_dev_is_removed(uint16_t port_id)
>   	return ret;
>   }
>   
> +static int
> +rte_eth_rx_queue_check_mempool(const struct rte_eth_rx_mempool *rx_mempool,
> +			       uint16_t n_pool, uint32_t *mbp_buf_size,
> +			       const struct rte_eth_dev_info *dev_info)
> +{
> +	uint16_t pool_idx;
> +
> +	if (n_pool > dev_info->max_pools) {
> +		RTE_ETHDEV_LOG(ERR,
> +			       "Invalid capabilities, max pools supported %u\n",

"Invalid capabilities" sounds misleading. Consider something
like:

"Too many Rx mempools %u vs maximum %u\n", n_pool, dev_info->max_pools

> +			       dev_info->max_pools);
> +		return -EINVAL;
> +	}
> +
> +	for (pool_idx = 0; pool_idx < n_pool; pool_idx++) {
> +		struct rte_mempool *mpl = rx_mempool[pool_idx].mp;
> +
> +		if (mpl == NULL) {
> +			RTE_ETHDEV_LOG(ERR, "null mempool pointer\n");

"null Rx mempool pointer\n"

> +			return -EINVAL;
> +		}
> +
> +		*mbp_buf_size = rte_pktmbuf_data_room_size(mpl);
> +		if (*mbp_buf_size < dev_info->min_rx_bufsize +
> +		    RTE_PKTMBUF_HEADROOM) {
> +			RTE_ETHDEV_LOG(ERR,
> +				       "%s mbuf_data_room_size %u < %u (RTE_PKTMBUF_HEADROOM=%u + min_rx_bufsize(dev)=%u)\n",
> +					mpl->name, *mbp_buf_size,
> +					RTE_PKTMBUF_HEADROOM + dev_info->min_rx_bufsize,
> +					RTE_PKTMBUF_HEADROOM,
> +					dev_info->min_rx_bufsize);
> +			return -EINVAL;
> +		}
> +

Please, remove extra empty line

> +	}

If Rx scatter is disabled, at least one mempool must be
sufficient for up to MTU packets.

> +
> +	return 0;
> +}
> +
>   static int
>   rte_eth_rx_queue_check_split(const struct rte_eth_rxseg_split *rx_seg,
>   			     uint16_t n_seg, uint32_t *mbp_buf_size,
> @@ -1733,7 +1772,8 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
>   
>   	if (mp != NULL) {
>   		/* Single pool configuration check. */
> -		if (rx_conf != NULL && rx_conf->rx_nseg != 0) {
> +		if (rx_conf != NULL &&
> +		    (rx_conf->rx_nseg != 0 ||  rx_conf->rx_npool)) {

rx_conf->rx_npool != 0 (as DPDK coding style says)

If mp is not NULL, it should be checked that neither buffer
split nor multiple mempool offloads are enabled.
Moreover, I think is a bug in a buffer split which
requires separate pre-patch. Check for rx_nsegs is 0 is
not required in fact since the offload flag must be used.

>   			RTE_ETHDEV_LOG(ERR,
>   				       "Ambiguous segment configuration\n");

segment -> Rx mempools

>   			return -EINVAL;
> @@ -1763,30 +1803,42 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,

>   				       dev_info.min_rx_bufsize);
>   			return -EINVAL;
>   		}
> -	} else {
> -		const struct rte_eth_rxseg_split *rx_seg;
> -		uint16_t n_seg;
> +	} else if (rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT ||
> +		  rx_conf->offloads & RTE_ETH_RX_OFFLOAD_MUL_MEMPOOL) {

May be:
(rx_conf->offloads & (RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT |
                       RTE_ETH_RX_OFFLOAD_MUL_MEMPOOL) != 0)
However, I'd split this branches to have more clear checks.
If we do not support both buffer split and multi-mempool
simultaneously - it must be checked. Just double check
that another offload is not requested.

>   
> -		/* Extended multi-segment configuration check. */
> -		if (rx_conf == NULL || rx_conf->rx_seg == NULL || rx_conf->rx_nseg == 0) {
> +		/* Extended multi-segment/pool configuration check. */
> +		if (rx_conf == NULL ||
> +		    (rx_conf->rx_seg == NULL && rx_conf->rx_mempool == NULL) ||
> +		    (rx_conf->rx_nseg == 0 && rx_conf->rx_npool == 0)) {

IMHO such generalized checks are wrong. We must check for
corresponding offload flag first.

>   			RTE_ETHDEV_LOG(ERR,
>   				       "Memory pool is null and no extended configuration provided\n");
>   			return -EINVAL;
>   		}
>   
> -		rx_seg = (const struct rte_eth_rxseg_split *)rx_conf->rx_seg;
> -		n_seg = rx_conf->rx_nseg;
> -
>   		if (rx_conf->offloads & RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT) {
> +			const struct rte_eth_rxseg_split *rx_seg =
> +				(const struct rte_eth_rxseg_split *)rx_conf->rx_seg;
> +			uint16_t n_seg = rx_conf->rx_nseg;
>   			ret = rte_eth_rx_queue_check_split(rx_seg, n_seg,
>   							   &mbp_buf_size,
>   							   &dev_info);
> -			if (ret != 0)
> +			if (ret)

Integers must be checked vs 0 explicitly in DPDK coding style.
Also the change looks unrelated.

>   				return ret;
> -		} else {
> -			RTE_ETHDEV_LOG(ERR, "No Rx segmentation offload configured\n");
> -			return -EINVAL;
>   		}
> +		if (rx_conf->offloads & RTE_ETH_RX_OFFLOAD_MUL_MEMPOOL) {
> +			const struct rte_eth_rx_mempool *rx_mempool =
> +				(const struct rte_eth_rx_mempool *)rx_conf->rx_mempool;
> +			ret = rte_eth_rx_queue_check_mempool(rx_mempool,
> +							     rx_conf->rx_npool,
> +							     &mbp_buf_size,
> +							     &dev_info);
> +			if (ret)
> +				return ret;
> +
> +		}
> +	} else {
> +		RTE_ETHDEV_LOG(ERR, "No Rx offload is configured\n");

THe log message is misleading. Consider:
"Missing Rx mempool configuration\n"

> +		return -EINVAL;
>   	}
>   
>   	/* Use default specified by driver, if nb_rx_desc is zero */
> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h
> index b62ac5bb6f..17deec2cbd 100644
> --- a/lib/ethdev/rte_ethdev.h
> +++ b/lib/ethdev/rte_ethdev.h
> @@ -1035,6 +1035,11 @@ union rte_eth_rxseg {
>   	/* The other features settings should be added here. */
>   };
>   
> +/* A common structure used to describe mbuf pools per Rx queue */
> +struct rte_eth_rx_mempool {
> +	struct rte_mempool *mp;
> +};

Why do we need it? Can we use below just
    struct rte_mempool *rx_mempools;

> +
>   /**
>    * A structure used to configure an Rx ring of an Ethernet port.
>    */
> @@ -1067,6 +1072,23 @@ struct rte_eth_rxconf {
>   	 */
>   	union rte_eth_rxseg *rx_seg;
>   
> +	/**
> +	 * Points to an array of mempools.
> +	 *

It should be highlighted that drivers should take a look at it
if and only if corresponding offload is enabled for the Rx
queue.

> +	 * This provides support for  multiple mbuf pools per Rx queue.
> +	 *
> +	 * This is often useful for saving the memory where the application can
> +	 * create a different pools to steer the specific size of the packet, thus
> +	 * enabling effective use of memory.
> +	 *
> +	 * Note that on Rx scatter enable, a packet may be delivered using a chain
> +	 * of mbufs obtained from single mempool or multiple mempools based on
> +	 * the NIC implementation.
> +	 *

Remove extra empty line above.

> +	 */
> +	struct rte_eth_rx_mempool *rx_mempool;
> +	uint16_t rx_npool; /** < number of mempools */
> +
>   	uint64_t reserved_64s[2]; /**< Reserved for future fields */
>   	void *reserved_ptrs[2];   /**< Reserved for future fields */
>   };

[snip]

> @@ -1615,6 +1638,7 @@ struct rte_eth_dev_info {
>   	/** Configured number of Rx/Tx queues */
>   	uint16_t nb_rx_queues; /**< Number of Rx queues. */
>   	uint16_t nb_tx_queues; /**< Number of Tx queues. */
> +	uint16_t max_pools;

Description of the new member is missing. Please, add it.

>   	/** Rx parameter recommendations */
>   	struct rte_eth_dev_portconf default_rxportconf;
>   	/** Tx parameter recommendations */