> -----Original Message-----
> From: Thomas Monjalon <tho...@monjalon.net>
> Sent: Friday, October 16, 2020 18:14
> To: Slava Ovsiienko <viachesl...@nvidia.com>
> Cc: dev@dpdk.org; step...@networkplumber.org; ferruh.yi...@intel.com;
> olivier.m...@6wind.com; jerinjac...@gmail.com;
> maxime.coque...@redhat.com; david.march...@redhat.com;
> arybche...@solarflare.com
> Subject: Re: [dpdk-dev] [PATCH v11 1/6] ethdev: introduce Rx buffer split
>
> 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 <tho...@monjalon.net>
All comments are adopted (in coming v12), thank you.
With best regards, Slava
