[PATCH v1] bbdev: allow operation type enum for growth

[PATCH v1] bbdev: allow operation type enum for growth

[Nicolas Chautru]

Hi Thomas, 

I would like to get your view on this topic and best recommendation moving forward.

This is related to the general intent to remove using MAX value for enums. There is consensus that we should avoid this for a while notably for future-proofed ABI concerns https://patches.dpdk.org/project/dpdk/patch/20200130142003.2645765-1-ferruh.yigit@intel.com/.
But still there is arguably not yet an explicit best recommendation to handle this especially when we actualy need to expose array whose index is such an enum.
As a specific example here I am refering to RTE_BBDEV_OP_TYPE_COUNT in enum rte_bbdev_op_type which is being extended for new operation type being support in bbdev (such as https://patches.dpdk.org/project/dpdk/patch/1646956157-245769-2-git-send-email-nicolas.chautru@intel.com/ adding new FFT operation)

There is also the intent to be able to expose information for each operation type through the bbdev api such as dynamically configured queues information per such operation type https://patches.dpdk.org/project/dpdk/patch/1646785355-168133-2-git-send-email-nicolas.chautru@intel.com/

Basically we are considering best way to accomodate for this, notably based on discussions with Ray Kinsella and Bruce Richardson, to handle such a case moving forward: specifically for the example with RTE_BBDEV_OP_TYPE_COUNT and also more generally.

One possible option is captured in that patchset and is basically based on the simple principle to allow for growth and prevent ABI breakage. Ie. the last value of the enum is set with a higher value than required so that to allow insertion of new enum outside of the major ABI versions.
In that case the RTE_BBDEV_OP_TYPE_COUNT is still present and can be exposed and used while still allowing for addition thanks to the implicit padding-like room. As an alternate variant, instead of using that last enum value, that extended size could be exposed as an #define outside of the enum but would be fundamentally the same (public).

Another option would be to avoid array alltogether and use each time this a new dedicated API function (operation type enum being an input argument instead of an index to an array in an existing structure so that to get access to structure related to a given operation type enum) but that is arguably not well scalable within DPDK to use such a scheme for each enums and keep an uncluttered and clean API. In that very example that would be very odd indeed not to get this simply from info_get().

Some pros and cons, arguably the simple option in that patchset is a valid compromise option and a step in the right direction but we would like to know your view wrt best recommendation, or any other thought. 

Note: Such a change is aimed for 22.11.

Thanks and regards, 
Nic


Nicolas Chautru (1):
  bbdev: allow operation type enum for growth

 lib/bbdev/rte_bbdev.c    | 5 ++++-
 lib/bbdev/rte_bbdev_op.h | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

-- 
1.8.3.1

[PATCH v1] bbdev: allow operation type enum for growth

[Nicolas Chautru]

Updating the last enum for rte_bbdev_op_type
to allow for enum insertion.

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
---
 lib/bbdev/rte_bbdev.c    | 5 ++++-
 lib/bbdev/rte_bbdev_op.h | 2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/lib/bbdev/rte_bbdev.c b/lib/bbdev/rte_bbdev.c
index aaee7b7..6003118 100644
--- a/lib/bbdev/rte_bbdev.c
+++ b/lib/bbdev/rte_bbdev.c
@@ -1122,7 +1122,10 @@ struct rte_mempool *
 		"RTE_BBDEV_OP_TURBO_DEC",
 		"RTE_BBDEV_OP_TURBO_ENC",
 		"RTE_BBDEV_OP_LDPC_DEC",
-		"RTE_BBDEV_OP_LDPC_ENC",
+		"RTE_BBDEV_OP_RESERVED_1",
+		"RTE_BBDEV_OP_RESERVED_2",
+		"RTE_BBDEV_OP_RESERVED_3",
+		"RTE_BBDEV_OP_RESERVED_4",
 	};
 
 	if (op_type < RTE_BBDEV_OP_TYPE_COUNT)
diff --git a/lib/bbdev/rte_bbdev_op.h b/lib/bbdev/rte_bbdev_op.h
index 6d56133..8d66007 100644
--- a/lib/bbdev/rte_bbdev_op.h
+++ b/lib/bbdev/rte_bbdev_op.h
@@ -748,7 +748,7 @@ enum rte_bbdev_op_type {
 	RTE_BBDEV_OP_TURBO_ENC,  /**< Turbo encode */
 	RTE_BBDEV_OP_LDPC_DEC,  /**< LDPC decode */
 	RTE_BBDEV_OP_LDPC_ENC,  /**< LDPC encode */
-	RTE_BBDEV_OP_TYPE_COUNT,  /**< Count of different op types */
+	RTE_BBDEV_OP_TYPE_COUNT = 8,  /**< Count of different op types */
 };
 
 /** Bit indexes of possible errors reported through status field */
