Re: [dpdk-dev] [RFC] cryptodev: proposed changes in rte_cryptodev_sym_session

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

Hi everyone,

> This RFC for proposes several changes inside rte_cryptodev_sym_session.
> Note that this is just RFC not a complete patch, so for now
> I modified only the librte_cryptodev itself,
> some cryptodev PMD, test-crypto-perf and ipsec-secgw example.
> Proposed changes means ABI/API breakage inside cryptodev,
> so looking for feedback from crypto-dev lib and crypto-PMD maintainiers.
> Below are details and reasoning for proposed changes.
> 
> 1.rte_cryptodev_sym_session_init()/ rte_cryptodev_sym_session_clear()
>   operate based on cytpodev device id, though inside
>   rte_cryptodev_sym_session device specific data is addressed
>   by driver id (not device id).
>   That creates a problem with current implementation when we have
>   two or more devices with the same driver used by the same session.
>   Consider the following example:
> 
>   struct rte_cryptodev_sym_session *sess;
>   rte_cryptodev_sym_session_init(dev_id=X, sess, ...);
>   rte_cryptodev_sym_session_init(dev_id=Y, sess, ...);
>   rte_cryptodev_sym_session_clear(dev_id=X, sess);
> 
>   After that point if X and Y uses the same driver,
>   then sess can't be used by device Y any more.
>   The reason for that - driver specific (not device specific)
>   data per session, plus there is no information
>   how many device instances use that data.
>   Probably the simplest way to deal with that issue -
>   add a reference counter per each driver data.
> 
> 2.rte_cryptodev_sym_session_set_user_data() and
>   rte_cryptodev_sym_session_get_user_data() -
>   with current implementation there is no defined way for the user to
>   determine what is the max allowed size of the private data.
>   Even within rte_cryptodev_sym_session_set_user_data() we just blindly
>   copying user provided data without checking memory boundaries violation.
>   To overcome that issue I added 'uint16_t priv_size' into
>   rte_cryptodev_sym_session structure.
> 
> 3.rte_cryptodev_sym_session contains an array of variable size for
>   driver specific data.
>   Though number of elements in that array is determined by static
>   variable nb_drivers, that could be modified by
>   rte_cryptodev_allocate_driver().
>   That construction seems to work ok so far, as right now users register
>   all their PMDs at startup, though it doesn't mean that it would always
>   remain like that.
>   To make it less error prone I added 'uint16_t nb_drivers' into the
>   rte_cryptodev_sym_session structure.
>   At least that allows related functions to check that provided
>   driver id wouldn't overrun variable array boundaries,
>   again it allows to determine size of already allocated session
>   without accessing global variable.
> 
> 4.#2 and #3 above implies that now each struct rte_cryptodev_sym_session
>   would have sort of readonly type data (init once at allocation time,
>   keep unmodified through session life-time).
>   That requires more changes in current cryptodev implementation:
>   Right now inside cryptodev framework both rte_cryptodev_sym_session
>   and driver specific session data are two completely different sctrucures
>   (e.g. struct struct null_crypto_session and struct null_crypto_session).
>   Though current cryptodev implementation implicitly assumes that driver
>   will allocate both of them from within the same mempool.
>   Plus this is done in a manner that they override each other fields
>   (reuse the same space - sort of implicit C union).
>   That's probably not the best programming practice,
>   plus make impossible to have readonly fields inside both of them.
>   So to overcome that situation I changed an API a bit, to allow
>   to use two different mempools for these two distinct data structures.
> 
>  5. Add 'uint64_t userdata' inside struct rte_cryptodev_sym_session.
>    I suppose that self-explanatory, and might be used in a lot of places
>    (would be quite useful for ipsec library we develop).
> 
> So the new proposed layout for rte_cryptodev_sym_session:
> struct rte_cryptodev_sym_session {
>         uint64_t userdata;
>         /**< Can be used for external metadata */
>         uint16_t nb_drivers;
>         /**< number of elements in sess_data array */
>         uint16_t priv_size;
>         /**< session private data will be placed after sess_data */
>         __extension__ struct {
>                 void *data;
>                 uint16_t refcnt;
>         } sess_data[0];
>         /**< Driver specific session material, variable size */
> };
> 
> 
> Signed-off-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---

Ok, didn't hear any objections, so far,
so I suppose everyone are ok in general with proposed changes.
Will go ahead with deprecation notice for 18.11 then.
Konstantin