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

[Slava Ovsiienko <viacheslavo@nvidia.com>]

Hi, Andrew

> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> Sent: Friday, October 16, 2020 11:52
> To: Slava Ovsiienko <viacheslavo@nvidia.com>; dev@dpdk.org
> Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
> stephen@networkplumber.org; ferruh.yigit@intel.com;
> olivier.matz@6wind.com; jerinjacobk@gmail.com;
> maxime.coquelin@redhat.com; david.marchand@redhat.com
> Subject: Re: [PATCH v8 1/6] ethdev: introduce Rx buffer split
> 
> On 10/16/20 10:48 AM, Viacheslav Ovsiienko wrote:
> > The DPDK datapath in the transmit direction is very flexible.
> > An application can build the multi-segment packet and manages almost
> > all data aspects - the memory pools where segments are allocated from,
> > the segment lengths, the memory attributes like external buffers,
> > registered for DMA, etc.
> >
[snip]
> > +struct rte_eth_rxseg {
> > +	union {
> 
> Why not just 'union rte_eth_rxseg' ?
> 
> > +		/* The settings for buffer split offload. */
> > +		struct rte_eth_rxseg_split split;
> 
> Pointer to a split table must be here. I.e.
> struct rte_eth_rxseg_split *split;
OK, will try to simplify with that, thanks.

> Also it must be specified how the array is terminated.
> We need either a number of define last item condition (mp == NULL ?)

We have one, please see: "rte_eth_rxconf->rx_nseg"
> 
> > +		/* The other features settings should be added here. */
> > +	} conf;
> > +};
> 
> 
> 
> > +
> > +/**
> >   * A structure used to configure an RX ring of an Ethernet port.
> >   */
> >  struct rte_eth_rxconf {
> > @@ -977,6 +998,46 @@ 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.
> */
> > +	struct rte_eth_rxseg *rx_seg;
> 
> It must not be a pointer. It looks really strange this way taking into account
> that it is a union in fact.
> Also, why is it put here in the middle of exsiting structure?
> IMHO it should be added after offlaods.
OK, agree, will move.

> 
> >  	/**
> >  	 * Per-queue Rx offloads to be set using DEV_RX_OFFLOAD_* flags.
> >  	 * Only offloads set on rx_queue_offload_capa or rx_offload_capa @@
> > -1260,6 +1321,7 @@ struct rte_eth_conf {
> >  #define DEV_RX_OFFLOAD_SCTP_CKSUM	0x00020000
> >  #define DEV_RX_OFFLOAD_OUTER_UDP_CKSUM  0x00040000
> >  #define DEV_RX_OFFLOAD_RSS_HASH		0x00080000
> > +#define RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT 0x00100000
> >
> >  #define DEV_RX_OFFLOAD_CHECKSUM (DEV_RX_OFFLOAD_IPV4_CKSUM | \
> >  				 DEV_RX_OFFLOAD_UDP_CKSUM | \
> > @@ -1376,6 +1438,17 @@ struct rte_eth_switch_info {  };
> >
> >  /**
> > + * Ethernet device Rx buffer segmentation capabilities.
> > + */
> > +__extension__
> > +struct rte_eth_rxseg_capa {
> > +	uint16_t max_seg; /**< Maximum amount of segments to split. */
> 
> May be 'max_segs' to avoid confusing vs maximum segment length.
> 
OK, max_nseg would be more appropriate.

> > +	uint16_t multi_pools:1; /**< Supports receiving to multiple pools.*/
> > +	uint16_t offset_allowed:1; /**< Supports buffer offsets. */
> > +	uint16_t offset_align_log2:4; /**< Required offset alignment. */
> 
> 4 bits are even insufficient to specify cache-line alignment.
> IMHO at least 8 bits are required.

4 bits seems to be quite sufficient. It is a log2, tells how many lsbs in offset should be zeroes.
2^15 is 32K, it covers all reasonable alignments for uint16_t type.

> 
> Consider to put 32 width bit-fields at start of the structure.
> Than, max_segs (16), offset_align_log2 (8), plus reserved (8).
OK, np.

With best regards, Slava