[v7,6/7] bbdev: add queue related warning and status information

  This allows to expose more information with regards to any
queue related failure and warning which cannot be supported
in existing API.

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 app/test-bbdev/test_bbdev_perf.c |  2 ++
 lib/bbdev/rte_bbdev.c            | 19 +++++++++++++++++++
 lib/bbdev/rte_bbdev.h            | 34 ++++++++++++++++++++++++++++++++++
 lib/bbdev/version.map            |  1 +
 4 files changed, 56 insertions(+)
  

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

Hi Akhil, 

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> 
> > 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.

Thanks, fixed in v8.

> 
> > + */
> > +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 was discussed at the time notably with advises from Ray. But good to close the loop formally.
  

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

---
  

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

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

Hi Ferruh, 

Thanks for your comment. 
To be totally honest I don't yet see how your suggestion would be better, but I quite possibly miss something. I did not reply in line with your comments so that to try to be clearer and avoid spreading the argument to much. Ray and Bruce feel free to chime in as well.

First to state the obvious: Nothing will change the fact that in case new enums are being added in DPDK, and if the application doesn't change, then user would not be able to interpret any such additional status/capability (backward compatible only feature parity and still ABI compliant) which is totally accepted as fine and up to the user, but the intention is at least not to have adverse effect even when they don’t update their code for such new features (notably in case they just use an older PMD not supporting such new features as a basic typical example in the ecosystem). I think we agree on that problematic. 

In term of history of not using MAX value for enum, I believe there is already well documented and you agree with the reasoning of why we had to move away from this [1]. Not just cosmetically where the max value is called an enum or a #define but to have application making hardcoded assumption on the possible maximum range for such enum notably when sizing array. The only caveat being that at the time, the community did spot the weakness but did not come to an agreement with regards to the best way to manage this moving forward. 

In case your point is purely cosmetic to rename the PADDED_MAX value from the enum to a #define (both public) I don't see how this would make thing clearer by obfuscating the fact it is genuinely a padded value and to have that value directly related to the enum structure. Also note that there is already an actual max value defined for these enums (but kept private on purpose) which is used by the lib/bbdev functions to restrict usage to what is actually supported in the given implementation (distinct from padded max value). 

Arguably the only concern I could understand in your message would be this one " my concern was if user assumes all values valid until PADDED_MAX and tries to iterate array until that value". 
But really the fact that it is indeed a padded value implies fairly explicitly that we have padded the supported enums with placeholders enums not yet defined. That is fairly tautological! I cannot see how it could confuse anyone. That is indeed to avoid such confusion that we went on that direction to expose a public future-proof padded maximum value. 

Then looking at usage in practice: when integrating the bbdev api with higher level SW stacks (such as FlexRAN reference sw or 3rd party stacks) I don’t see how any of this theoretical concerns you raised would be relevant for any of these very cases (enqueue status, new capability etc...). The only genuine concern was sizing array based on MAX value being not ABI compliant. 
I cannot think of any code in the application presently deployed or future that would then do what you are concerned about and cause an issue, and we definitely don’t do such things in any example for bbdev-test or in FlexRAN reference code provided to the ecosystem. The application would already have a default case when an enum being provided has no matching application, or more accurately in practice they would purely not look for these and hence these would be ignored seamlessly. 

Thanks again for the discussion. I wish this had happened earlier (we only discussed this with Ray and Bruce while you were still at Intel), let me know what you think.
It may be more generally good moving forward to come to a general agreement at your technical forum level to avoid confusion. When we discussed earlier we came to the conclusion that the DPDK community had well documented what not to do to avoid ABI breakage but not necessarily what are the best alternatives. 
Hopefully such future discussion should not delay this serie to be applied but still let me know. 

Thanks again

