[v4,1/3] ethdev: Add support for mulitiple mbuf pools per Rx queue
Checks
Commit Message
This patch adds support for multiple mempool capability.
Some of the HW has support for choosing memory pools based on the
packet's size. Thiscapability allows PMD to choose a memory pool
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>
v4:
- Renamed Offload capability name from RTE_ETH_RX_OFFLOAD_BUFFER_SORT
to RTE_ETH_RX_OFFLOAD_MUL_MEMPOOL.
- In struct rte_eth_rxconf, defined new pointer, which holds array of
type struct rte_eth_rx_mempool(memory pools). This array is used
by PMD to program multiple mempools.
v3:
- Implemented Pool Sort capability as new Rx offload capability,
RTE_ETH_RX_OFFLOAD_BUFFER_SORT.
v2:
- Along with spec changes, uploading testpmd and driver changes.
---
lib/ethdev/rte_ethdev.c | 78 ++++++++++++++++++++++++++++++++++-------
lib/ethdev/rte_ethdev.h | 24 +++++++++++++
2 files changed, 89 insertions(+), 13 deletions(-)
Comments
"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 */
28/09/2022 11:43, Andrew Rybchenko:
> "Add support for" -> "add support for" or just "support" if
> line is long
Even if line is not too long, we all prefer shorter lines,
"add support for" should always be replaced by "support" in titles.
Note I am often doing such change when pulling.
@@ -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",
+ 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");
+ 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;
+ }
+
+ }
+
+ 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)) {
RTE_ETHDEV_LOG(ERR,
"Ambiguous segment configuration\n");
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) {
- /* 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)) {
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)
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");
+ return -EINVAL;
}
/* Use default specified by driver, if nb_rx_desc is zero */
@@ -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;
+};
+
/**
* 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.
+ *
+ * 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.
+ *
+ */
+ 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 */
};
@@ -1395,6 +1417,7 @@ struct rte_eth_conf {
#define RTE_ETH_RX_OFFLOAD_OUTER_UDP_CKSUM RTE_BIT64(18)
#define RTE_ETH_RX_OFFLOAD_RSS_HASH RTE_BIT64(19)
#define RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT RTE_BIT64(20)
+#define RTE_ETH_RX_OFFLOAD_MUL_MEMPOOL RTE_BIT64(21)
#define RTE_ETH_RX_OFFLOAD_CHECKSUM (RTE_ETH_RX_OFFLOAD_IPV4_CKSUM | \
RTE_ETH_RX_OFFLOAD_UDP_CKSUM | \
@@ -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;
/** Rx parameter recommendations */
struct rte_eth_dev_portconf default_rxportconf;
/** Tx parameter recommendations */