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

["Chautru, Nicolas" <nicolas.chautru@intel.com>]

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

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

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

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

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

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