[1]
    * lib: will fix extending some enum/define breaking the ABI. There are multiple
      samples in DPDK that enum/define terminated with a ``.*MAX.*`` value which is
      used by iterators, and arrays holding these values are sized with this
      ``.*MAX.*`` value. So extending this enum/define increases the ``.*MAX.*``
      value which increases the size of the array and depending on how/where the
      array is used this may break the ABI.
     ``RTE_ETH_FLOW_MAX`` is one sample of the mentioned case, adding a new flow
      type will break the ABI because of ``flex_mask[RTE_ETH_FLOW_MAX]`` array
      usage in following public struct hierarchy:
      ``rte_eth_fdir_flex_conf -> rte_eth_fdir_conf -> rte_eth_conf (in the middle)``.
      Need to identify this kind of usages and fix in 20.11, otherwise this blocks
      us extending existing enum/define.
      One solution can be using a fixed size array instead of ``.*MAX.*`` value.


> -----Original Message-----
> From: Chautru, Nicolas
> Sent: Saturday, September 24, 2022 9:35 AM
> To: 'Akhil Goyal' <gakhil@marvell.com>; dev@dpdk.org; Maxime Coquelin
> <maxime.coquelin@redhat.com>; ferruh.yigit@xilinx.com; Ray Kinsella
> <mdr@ashroe.eu>
> Cc: 'Akhil Goyal' <gakhil@marvell.com>; Chautru, Nicolas
> <nicolas.chautru@intel.com>; 'thomas@monjalon.net'
> <thomas@monjalon.net>; 'Ray Kinsella' <mdr@ashroe.eu>;
> 'trix@redhat.com' <trix@redhat.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; 'david.marchand@redhat.com'
> <david.marchand@redhat.com>; stephen@networkplumber.org; Zhang,
> Mingshan <mingshan.zhang@intel.com>; 'hemant.agrawal@nxp.com'
> <hemant.agrawal@nxp.com>
> Subject: RE: [EXT] [PATCH v7 6/7] bbdev: add queue related warning and
> status information
> 
> 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);
> > >
> > > ---
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
> > >
  

On 9/27/2022 9:59 PM, Chautru, Nicolas wrote:
> CAUTION: This message has originated from an External Source. Please use proper judgment and caution when opening attachments, clicking links, or responding to this email.
> 
> 
> Hi Ferruh,
> 
> Thanks for your comment.
> To be totally honest I don't yet see how your suggestion would be better, but I quite possibly miss something. I did not reply in line with your comments so that to try to be clearer and avoid spreading the argument to much. Ray and Bruce feel free to chime in as well.
> 
> First to state the obvious: Nothing will change the fact that in case new enums are being added in DPDK, and if the application doesn't change, then user would not be able to interpret any such additional status/capability (backward compatible only feature parity and still ABI compliant) which is totally accepted as fine and up to the user, but the intention is at least not to have adverse effect even when they don’t update their code for such new features (notably in case they just use an older PMD not supporting such new features as a basic typical example in the ecosystem). I think we agree on that problematic.
> 
> In term of history of not using MAX value for enum, I believe there is already well documented and you agree with the reasoning of why we had to move away from this [1]. Not just cosmetically where the max value is called an enum or a #define but to have application making hardcoded assumption on the possible maximum range for such enum notably when sizing array. The only caveat being that at the time, the community did spot the weakness but did not come to an agreement with regards to the best way to manage this moving forward.
> 
> In case your point is purely cosmetic to rename the PADDED_MAX value from the enum to a #define (both public) I don't see how this would make thing clearer by obfuscating the fact it is genuinely a padded value and to have that value directly related to the enum structure. Also note that there is already an actual max value defined for these enums (but kept private on purpose) which is used by the lib/bbdev functions to restrict usage to what is actually supported in the given implementation (distinct from padded max value).
> 
> Arguably the only concern I could understand in your message would be this one " my concern was if user assumes all values valid until PADDED_MAX and tries to iterate array until that value".
> But really the fact that it is indeed a padded value implies fairly explicitly that we have padded the supported enums with placeholders enums not yet defined. That is fairly tautological! I cannot see how it could confuse anyone. That is indeed to avoid such confusion that we went on that direction to expose a public future-proof padded maximum value.
> 
> Then looking at usage in practice: when integrating the bbdev api with higher level SW stacks (such as FlexRAN reference sw or 3rd party stacks) I don’t see how any of this theoretical concerns you raised would be relevant for any of these very cases (enqueue status, new capability etc...). The only genuine concern was sizing array based on MAX value being not ABI compliant.
> I cannot think of any code in the application presently deployed or future that would then do what you are concerned about and cause an issue, and we definitely don’t do such things in any example for bbdev-test or in FlexRAN reference code provided to the ecosystem. The application would already have a default case when an enum being provided has no matching application, or more accurately in practice they would purely not look for these and hence these would be ignored seamlessly.
> 
> Thanks again for the discussion. I wish this had happened earlier (we only discussed this with Ray and Bruce while you were still at Intel), let me know what you think.
> It may be more generally good moving forward to come to a general agreement at your technical forum level to avoid confusion. When we discussed earlier we came to the conclusion that the DPDK community had well documented what not to do to avoid ABI breakage but not necessarily what are the best alternatives.
> Hopefully such future discussion should not delay this serie to be applied but still let me know.
> 

