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

[Slava Ovsiienko <viacheslavo@nvidia.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Friday, October 16, 2020 14:21
> To: Slava Ovsiienko <viacheslavo@nvidia.com>; dev@dpdk.org
> Cc: NBU-Contact-Thomas Monjalon <thomas@monjalon.net>;
> stephen@networkplumber.org; olivier.matz@6wind.com;
> jerinjacobk@gmail.com; maxime.coquelin@redhat.com;
> david.marchand@redhat.com; arybchenko@solarflare.com
> Subject: Re: [PATCH v9 1/6] ethdev: introduce Rx buffer split
> 
> On 10/16/2020 11:22 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]
> > +
> > +* **[uses]       rte_eth_rxconf,rte_eth_rxmode**:
> ``offloads:RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT``.
> > +* **[uses]       rte_eth_rxconf**: ``rx_conf.rx_seg, rx_conf.rx_nseg``.
> > +* **[implements] datapath**: ``Buffer Split functionality``.
> > +* **[provides]   rte_eth_dev_info**:
> ``rx_offload_capa:RTE_ETH_RX_OFFLOAD_BUFFER_SPLIT``.
> > +* **[provides]   eth_dev_ops**: ``rxq_info_get:buffer_split``.
> 
> Previously you mentioned this is because 'rxq_info_get()' can provide
> buffer_split information, but with current implementation it doesn't and there
> is no filed in the struct to report such.
> 
> I suggest either add it now, while you can :) [with a techboard approval], or
> remove above documentation of it.
> 
> <...>
> 

Mmm, I messed up with rx_burst_mode_get(). Will fix, thanks.


> >   /**
> > + * Ethernet device Rx buffer segmentation capabilities.
> > + */
> > +__rte_experimental
> > +struct rte_eth_rxseg_capa {
> > +	__extension__
> > +	uint32_t max_nseg:16; /**< Maximum amount of segments to split. */
> > +	uint32_t multi_pools:1; /**< Supports receiving to multiple pools.*/
> > +	uint32_t offset_allowed:1; /**< Supports buffer offsets. */
> > +	uint32_t offset_align_log2:4; /**< Required offset alignment. */ };
> 
> Now we are fiddling details, but,
> 
> I am not fun of the bitfields [1], but I assumed Thomas' request was to enable
> expanding capabilities later without breaking the ABI, which makes senses and
> suits to this kind of capability struct, if this is correct why made the 'max_nseg'
> a bitfield too?
> 
> Why not,
> uint16_t max_nseg;
> uint16_t multi_pools:1
> uint16_t offset_allowed:1;
> uint16_t offset_align_log2:4;
> < This still leaves 10 bits to expand without ABI break>
> 
> [1]
> unles very space critical use case, otherwise they just add more code to extract
> the same value, and not as simple as a simple variable :)

It seems not to be the case,  there is the listing of the rte_eth_rx_queue_check_split():

 8963 4b67 440FB784      movzwl 188(%rsp),%r8d  ; [SO] max_nseg is fetched as regular uint16_t
 8963      24BC0000
 8963      00
 8964 4b70 664539C1      cmpw %r8w,%r9w
 8965 4b74 0F87A402      ja .L1749
 8965      0000

I would prefer to keep uint32_t - it is more generic, IMO.

With best regards, Slava