diff mbox series

[v4,3/7] bbdev: add device info on queue topology

Message ID 1657067022-54373-4-git-send-email-nicolas.chautru@intel.com (mailing list archive)
State Superseded, archived
Delegated to: akhil goyal
Headers
Series bbdev changes for 22.11 |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Chautru, Nicolas July 6, 2022, 12:23 a.m. UTC 
  Adding more options in the API to expose the number
of queues exposed and related priority.

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
---
 lib/bbdev/rte_bbdev.h | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Tom Rix July 6, 2022, 4:06 p.m. UTC | #1 
On 7/5/22 5:23 PM, Nicolas Chautru wrote:
> Adding more options in the API to expose the number
> of queues exposed and related priority.
>
> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> ---
>   lib/bbdev/rte_bbdev.h | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/lib/bbdev/rte_bbdev.h b/lib/bbdev/rte_bbdev.h
> index 9b1ffa4..ac941d6 100644
> --- a/lib/bbdev/rte_bbdev.h
> +++ b/lib/bbdev/rte_bbdev.h
> @@ -289,6 +289,10 @@ struct rte_bbdev_driver_info {
>   
>   	/** Maximum number of queues supported by the device */
>   	unsigned int max_num_queues;
> +	/** Maximum number of queues supported per operation type */
> +	unsigned int num_queues[RTE_BBDEV_OP_TYPE_PADDED_MAX];
> +	/** Priority level supported per operation type */
> +	unsigned int queue_priority[RTE_BBDEV_OP_TYPE_PADDED_MAX];

It is better to add new elements to the end of a structure for better 
backward compatiblity

Tom

>   	/** Queue size limit (queue size must also be power of 2) */
>   	uint32_t queue_size_lim;
>   	/** Set if device off-loads operation to hardware  */
Chautru, Nicolas July 6, 2022, 9:12 p.m. UTC | #2 
Hi Tom, 

> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Subject: Re: [PATCH v4 3/7] bbdev: add device info on queue topology
> 
> 
> On 7/5/22 5:23 PM, Nicolas Chautru wrote:
> > Adding more options in the API to expose the number of queues exposed
> > and related priority.
> >
> > Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> > ---
> >   lib/bbdev/rte_bbdev.h | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/lib/bbdev/rte_bbdev.h b/lib/bbdev/rte_bbdev.h index
> > 9b1ffa4..ac941d6 100644
> > --- a/lib/bbdev/rte_bbdev.h
> > +++ b/lib/bbdev/rte_bbdev.h
> > @@ -289,6 +289,10 @@ struct rte_bbdev_driver_info {
> >
> >   	/** Maximum number of queues supported by the device */
> >   	unsigned int max_num_queues;
> > +	/** Maximum number of queues supported per operation type */
> > +	unsigned int num_queues[RTE_BBDEV_OP_TYPE_PADDED_MAX];
> > +	/** Priority level supported per operation type */
> > +	unsigned int queue_priority[RTE_BBDEV_OP_TYPE_PADDED_MAX];
> 
> It is better to add new elements to the end of a structure for better backward
> compatibility

All that serie is not ABI compatible (sizes change etc...). I don’t believe there is such a recommendation, is there?

> 
> Tom
> 
> >   	/** Queue size limit (queue size must also be power of 2) */
> >   	uint32_t queue_size_lim;
> >   	/** Set if device off-loads operation to hardware  */
Tom Rix July 7, 2022, 1:34 p.m. UTC | #3 
On 7/6/22 2:12 PM, Chautru, Nicolas wrote:
> Hi Tom,
>
>> -----Original Message-----
>> From: Tom Rix <trix@redhat.com>
>> Subject: Re: [PATCH v4 3/7] bbdev: add device info on queue topology
>>
>>
>> On 7/5/22 5:23 PM, Nicolas Chautru wrote:
>>> Adding more options in the API to expose the number of queues exposed
>>> and related priority.
>>>
>>> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
>>> ---
>>>    lib/bbdev/rte_bbdev.h | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git a/lib/bbdev/rte_bbdev.h b/lib/bbdev/rte_bbdev.h index
>>> 9b1ffa4..ac941d6 100644
>>> --- a/lib/bbdev/rte_bbdev.h
>>> +++ b/lib/bbdev/rte_bbdev.h
>>> @@ -289,6 +289,10 @@ struct rte_bbdev_driver_info {
>>>
>>>    	/** Maximum number of queues supported by the device */
>>>    	unsigned int max_num_queues;
>>> +	/** Maximum number of queues supported per operation type */
>>> +	unsigned int num_queues[RTE_BBDEV_OP_TYPE_PADDED_MAX];
>>> +	/** Priority level supported per operation type */
>>> +	unsigned int queue_priority[RTE_BBDEV_OP_TYPE_PADDED_MAX];
>> It is better to add new elements to the end of a structure for better backward
>> compatibility
> All that serie is not ABI compatible (sizes change etc...). I don’t believe there is such a recommendation, is there?

Depends on what users expect, a dynamically linked old application would 
at best core here.  If the elements were added to the end, yes the size 
would change but the old dynamically linked application would not use 
them.  Dynamically linking is nice because problems in the library can 
be fixed and shipped without forcing the user recompile.  Though the 
user may not realize  it, this change forces them to recompile.

Tom

>
>> Tom
>>
>>>    	/** Queue size limit (queue size must also be power of 2) */
>>>    	uint32_t queue_size_lim;
>>>    	/** Set if device off-loads operation to hardware  */
Chautru, Nicolas July 7, 2022, 5:13 p.m. UTC | #4 
Hi Tom, 

> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Sent: Thursday, July 7, 2022 6:34 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com
> Cc: maxime.coquelin@redhat.com; mdr@ashroe.eu; Richardson, Bruce
> <bruce.richardson@intel.com>; david.marchand@redhat.com;
> stephen@networkplumber.org
> Subject: Re: [PATCH v4 3/7] bbdev: add device info on queue topology
> 
> 
> On 7/6/22 2:12 PM, Chautru, Nicolas wrote:
> > Hi Tom,
> >
> >> -----Original Message-----
> >> From: Tom Rix <trix@redhat.com>
> >> Subject: Re: [PATCH v4 3/7] bbdev: add device info on queue topology
> >>
> >>
> >> On 7/5/22 5:23 PM, Nicolas Chautru wrote:
> >>> Adding more options in the API to expose the number of queues
> >>> exposed and related priority.
> >>>
> >>> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> >>> ---
> >>>    lib/bbdev/rte_bbdev.h | 4 ++++
> >>>    1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/lib/bbdev/rte_bbdev.h b/lib/bbdev/rte_bbdev.h index
> >>> 9b1ffa4..ac941d6 100644
> >>> --- a/lib/bbdev/rte_bbdev.h
> >>> +++ b/lib/bbdev/rte_bbdev.h
> >>> @@ -289,6 +289,10 @@ struct rte_bbdev_driver_info {
> >>>
> >>>    	/** Maximum number of queues supported by the device */
> >>>    	unsigned int max_num_queues;
> >>> +	/** Maximum number of queues supported per operation type */
> >>> +	unsigned int num_queues[RTE_BBDEV_OP_TYPE_PADDED_MAX];
> >>> +	/** Priority level supported per operation type */
> >>> +	unsigned int queue_priority[RTE_BBDEV_OP_TYPE_PADDED_MAX];
> >> It is better to add new elements to the end of a structure for better
> >> backward compatibility
> > All that serie is not ABI compatible (sizes change etc...). I don’t believe there
> is such a recommendation, is there?
> 
> Depends on what users expect, a dynamically linked old application would at
> best core here.  If the elements were added to the end, yes the size would
> change but the old dynamically linked application would not use
> them.  Dynamically linking is nice because problems in the library can be fixed
> and shipped without forcing the user recompile.  Though the user may not
> realize  it, this change forces them to recompile.
> 
> Tom

Thanks Tom. In that very context, the change are big enough not to have any form of compatibility. This a new ABI version, and user knows they will have to recompile. 
Still it would be great to capture a recommendation in DPDK coding guideline in case there is such a BKM, I have heard multiple arguments for different preference, if we want to harmonize such things let's capture in coding guide lines, it would not hurt. Maybe one for Thomas?

> 
> >
> >> Tom
> >>
> >>>    	/** Queue size limit (queue size must also be power of 2) */
> >>>    	uint32_t queue_size_lim;
> >>>    	/** Set if device off-loads operation to hardware  */
Tom Rix July 18, 2022, 1:04 p.m. UTC | #5 
On 7/7/22 10:13 AM, Chautru, Nicolas wrote:
> Hi Tom,
>
>> -----Original Message-----
>> From: Tom Rix <trix@redhat.com>
>> Sent: Thursday, July 7, 2022 6:34 AM
>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
>> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com
>> Cc: maxime.coquelin@redhat.com; mdr@ashroe.eu; Richardson, Bruce
>> <bruce.richardson@intel.com>; david.marchand@redhat.com;
>> stephen@networkplumber.org
>> Subject: Re: [PATCH v4 3/7] bbdev: add device info on queue topology
>>
>>
>> On 7/6/22 2:12 PM, Chautru, Nicolas wrote:
>>> Hi Tom,
>>>
>>>> -----Original Message-----
>>>> From: Tom Rix <trix@redhat.com>
>>>> Subject: Re: [PATCH v4 3/7] bbdev: add device info on queue topology
>>>>
>>>>
>>>> On 7/5/22 5:23 PM, Nicolas Chautru wrote:
>>>>> Adding more options in the API to expose the number of queues
>>>>> exposed and related priority.
>>>>>
>>>>> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
>>>>> ---
>>>>>     lib/bbdev/rte_bbdev.h | 4 ++++
>>>>>     1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/lib/bbdev/rte_bbdev.h b/lib/bbdev/rte_bbdev.h index
>>>>> 9b1ffa4..ac941d6 100644
>>>>> --- a/lib/bbdev/rte_bbdev.h
>>>>> +++ b/lib/bbdev/rte_bbdev.h
>>>>> @@ -289,6 +289,10 @@ struct rte_bbdev_driver_info {
>>>>>
>>>>>     	/** Maximum number of queues supported by the device */
>>>>>     	unsigned int max_num_queues;
>>>>> +	/** Maximum number of queues supported per operation type */
>>>>> +	unsigned int num_queues[RTE_BBDEV_OP_TYPE_PADDED_MAX];
>>>>> +	/** Priority level supported per operation type */
>>>>> +	unsigned int queue_priority[RTE_BBDEV_OP_TYPE_PADDED_MAX];
>>>> It is better to add new elements to the end of a structure for better
>>>> backward compatibility
>>> All that serie is not ABI compatible (sizes change etc...). I don’t believe there
>> is such a recommendation, is there?
>>
>> Depends on what users expect, a dynamically linked old application would at
>> best core here.  If the elements were added to the end, yes the size would
>> change but the old dynamically linked application would not use
>> them.  Dynamically linking is nice because problems in the library can be fixed
>> and shipped without forcing the user recompile.  Though the user may not
>> realize  it, this change forces them to recompile.
>>
>> Tom
> Thanks Tom. In that very context, the change are big enough not to have any form of compatibility. This a new ABI version, and user knows they will have to recompile.
> Still it would be great to capture a recommendation in DPDK coding guideline in case there is such a BKM, I have heard multiple arguments for different preference, if we want to harmonize such things let's capture in coding guide lines, it would not hurt. Maybe one for Thomas?

When sw is deployed, how would a user know ?

For that matter, how would a developer know without a deep reading of 
header files ?

I am not asking for a compatibility testsuite here, just the placement 
of new elements (the same code) at the end of structures.  As a library 
writer, please consider the users of the library.  Your improvements are 
amplified by all of the library's users.  The user's code quality is 
based on this library's code quality.

My expectation is a new ABI introduces new functionality without 
breaking old binaries. Or if it does, it is for a good reason.

There is no good reason for putting new elements into the middle of an 
existing structure.

Tom

>
>>>> Tom
>>>>
>>>>>     	/** Queue size limit (queue size must also be power of 2) */
>>>>>     	uint32_t queue_size_lim;
>>>>>     	/** Set if device off-loads operation to hardware  */
diff mbox series

Patch

diff --git a/lib/bbdev/rte_bbdev.h b/lib/bbdev/rte_bbdev.h
index 9b1ffa4..ac941d6 100644
--- a/lib/bbdev/rte_bbdev.h
+++ b/lib/bbdev/rte_bbdev.h
@@ -289,6 +289,10 @@  struct rte_bbdev_driver_info {
 
 	/** Maximum number of queues supported by the device */
 	unsigned int max_num_queues;
+	/** Maximum number of queues supported per operation type */
+	unsigned int num_queues[RTE_BBDEV_OP_TYPE_PADDED_MAX];
+	/** Priority level supported per operation type */
+	unsigned int queue_priority[RTE_BBDEV_OP_TYPE_PADDED_MAX];
 	/** Queue size limit (queue size must also be power of 2) */
 	uint32_t queue_size_lim;
 	/** Set if device off-loads operation to hardware  */