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