-- 
1.8.3.1

Re: [PATCH v1] bbdev: allow operation type enum for growth

[Stephen Hemminger]

On Mon, 13 Jun 2022 11:24:35 -0700
Nicolas Chautru <nicolas.chautru@intel.com> wrote:

> Updating the last enum for rte_bbdev_op_type
> to allow for enum insertion.
> 
> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>

Only allowed if you check now for the any of the reserved types
and fail.

RE: [PATCH v1] bbdev: allow operation type enum for growth

[Chautru, Nicolas]

Hi Stephen, 

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Monday, June 13, 2022 12:48 PM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>
> Cc: dev@dpdk.org; thomas@monjalon.net; maxime.coquelin@redhat.com;
> trix@redhat.com; mdr@ashroe.eu; Richardson, Bruce
> <bruce.richardson@intel.com>; hemant.agrawal@nxp.com;
> david.marchand@redhat.com
> Subject: Re: [PATCH v1] bbdev: allow operation type enum for growth
> 
> On Mon, 13 Jun 2022 11:24:35 -0700
> Nicolas Chautru <nicolas.chautru@intel.com> wrote:
> 
> > Updating the last enum for rte_bbdev_op_type to allow for enum
> > insertion.
> >
> > Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> 
> Only allowed if you check now for the any of the reserved types and fail.

Let me try to clarify what you mean. You would enforce such check in which function? Do you mean in any implementation of bbdev function taking enum rte_bbdev_op_type as argument?
In that case do you mean having in effect 2 values: 
- one for the number of supported implemented operation type = could be kept private within rte_bbdev.c implementation, purely to reject using a non-supported operation type
- and another higher value for padded maximum numbers of operations allowing for enumeration insertion.
That would sound fine to me, but please kindly confirm this is what you are implying. 
I will also check for Thomas feedback as well, thanks. 

Re: [PATCH v1] bbdev: allow operation type enum for growth

[Thomas Monjalon]

This solution is what I proposed to the techboard some years ago,
but the preference was to completely remove the MAX values.


13/06/2022 20:24, Nicolas Chautru:
> Updating the last enum for rte_bbdev_op_type
> to allow for enum insertion.

Please explain that the reason is to keep ABI compatible,
and you want to keep the MAX value for array needs.

> --- a/lib/bbdev/rte_bbdev.c
> +++ b/lib/bbdev/rte_bbdev.c
> @@ -1122,7 +1122,10 @@ struct rte_mempool *
>  		"RTE_BBDEV_OP_TURBO_DEC",
>  		"RTE_BBDEV_OP_TURBO_ENC",
>  		"RTE_BBDEV_OP_LDPC_DEC",
> -		"RTE_BBDEV_OP_LDPC_ENC",
> +		"RTE_BBDEV_OP_RESERVED_1",
> +		"RTE_BBDEV_OP_RESERVED_2",
> +		"RTE_BBDEV_OP_RESERVED_3",
> +		"RTE_BBDEV_OP_RESERVED_4",

As Stephen said, you should make sure that using these values
with the API functions will lead to a clear and expected error.