Hi Nic,

I believe it is more clear/safe to convert to SIZE_MAX macros, although 
it is not a blocker.

Anyway, I am not sure about the value of continuing this discussion, 
perhaps it is better to clarify the guidance for similar case with ABI 
maintainer and techboard, so it can proceed according to the decision.

Thanks,
ferruh

> Thanks again
> 
> [1]
>      * lib: will fix extending some enum/define breaking the ABI. There are multiple
>        samples in DPDK that enum/define terminated with a ``.*MAX.*`` value which is
>        used by iterators, and arrays holding these values are sized with this
>        ``.*MAX.*`` value. So extending this enum/define increases the ``.*MAX.*``
>        value which increases the size of the array and depending on how/where the
>        array is used this may break the ABI.
>       ``RTE_ETH_FLOW_MAX`` is one sample of the mentioned case, adding a new flow
>        type will break the ABI because of ``flex_mask[RTE_ETH_FLOW_MAX]`` array
>        usage in following public struct hierarchy:
>        ``rte_eth_fdir_flex_conf -> rte_eth_fdir_conf -> rte_eth_conf (in the middle)``.
>        Need to identify this kind of usages and fix in 20.11, otherwise this blocks
>        us extending existing enum/define.
>        One solution can be using a fixed size array instead of ``.*MAX.*`` value.
> 
> 
>> -----Original Message-----
>> From: Chautru, Nicolas
>> Sent: Saturday, September 24, 2022 9:35 AM
>> To: 'Akhil Goyal' <gakhil@marvell.com>; dev@dpdk.org; Maxime Coquelin
>> <maxime.coquelin@redhat.com>; ferruh.yigit@xilinx.com; Ray Kinsella
>> <mdr@ashroe.eu>
>> Cc: 'Akhil Goyal' <gakhil@marvell.com>; Chautru, Nicolas
>> <nicolas.chautru@intel.com>; 'thomas@monjalon.net'
>> <thomas@monjalon.net>; 'Ray Kinsella' <mdr@ashroe.eu>;
>> 'trix@redhat.com' <trix@redhat.com>; Richardson, Bruce
>> <bruce.richardson@intel.com>; 'david.marchand@redhat.com'
>> <david.marchand@redhat.com>; stephen@networkplumber.org; Zhang,
>> Mingshan <mingshan.zhang@intel.com>; 'hemant.agrawal@nxp.com'
>> <hemant.agrawal@nxp.com>
>> Subject: RE: [EXT] [PATCH v7 6/7] bbdev: add queue related warning and
>> status information
>>
>> 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);
>>>>
>>>> ---
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>>>>
>
  

