On 9/24/2022 5:34 PM, Chautru, Nicolas wrote:
Hi Ferruh, Ray, Akhil,
-----Original Message-----
From: Ferruh Yigit <ferruh.yi...@amd.com>
Sent: Friday, September 23, 2022 4:28 PM
To: Akhil Goyal <gak...@marvell.com>; Nicolas Chautru
<nicolas.chau...@intel.com>; dev@dpdk.org; tho...@monjalon.net;
hemant.agra...@nxp.com; Ray Kinsella <m...@ashroe.eu>
Cc: maxime.coque...@redhat.com; t...@redhat.com;
bruce.richard...@intel.com; david.march...@redhat.com;
step...@networkplumber.org; mingshan.zh...@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);
---