> @@ -748,7 +748,7 @@ enum rte_bbdev_op_type {
>  	RTE_BBDEV_OP_TURBO_ENC,  /**< Turbo encode */
>  	RTE_BBDEV_OP_LDPC_DEC,  /**< LDPC decode */
>  	RTE_BBDEV_OP_LDPC_ENC,  /**< LDPC encode */
> -	RTE_BBDEV_OP_TYPE_COUNT,  /**< Count of different op types */
> +	RTE_BBDEV_OP_TYPE_COUNT = 8,  /**< Count of different op types */

You must update the comment to explain there may be a padding,
it is not exactly the count.
Maybe "MAX" is a better fit than "COUNT" in this case.

RE: [PATCH v1] bbdev: allow operation type enum for growth

[Chautru, Nicolas]

> From: Thomas Monjalon <thomas@monjalon.net>
> 
> This solution is what I proposed to the techboard some years ago, but the
> preference was to completely remove the MAX values.
> 

Thanks, good to see you had similar thought! I don't believe there is an actual recommendation captured in term of how to remove completely MAX values in that case below. I believe that this option is still compatible with the spirit of keeping AB future proof. 

> 
> 13/06/2022 20:24, Nicolas Chautru:
> > Updating the last enum for rte_bbdev_op_type to allow for enum
> > insertion.
> 
> Please explain that the reason is to keep ABI compatible, and you want to keep
> the MAX value for array needs.
> 
> > --- a/lib/bbdev/rte_bbdev.c
> > +++ b/lib/bbdev/rte_bbdev.c
> > @@ -1122,7 +1122,10 @@ struct rte_mempool *
> >  		"RTE_BBDEV_OP_TURBO_DEC",
> >  		"RTE_BBDEV_OP_TURBO_ENC",
> >  		"RTE_BBDEV_OP_LDPC_DEC",
> > -		"RTE_BBDEV_OP_LDPC_ENC",
> > +		"RTE_BBDEV_OP_RESERVED_1",
> > +		"RTE_BBDEV_OP_RESERVED_2",
> > +		"RTE_BBDEV_OP_RESERVED_3",
> > +		"RTE_BBDEV_OP_RESERVED_4",
> 
> As Stephen said, you should make sure that using these values with the API
> functions will lead to a clear and expected error.

Yes will do this. 

> 
> > @@ -748,7 +748,7 @@ enum rte_bbdev_op_type {
> >  	RTE_BBDEV_OP_TURBO_ENC,  /**< Turbo encode */
> >  	RTE_BBDEV_OP_LDPC_DEC,  /**< LDPC decode */
> >  	RTE_BBDEV_OP_LDPC_ENC,  /**< LDPC encode */
> > -	RTE_BBDEV_OP_TYPE_COUNT,  /**< Count of different op types */
> > +	RTE_BBDEV_OP_TYPE_COUNT = 8,  /**< Count of different op types */
> 
> You must update the comment to explain there may be a padding, it is not
> exactly the count.
> Maybe "MAX" is a better fit than "COUNT" in this case.
> 

OK

Re: [PATCH v1] bbdev: allow operation type enum for growth

[Ray Kinsella]

Thomas Monjalon <thomas@monjalon.net> writes:

> This solution is what I proposed to the techboard some years ago,
> but the preference was to completely remove the MAX values.


I am mindful we don't have an explicit guidance in the documentation.
I am including to add something to the abi_versioning document, that
basically says

1. Try not to use _MAX values with enumerations.
2. If you have to use _MAX values with enumeration, consider giving
yourself some headroom along with rigourous checking?

>
>
> 13/06/2022 20:24, Nicolas Chautru:
>> Updating the last enum for rte_bbdev_op_type
>> to allow for enum insertion.
>
> Please explain that the reason is to keep ABI compatible,
> and you want to keep the MAX value for array needs.
>
>> --- a/lib/bbdev/rte_bbdev.c
>> +++ b/lib/bbdev/rte_bbdev.c
>> @@ -1122,7 +1122,10 @@ struct rte_mempool *
>>  		"RTE_BBDEV_OP_TURBO_DEC",
>>  		"RTE_BBDEV_OP_TURBO_ENC",
>>  		"RTE_BBDEV_OP_LDPC_DEC",
>> -		"RTE_BBDEV_OP_LDPC_ENC",
>> +		"RTE_BBDEV_OP_RESERVED_1",
>> +		"RTE_BBDEV_OP_RESERVED_2",
>> +		"RTE_BBDEV_OP_RESERVED_3",
>> +		"RTE_BBDEV_OP_RESERVED_4",
>
> As Stephen said, you should make sure that using these values
> with the API functions will lead to a clear and expected error.
>
>> @@ -748,7 +748,7 @@ enum rte_bbdev_op_type {
>>  	RTE_BBDEV_OP_TURBO_ENC,  /**< Turbo encode */
>>  	RTE_BBDEV_OP_LDPC_DEC,  /**< LDPC decode */
>>  	RTE_BBDEV_OP_LDPC_ENC,  /**< LDPC encode */
>> -	RTE_BBDEV_OP_TYPE_COUNT,  /**< Count of different op types */
>> +	RTE_BBDEV_OP_TYPE_COUNT = 8,  /**< Count of different op types */
>
> You must update the comment to explain there may be a padding,
> it is not exactly the count.
> Maybe "MAX" is a better fit than "COUNT" in this case.


-- 
Regards, Ray K