> > Thanks for your comment.
> > To be totally honest I don't yet see how your suggestion would be better, but I
> quite possibly miss something. I did not reply in line with your comments so that
> to try to be clearer and avoid spreading the argument to much. Ray and Bruce
> feel free to chime in as well.
> >
> > First to state the obvious: Nothing will change the fact that in case new enums
> are being added in DPDK, and if the application doesn't change, then user would
> not be able to interpret any such additional status/capability (backward
> compatible only feature parity and still ABI compliant) which is totally accepted
> as fine and up to the user, but the intention is at least not to have adverse effect
> even when they don’t update their code for such new features (notably in case
> they just use an older PMD not supporting such new features as a basic typical
> example in the ecosystem). I think we agree on that problematic.
> >
> > In term of history of not using MAX value for enum, I believe there is already
> well documented and you agree with the reasoning of why we had to move
> away from this [1]. Not just cosmetically where the max value is called an enum
> or a #define but to have application making hardcoded assumption on the
> possible maximum range for such enum notably when sizing array. The only
> caveat being that at the time, the community did spot the weakness but did not
> come to an agreement with regards to the best way to manage this moving
> forward.
> >
> > In case your point is purely cosmetic to rename the PADDED_MAX value from
> the enum to a #define (both public) I don't see how this would make thing
> clearer by obfuscating the fact it is genuinely a padded value and to have that
> value directly related to the enum structure. Also note that there is already an
> actual max value defined for these enums (but kept private on purpose) which is
> used by the lib/bbdev functions to restrict usage to what is actually supported in
> the given implementation (distinct from padded max value).
> >
> > Arguably the only concern I could understand in your message would be this
> one " my concern was if user assumes all values valid until PADDED_MAX and
> tries to iterate array until that value".
> > But really the fact that it is indeed a padded value implies fairly explicitly that
> we have padded the supported enums with placeholders enums not yet defined.
> That is fairly tautological! I cannot see how it could confuse anyone. That is
> indeed to avoid such confusion that we went on that direction to expose a
> public future-proof padded maximum value.
> >
> > Then looking at usage in practice: when integrating the bbdev api with higher
> level SW stacks (such as FlexRAN reference sw or 3rd party stacks) I don’t see
> how any of this theoretical concerns you raised would be relevant for any of
> these very cases (enqueue status, new capability etc...). The only genuine
> concern was sizing array based on MAX value being not ABI compliant.
> > I cannot think of any code in the application presently deployed or future that
> would then do what you are concerned about and cause an issue, and we
> definitely don’t do such things in any example for bbdev-test or in FlexRAN
> reference code provided to the ecosystem. The application would already have
> a default case when an enum being provided has no matching application, or
> more accurately in practice they would purely not look for these and hence
> these would be ignored seamlessly.
> >
> > Thanks again for the discussion. I wish this had happened earlier (we only
> discussed this with Ray and Bruce while you were still at Intel), let me know what
> you think.
> > It may be more generally good moving forward to come to a general
> agreement at your technical forum level to avoid confusion. When we discussed
> earlier we came to the conclusion that the DPDK community had well
> documented what not to do to avoid ABI breakage but not necessarily what are
> the best alternatives.
> > Hopefully such future discussion should not delay this serie to be applied but
> still let me know.
> >
> 
> Hi Nic,
> 
> I believe it is more clear/safe to convert to SIZE_MAX macros, although
> it is not a blocker.
> 
> Anyway, I am not sure about the value of continuing this discussion,
> perhaps it is better to clarify the guidance for similar case with ABI
> maintainer and techboard, so it can proceed according to the decision.
> 
I agree with Ferruh's comment for converting to SIZE_MAX macros.
However, it is not a strong comment from my side.
Moving to techboard would mean this patchset would skip the RC1 window.
I believe as Ray is the maintainer and go to person for ABI related issues.
I believe if he can take a look at the suggestion and provide ack/nack to whichever
Approach would be fine and we can go ahead in that direction.
I would like to close this as soon as possible. There are a lot of patches to be blocked on this series.

Regards,
Akhil
  

