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

[Akhil Goyal <akhil.goyal@nxp.com>]

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?

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.

> > > IMO, it seems an ABI breakage, but not sure. So wanted to double check.
> > > Now if it is an ABI breakage, then can we allow it? There was no deprecation
> > > notice Prior to this release.
> 
> Yes, there was no deprecation note in advance.
> Though I think the risk is minimal - size of the struct will remain unchanged (see
> above).
> My vote to let it in for 20.11.
> 
> > > Also I think if we are allowing the above change, then we should also add
> > > another Field for deq_cbs also for post crypto processing in this patch only.
> 
> +1 for this.
> I think it was already addressed in v5.
> 
> Konstantin