Re: [RFC v2 3/7] ethdev: introduce Rx queue based limit watermark

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

On 5/24/22 11:18, Thomas Monjalon wrote:
> 24/05/2022 04:50, Spike Du:
>> From: Thomas Monjalon <thomas@monjalon.net>
>>> 23/05/2022 05:01, Spike Du:
>>>> From: Stephen Hemminger <stephen@networkplumber.org>
>>>>> Spike Du <spiked@nvidia.com> wrote:
>>>>>> --- a/lib/ethdev/rte_ethdev.h
>>>>>> +++ b/lib/ethdev/rte_ethdev.h
>>>>>> @@ -1249,7 +1249,16 @@ struct rte_eth_rxconf {
>>>>>>         */
>>>>>>        union rte_eth_rxseg *rx_seg;
>>>>>>
>>>>>> -     uint64_t reserved_64s[2]; /**< Reserved for future fields */
>>>>>> +     /**
>>>>>> +      * Per-queue Rx limit watermark defined as percentage of Rx queue
>>>>>> +      * size. If Rx queue receives traffic higher than this percentage,
>>>>>> +      * the event RTE_ETH_EVENT_RX_LWM is triggered.
>>>>>> +      */
>>>>>> +     uint8_t lwm;
>>>>>> +
>>>>>> +     uint8_t reserved_bits[3];
>>>>>> +     uint32_t reserved_32s;
>>>>>> +     uint64_t reserved_64s;
>>>>>
>>>>> Ok but, this is an ABI risk about this because reserved stuff was
>>>>> never required before.
>>>
>>> An ABI compatibility issue would be for an application compiled with an old
>>> DPDK, and loading a new DPDK at runtime.
>>> Let's think what would happen in such a case.
>>>
>>>>> Whenever is a reserved field is introduced the code (in this case
>>>>> rte_ethdev_configure).
>>>
>>> rte_eth_rx_queue_setup() is called with rx_conf->lwm not initialized.
>>> Then the library and drivers may interpret a wrong value.
>>>
>>>>> Best practice would have been to have the code require all reserved
>>>>> fields be
>>>>> 0 in earlier releases. In this case an application is like to define
>>>>> a watermark of zero; how will your code handle it.
>>>>
>>>> Having watermark of 0 is desired, which is the default. LWM of 0 means
>>>> the Rx Queue's watermark is not monitored, hence no LWM event is
>>> generated.
>>>
>>> The problem is to have a value not initialized.
>>> I think the best approach is to not expose the LWM value through this
>>> configuration structure.
>>> If the need is to get the current value, we should better add a field in the
>>> struct rte_eth_rxq_info.
>>
>> At least from all the dpdk app/example code, rxconf is initialized to 0 then setup
>> The Rx queue, if user follows these examples we should not have ABI issue.
>> Since many people are concerned about rxconf change, it's ok to remove the LWM
>> Field there.
>> Yes, I think we can add lwm into rte_eth_rxq_info. If we can set Rx queue's attribute,
>> We should have a way to get it.
> 
> Unfortunately we cannot rely on examples for ABI compatibility.
> My suggestion of moving the field in rte_eth_rxq_info
> is not obvious because it could change the size of the struct.
> But thanks to __rte_cache_min_aligned, it is OK.
> Running pahole on this struct shows we have 50 bytes free:
>          /* size: 128, cachelines: 2, members: 6 */
>          /* padding: 50 */
> 
> The other option would be to get the LWM value with a "get" function.
> 
> What others prefer?

If I'm not mistaken the changeset breaks ABI in any case since
it adds a new event and changes MAX. If so, I'd wait for the
next ABI breaking release and do not touch reserved fields.