Re: [dpdk-dev] [PATCH v11 1/6] ethdev: introduce Rx buffer split

[Thomas Monjalon <thomas@monjalon.net>]

16/10/2020 15:39, Viacheslav Ovsiienko:
> +		if (offset != 0) {
> +			if (seg_capa->offset_allowed == 0) {
> +				RTE_ETHDEV_LOG(ERR, "Rx segmentation with offset is not supported\n");
> +				return -ENOTSUP;
> +			}
> +			if (offset & offset_mask) {
> +				RTE_ETHDEV_LOG(ERR, "Rx segmentat invalid offset alignment %u, %u\n",

small typo: segmentat -> segmentation

[...]
>  /**
> + * A structure used to configure an Rx packet segment to split.
> + *
> + * If RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT flag is set in offloads field,
> + * the PMD will split the received packets into multiple segments
> + * according to the specification in the description array:
> + *

Minor detail: below bullets should start with a capital letter.

> + * - the first network buffer will be allocated from the memory pool,
> + *   specified in the first array element, the second buffer, from the
> + *   pool in the second element, and so on.
> + *
> + * - the offsets from the segment description elements specify
> + *   the data offset from the buffer beginning except the first mbuf.
> + *   For this one the offset is added with RTE_PKTMBUF_HEADROOM.

"this one" -> "the first segment, "

> + *
> + * - the lengths in the elements define the maximal data amount
> + *   being received to each segment. The receiving starts with filling
> + *   up the first mbuf data buffer up to specified length. If the
> + *   there are data remaining (packet is longer than buffer in the first
> + *   mbuf) the following data will be pushed to the next segment
> + *   up to its own length, and so on.
> + *
> + * - If the length in the segment description element is zero
> + *   the actual buffer size will be deduced from the appropriate
> + *   memory pool properties.
> + *
> + * - if there is not enough elements to describe the buffer for entire
> + *   packet of maximal length the following parameters will be used
> + *   for the all remaining segments:
> + *     - pool from the last valid element
> + *     - the buffer size from this pool
> + *     - zero offset
> + */

I think the description is precise enough, thanks.

> +__rte_experimental
> +struct rte_eth_rxseg_split {
> +	struct rte_mempool *mp; /**< Memory pool to allocate segment from. */
> +	uint16_t length; /**< Segment data length, configures split point. */
> +	uint16_t offset; /**< Data offset from beginning of mbuf data buffer. */
> +	uint32_t reserved; /**< Reserved field. */
> +};
> +
> +/**
> + * A common structure used to describe Rx packet segment properties.
> + */
> +__rte_experimental
> +union rte_eth_rxseg {
> +	/* The settings for buffer split offload. */
> +	struct rte_eth_rxseg_split split;
> +	/* The other features settings should be added here. */
> +};

OK for having an union.

> +
> +/**
>   * A structure used to configure an RX ring of an Ethernet port.
>   */
>  struct rte_eth_rxconf {
> @@ -977,12 +1028,22 @@ struct rte_eth_rxconf {
>  	uint16_t rx_free_thresh; /**< Drives the freeing of RX descriptors. */
>  	uint8_t rx_drop_en; /**< Drop packets if no descriptors are available. */
>  	uint8_t rx_deferred_start; /**< Do not start queue with rte_eth_dev_start(). */
> +	uint16_t rx_nseg; /**< Number of descriptions in rx_seg array. */
>  	/**
>  	 * Per-queue Rx offloads to be set using DEV_RX_OFFLOAD_* flags.
>  	 * Only offloads set on rx_queue_offload_capa or rx_offload_capa
>  	 * fields on rte_eth_dev_info structure are allowed to be set.
>  	 */
>  	uint64_t offloads;
> +	/**
> +	 * Points to the array of segment descriptions. Each array element
> +	 * describes the properties for each segment in the receiving
> +	 * buffer according to feature descripting structure.

Alternative (shorter) wording:
Points to the array of segment descriptions for an entire packet.
Array elements are properties for consecutive Rx segments.

> +	 *
> +	 * The supported capabilities of receiving segmentation is reported
> +	 * in rte_eth_dev_info ->rx_seg_capa field.

Rx segmentation capabilities are reported in rte_eth_dev_info.rx_seg_capa.

> +	 */
> +	union rte_eth_rxseg *rx_seg;

[...]
> + *   The configuration structure also contains the pointer to the array
> + *   of the receiving buffer segment descriptions, see rx_seg and rx_nseg
> + *   fields, this extended configuration might be used by split offloads like
> + *   RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT. If mp_pool is not NULL,
> + *   the extended configuration fields must be set to NULL and zero.
>   * @param mb_pool
>   *   The pointer to the memory pool from which to allocate *rte_mbuf* network
> - *   memory buffers to populate each descriptor of the receive ring.
> + *   memory buffers to populate each descriptor of the receive ring. There are
> + *   two options to provide Rx buffer configuration:
> + *   - single pool:
> + *     mb_pool is not NULL, rx_conf.rx_seg is NULL, rx_conf.rx_nseg is 0.
> + *   - multiple segments description:
> + *     mb_pool is NULL, rx_conf.rx_seg is not NULL, rx_conf.rx_nseg is not 0.
> + *     Taken only if flag RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT is set in offloads.

As said previously, I feel rx_conf.rx_seg does not need to be part
of the requirements, as long as rx_conf.rx_nseg is defined.

All my comments are optional, this version is already very good.

Acked-by: Thomas Monjalon <thomas@monjalon.net>