Re: [EXT] [PATCH v7 6/7] bbdev: add queue related warning and status information

[Ferruh Yigit <ferruh.yigit@amd.com>]

On 9/24/2022 5:34 PM, Chautru, Nicolas wrote:
> Hi Ferruh, Ray, Akhil,
> 
> 
>>> -----Original Message-----
>>> From: Ferruh Yigit <ferruh.yigit@amd.com>
>>> Sent: Friday, September 23, 2022 4:28 PM
>>> To: Akhil Goyal <gakhil@marvell.com>; Nicolas Chautru
>>> <nicolas.chautru@intel.com>; dev@dpdk.org; thomas@monjalon.net;
>>> hemant.agrawal@nxp.com; Ray Kinsella <mdr@ashroe.eu>
>>> Cc: maxime.coquelin@redhat.com; trix@redhat.com;
>>> bruce.richardson@intel.com; david.marchand@redhat.com;
>>> stephen@networkplumber.org; mingshan.zhang@intel.com
>>> Subject: Re: [EXT] [PATCH v7 6/7] bbdev: add queue related warning and
>>> status information
>>>
>>> On 9/21/2022 8:21 PM, Akhil Goyal wrote:
>>>>> diff --git a/lib/bbdev/rte_bbdev.h b/lib/bbdev/rte_bbdev.h index
>>>>> ed528b8..b7ecf94 100644
>>>>> --- a/lib/bbdev/rte_bbdev.h
>>>>> +++ b/lib/bbdev/rte_bbdev.h
>>>>> @@ -224,6 +224,19 @@ struct rte_bbdev_queue_conf {
>>>>>    rte_bbdev_queue_stop(uint16_t dev_id, uint16_t queue_id);
>>>>>
>>>>>    /**
>>>>> + * Flags indicate the reason why a previous enqueue may not have
>>>>> + * consumed all requested operations
>>>>> + * In case of multiple reasons the latter superdes a previous one
>>>> Spell check - supersedes.
>>>>
>>>>> + */
>>>>> +enum rte_bbdev_enqueue_status {
>>>>> +	RTE_BBDEV_ENQ_STATUS_NONE,             /**< Nothing to report */
>>>>> +	RTE_BBDEV_ENQ_STATUS_QUEUE_FULL,       /**< Not enough room
>> in
>>>>> queue */
>>>>> +	RTE_BBDEV_ENQ_STATUS_RING_FULL,        /**< Not enough room
>> in
>>>>> ring */
>>>>> +	RTE_BBDEV_ENQ_STATUS_INVALID_OP,       /**< Operation was
>>>>> rejected as invalid */
>>>>> +	RTE_BBDEV_ENQ_STATUS_PADDED_MAX = 6,   /**< Maximum enq
>>>>> status number including padding */
>>>>
>>>> Are we ok to have this kind of padding across DPDK for all the enums
>>>> to avoid
>>> ABI issues?
>>>> @Ray, @Thomas: any thoughts?
>>>>
>>>>
>>>
>>> This kind of usage can prevent ABI tool warning, and can fix issues
>>> caused by application using returned enum as index [1].
>>>
>>> But I think it is still problematic in case application tries to walk
>>> through till MAX, that is a common usage [2], user may miss that this
>>> is PADDED.	
> 
> Hi Ferruh,
> I don’t believe that case can happen here. Even if application was using an undefined index, the related functions are protected for that :
> See rte_bbdev_enqueue_status_str(enum rte_bbdev_enqueue_status status)

It doesn't have to use undefined index for DPDK API, application can 
iterate until MAX enum for application specific array or function.

> The reason for having padded max, is that the application may use this for array sizing if required without concern, we will never return a value that would exceeds this.
> In the other direction that is not a problem either since application (even it needs to store thigs in array) can used the padded version.
> Note that this discussed with Ray notably as a BKM.
> 

I can see usage is more for size, that is why I think it is better to 
have a #define that has SIZE_MAX in name, instead of enum with 
PADDED_MAX name.

As said above, since you will never return value exceeds PADDED_MAX it 
solves the issue that application using returned enum as index [1]. So, 
this usage is not that problematic.

But my concern was if user assumes all values valid until PADDED_MAX and 
tries to iterate array until that value [2].


>>>
>>> Overall exchanging enum values between library and application is
>>> possible trouble for binary compatibility. If application and library
>>> uses enum values independently, this is OK.
>>> Since enum cases not deleted but added in new version of the
>>> libraries, more problematic usage is passing enum value from library
>>> to application, and bbdev seems doing this in a few places.
> 
> I don’t see a case where it is a genuine issue.
> An enum is being reported from library, even if due to future enum insertion there is a new enum reported between 2 ABI changes, that would still be within bounds.
> 

Passing enum case from library to application has problem [1], other 
issue can be application may miss that library added new enum cases and 
application code needs updating.
Overall I think it is not good idea for library to exchange enum values 
in APIs, specially if there is an intention to have ABI compatibility.

>>>
>>> With current approach PADDED_MAX usage is more like #define usage, it
>>> is not dynamic but a hardcoded value that is used as array size value.
>>>
>>> Not providing a MAX enum case restricts the application, application
>>> can't use it as size of an array or can't use it walk through related
>>> array, usage reduces to if/switch comparisons.
> 
> It can use the padded_max to size application array. Even if application was walking through these, there is no adverse effect.
> 
>>> Although this may not be most convenient for application, it can
>>> provide safer usage for binary compatibility.
>>>
>>>
>>> @Nic, what do you think provide these PADDED_MAX as #define SIZE_MAX
>>> macros?
>>> With this application still can allocate a relevant array with correct
>>> size, or know the size of library provided array, but can't use it to
>>> iterate on these arrays.
>>>
> 
> That would be back to how it was done before which made things very inflexible and prevented to change these enums between ABIs versions.
> 

It would be same, the problem was MAX enum value changing. Removing MAX 
enum but introduce #define SIZE_MAX will be same for application.
Only it would be more clear.

> This change was highlighted and discussed many months ago and flagged in the deprecation notice in previous release for that very reason.
> 

As far as I remember the deprecation notice was to remove MAX enum 
values, now you are introducing PADDED_MAX enum value.

> Ray, can you please chime in since you know best.
> 

I think this PADDED_MAX solution is not too problematic, but I prefer 
SIZE_MAX define, I hope my reasoning above is clear.
This is a comment from me, not a blocker, I am OK to go with whatever 
consensus is.

> Thanks Ferruh,
> Nic
> 
> 
> 
>>>
>>>
>>>
>>> [1]
>>> --------------- library old version ---------------------------- enum
>>> type {
>>> 	CASE_A,
>>> 	CASE_B,
>>> 	CASE_MAX,
>>> };
>>>
>>> struct data {
>>> 	enum type type;
>>> 	union {
>>> 		type specific struct
>>> 	};
>>> };
>>>
>>> int api_get(struct data *data);
>>>
>>>
>>> --------------- application ----------------------------
>>>
>>> struct data data;
>>> int array[CASE_MAX];
>>>
>>> api_get(&data);
>>> foo(array[data.type]);
>>>
>>>
>>> --------------- library NEW version ----------------------------
>>>
>>> enum type {
>>> 	CASE_A,
>>> 	CASE_B,
>>> 	CASE_C,
>>> 	CASE_D,
>>> 	CASE_MAX,
>>> };
>>>
>>>
>>> When application is NOT recompiled but using new version of the
>>> library, values 'CASE_C' & 'CASE_D' will crash application, so this
>>> will create a ABI compatibility issue.
>>>
>>> Note: In the past I have claimed that application should check
>>> 'CASE_MAX', instead of using returned value directly as index, but
>>> this is refused by argument that we can't control the application and
>>> should play safe assuming application behaves wrong.
>>>
>>>
>>>
>>>
>>> [2]
>>>
>>> --------------- library ----------------------------
>>>
>>> enum type {
>>> 	CASE_NONE,
>>> 	CASE_A,
>>> 	CASE_B,
>>> 	CASE_C,
>>> 	CASE_D,
>>> 	CASE_PADDED_MAX = 666,
>>> };
>>>
>>> --------------- application ----------------------------
>>>
>>> for (int i = CASE_NONE; i < CASE_PADDED_MAX; i++)
>>> 	fragile_init(i);
>>>
>>> ---
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>>>
>