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