[dpdk-dev] [dpdk-techboard] [v4 1/3] cryptodev: support enqueue callback functions

[Kinsella, Ray]

On 28/10/2020 15:22, Honnappa Nagarahalli wrote:
> + Ray for ABI
> 
> <snip>
> 
>>
>> On Wed, Oct 28, 2020 at 02:28:43PM +0000, Akhil Goyal wrote:
>>>
>>> Hi Konstantin,
>>>
>>>>>> Hi Tech board members,
>>>>>>
>>>>>> I have a doubt about the ABI breakage in below addition of field.
>>>>>> Could you please comment.
>>>>>>
>>>>>>>  /** The data structure associated with each crypto device. */
>>>>>>> struct rte_cryptodev {
>>>>>>>  	dequeue_pkt_burst_t dequeue_burst; @@ -867,6 +922,10
>> @@
>>>>>>> struct rte_cryptodev {
>>>>>>>  	__extension__
>>>>>>>  	uint8_t attached : 1;
>>>>>>>  	/**< Flag indicating the device is attached */
>>>>>>> +
>>>>>>> +	struct rte_cryptodev_enq_cb_rcu *enq_cbs;
>>>>>>> +	/**< User application callback for pre enqueue processing */
>>>>>>> +
>>>>>>>  } __rte_cache_aligned;
>>>>>>
>>>>>> Here rte_cryptodevs is defined in stable API list in map file
>>>>>> which is a pointer To all rte_cryptodev and the above change is
>>>>>> changing the size of the
>>>> structure.
>>>>
>>>> While this patch adds new fields into rte_cryptodev structure, it
>>>> doesn't change the size of it.
>>>> struct rte_cryptodev is cache line aligned, so it's current size:
>>>> 128B for 64-bit systems, and 64B(/128B) for 32-bit systems.
>>>> So for 64-bit we have 47B implicitly reserved, and for 32-bit we
>>>> have 19B reserved.
>>>> That's enough to add two pointers without changing size of this struct.
>>>>
>>>
>>> The structure is cache aligned, and if the cache line size in 32Byte
>>> and the compilation is done on 64bit machine, then we will be left
>>> with 15Bytes which is not sufficient for 2 pointers.
>>> Do we have such systems? Am I missing something?
>>>
>>
>> I don't think we support any such systems, so unless someone can point out
>> a specific case where we need to support 32-byte CLs, I'd tend towards
>> ignoring this as a non-issue.
> Agree. I have not come across 32B cache line.
> 
>>
>>> The reason I brought this into techboard is to have a consensus on
>>> such change As rte_cryptodev is a very popular and stable structure.
>>> Any changes to it may Have impacts which one person cannot judge all use
>> cases.
>>>
>>
>> Haven't been tracking this discussion much, but from what I read here, this
>> doesn't look like an ABI break and should be ok.
> If we are filling the holes in the cache line with new fields, it should not be an ABI break.

Agreed, risk seems minimal  ... it is an ABI Breakage window in anycase.
 
>>
>> Regards,
>> /Bruce