Hi Thomas, 
In absence of Ray (I did not see email from him for a some time) can you please advise on best option so that as to move on.
I can either keep as is based on initial review with Ray, or replace _PADDED_MAX to _SIZE_MAX macro as suggested by Ferruh. 
I am happy either way as long as we are able to move forward. There is no full consensus but not strong opinion either from anyone. 
Thanks, 
Nic

> -----Original Message-----
> From: Akhil Goyal <gakhil@marvell.com>
> Sent: Thursday, September 29, 2022 11:33 AM
> To: Ferruh Yigit <ferruh.yigit@amd.com>; Chautru, Nicolas
> <nicolas.chautru@intel.com>; dev@dpdk.org; Maxime Coquelin
> <maxime.coquelin@redhat.com>; ferruh.yigit@xilinx.com; Ray Kinsella
> <mdr@ashroe.eu>
> Cc: thomas@monjalon.net; trix@redhat.com; Richardson, Bruce
> <bruce.richardson@intel.com>; david.marchand@redhat.com;
> stephen@networkplumber.org; Zhang, Mingshan
> <mingshan.zhang@intel.com>; hemant.agrawal@nxp.com
> Subject: RE: [EXT] [PATCH v7 6/7] bbdev: add queue related warning and status
> information
> 
> > > Thanks for your comment.
> > > To be totally honest I don't yet see how your suggestion would be
> > > better, but I
> > quite possibly miss something. I did not reply in line with your
> > comments so that to try to be clearer and avoid spreading the argument
> > to much. Ray and Bruce feel free to chime in as well.
> > >
> > > First to state the obvious: Nothing will change the fact that in
> > > case new enums
> > are being added in DPDK, and if the application doesn't change, then
> > user would not be able to interpret any such additional
> > status/capability (backward compatible only feature parity and still
> > ABI compliant) which is totally accepted as fine and up to the user,
> > but the intention is at least not to have adverse effect even when
> > they don’t update their code for such new features (notably in case
> > they just use an older PMD not supporting such new features as a basic
> typical example in the ecosystem). I think we agree on that problematic.
> > >
> > > In term of history of not using MAX value for enum, I believe there
> > > is already
> > well documented and you agree with the reasoning of why we had to move
> > away from this [1]. Not just cosmetically where the max value is
> > called an enum or a #define but to have application making hardcoded
> > assumption on the possible maximum range for such enum notably when
> > sizing array. The only caveat being that at the time, the community
> > did spot the weakness but did not come to an agreement with regards to
> > the best way to manage this moving forward.
> > >
> > > In case your point is purely cosmetic to rename the PADDED_MAX value
> > > from
> > the enum to a #define (both public) I don't see how this would make
> > thing clearer by obfuscating the fact it is genuinely a padded value
> > and to have that value directly related to the enum structure. Also
> > note that there is already an actual max value defined for these enums
> > (but kept private on purpose) which is used by the lib/bbdev functions
> > to restrict usage to what is actually supported in the given implementation
> (distinct from padded max value).
> > >
> > > Arguably the only concern I could understand in your message would
> > > be this
> > one " my concern was if user assumes all values valid until PADDED_MAX
> > and tries to iterate array until that value".
> > > But really the fact that it is indeed a padded value implies fairly
> > > explicitly that
> > we have padded the supported enums with placeholders enums not yet
> defined.
> > That is fairly tautological! I cannot see how it could confuse anyone.
> > That is indeed to avoid such confusion that we went on that direction
> > to expose a public future-proof padded maximum value.
> > >
> > > Then looking at usage in practice: when integrating the bbdev api
> > > with higher
> > level SW stacks (such as FlexRAN reference sw or 3rd party stacks) I
> > don’t see how any of this theoretical concerns you raised would be
> > relevant for any of these very cases (enqueue status, new capability
> > etc...). The only genuine concern was sizing array based on MAX value being
> not ABI compliant.
> > > I cannot think of any code in the application presently deployed or
> > > future that
> > would then do what you are concerned about and cause an issue, and we
> > definitely don’t do such things in any example for bbdev-test or in
> > FlexRAN reference code provided to the ecosystem. The application
> > would already have a default case when an enum being provided has no
> > matching application, or more accurately in practice they would purely
> > not look for these and hence these would be ignored seamlessly.
> > >
> > > Thanks again for the discussion. I wish this had happened earlier
> > > (we only
> > discussed this with Ray and Bruce while you were still at Intel), let
> > me know what you think.
> > > It may be more generally good moving forward to come to a general
> > agreement at your technical forum level to avoid confusion. When we
> > discussed earlier we came to the conclusion that the DPDK community
> > had well documented what not to do to avoid ABI breakage but not
> > necessarily what are the best alternatives.
> > > Hopefully such future discussion should not delay this serie to be
> > > applied but
> > still let me know.
> > >
> >
> > Hi Nic,
> >
> > I believe it is more clear/safe to convert to SIZE_MAX macros,
> > although it is not a blocker.
> >
> > Anyway, I am not sure about the value of continuing this discussion,
> > perhaps it is better to clarify the guidance for similar case with ABI
> > maintainer and techboard, so it can proceed according to the decision.
> >
> I agree with Ferruh's comment for converting to SIZE_MAX macros.
> However, it is not a strong comment from my side.
> Moving to techboard would mean this patchset would skip the RC1 window.
> I believe as Ray is the maintainer and go to person for ABI related issues.
> I believe if he can take a look at the suggestion and provide ack/nack to
> whichever Approach would be fine and we can go ahead in that direction.
> I would like to close this as soon as possible. There are a lot of patches to be
> blocked on this series.
> 
> Regards,
> Akhil
  

