> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@intel.com>
> Sent: Friday, October 16, 2020 14:21
> To: Slava Ovsiienko <viachesl...@nvidia.com>; dev@dpdk.org
> Cc: NBU-Contact-Thomas Monjalon <tho...@monjalon.net>;
> step...@networkplumber.org; olivier.m...@6wind.com;
> jerinjac...@gmail.com; maxime.coque...@redhat.com;
> david.march...@redhat.com; arybche...@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
