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

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 10/16/20 12:15 PM, Slava Ovsiienko wrote:
> 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"

A bit confusing to have it outside of union far from split
but may be acceptable for the experimental API...
If there are better ideas - welcome.

>>
>>> +		/* 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.

Agreed.

> 
>>> +	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
>