Hi Nic,

On 9/29/22 21:48, Chautru, Nicolas wrote:
> Hi Thomas,
> In absence of Ray (I did not see email from him for a some time) can you please advise on best option so that as to move on.
> I can either keep as is based on initial review with Ray, or replace _PADDED_MAX to _SIZE_MAX macro as suggested by Ferruh.
> I am happy either way as long as we are able to move forward. There is no full consensus but not strong opinion either from anyone.

I would go with Ferruh's suggestion.

Regards,
Maxime

> Thanks,
> Nic
> 
>> -----Original Message-----
>> From: Akhil Goyal <gakhil@marvell.com>
>> Sent: Thursday, September 29, 2022 11:33 AM
>> To: Ferruh Yigit <ferruh.yigit@amd.com>; Chautru, Nicolas
>> <nicolas.chautru@intel.com>; dev@dpdk.org; Maxime Coquelin
>> <maxime.coquelin@redhat.com>; ferruh.yigit@xilinx.com; Ray Kinsella
>> <mdr@ashroe.eu>
>> Cc: thomas@monjalon.net; trix@redhat.com; Richardson, Bruce
>> <bruce.richardson@intel.com>; david.marchand@redhat.com;
>> stephen@networkplumber.org; Zhang, Mingshan
>> <mingshan.zhang@intel.com>; hemant.agrawal@nxp.com
>> Subject: RE: [EXT] [PATCH v7 6/7] bbdev: add queue related warning and status
>> information
>>
>>>> Thanks for your comment.
>>>> To be totally honest I don't yet see how your suggestion would be
>>>> better, but I
>>> quite possibly miss something. I did not reply in line with your
>>> comments so that to try to be clearer and avoid spreading the argument
>>> to much. Ray and Bruce feel free to chime in as well.
>>>>
>>>> First to state the obvious: Nothing will change the fact that in
>>>> case new enums
>>> are being added in DPDK, and if the application doesn't change, then
>>> user would not be able to interpret any such additional
>>> status/capability (backward compatible only feature parity and still
>>> ABI compliant) which is totally accepted as fine and up to the user,
>>> but the intention is at least not to have adverse effect even when
>>> they don’t update their code for such new features (notably in case
>>> they just use an older PMD not supporting such new features as a basic
>> typical example in the ecosystem). I think we agree on that problematic.
>>>>
>>>> In term of history of not using MAX value for enum, I believe there
>>>> is already
>>> well documented and you agree with the reasoning of why we had to move
>>> away from this [1]. Not just cosmetically where the max value is
>>> called an enum or a #define but to have application making hardcoded
>>> assumption on the possible maximum range for such enum notably when
>>> sizing array. The only caveat being that at the time, the community
>>> did spot the weakness but did not come to an agreement with regards to
>>> the best way to manage this moving forward.
>>>>
>>>> In case your point is purely cosmetic to rename the PADDED_MAX value
>>>> from
>>> the enum to a #define (both public) I don't see how this would make
>>> thing clearer by obfuscating the fact it is genuinely a padded value
>>> and to have that value directly related to the enum structure. Also
>>> note that there is already an actual max value defined for these enums
>>> (but kept private on purpose) which is used by the lib/bbdev functions
>>> to restrict usage to what is actually supported in the given implementation
>> (distinct from padded max value).
>>>>
>>>> Arguably the only concern I could understand in your message would
>>>> be this
>>> one " my concern was if user assumes all values valid until PADDED_MAX
>>> and tries to iterate array until that value".
>>>> But really the fact that it is indeed a padded value implies fairly
>>>> explicitly that
>>> we have padded the supported enums with placeholders enums not yet
>> defined.
>>> That is fairly tautological! I cannot see how it could confuse anyone.
>>> That is indeed to avoid such confusion that we went on that direction
>>> to expose a public future-proof padded maximum value.
>>>>
>>>> Then looking at usage in practice: when integrating the bbdev api
>>>> with higher
>>> level SW stacks (such as FlexRAN reference sw or 3rd party stacks) I
>>> don’t see how any of this theoretical concerns you raised would be
>>> relevant for any of these very cases (enqueue status, new capability
>>> etc...). The only genuine concern was sizing array based on MAX value being
>> not ABI compliant.
>>>> I cannot think of any code in the application presently deployed or
>>>> future that
>>> would then do what you are concerned about and cause an issue, and we
>>> definitely don’t do such things in any example for bbdev-test or in
>>> FlexRAN reference code provided to the ecosystem. The application
>>> would already have a default case when an enum being provided has no
>>> matching application, or more accurately in practice they would purely
>>> not look for these and hence these would be ignored seamlessly.
>>>>
>>>> Thanks again for the discussion. I wish this had happened earlier
>>>> (we only
>>> discussed this with Ray and Bruce while you were still at Intel), let
>>> me know what you think.
>>>> It may be more generally good moving forward to come to a general
>>> agreement at your technical forum level to avoid confusion. When we
>>> discussed earlier we came to the conclusion that the DPDK community
>>> had well documented what not to do to avoid ABI breakage but not
>>> necessarily what are the best alternatives.
>>>> Hopefully such future discussion should not delay this serie to be
>>>> applied but
>>> still let me know.
>>>>
>>>
>>> Hi Nic,
>>>
>>> I believe it is more clear/safe to convert to SIZE_MAX macros,
>>> although it is not a blocker.
>>>
>>> Anyway, I am not sure about the value of continuing this discussion,
>>> perhaps it is better to clarify the guidance for similar case with ABI
>>> maintainer and techboard, so it can proceed according to the decision.
>>>
>> I agree with Ferruh's comment for converting to SIZE_MAX macros.
>> However, it is not a strong comment from my side.
>> Moving to techboard would mean this patchset would skip the RC1 window.
>> I believe as Ray is the maintainer and go to person for ABI related issues.
>> I believe if he can take a look at the suggestion and provide ack/nack to
>> whichever Approach would be fine and we can go ahead in that direction.
>> I would like to close this as soon as possible. There are a lot of patches to be
>> blocked on this series.
>>
>> Regards,
>> Akhil
>