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

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

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.

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.


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




[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);

---