[dpdk-dev] [PATCH] doc: cryptodev deprecation notice for sym session changes

[dpdk-dev] [PATCH] doc: cryptodev deprecation notice for sym session changes

[Konstantin Ananyev]

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.
  rte_cryptodev_sym_session_set_user_data() just blindly copies
  user provided data without checking memory boundaries violation.
  To overcome that issue propose to add '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 propose to add '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 cryptodev_sym_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.
  To overcome that situation propose to 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).

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>
---
 doc/guides/rel_notes/deprecation.rst | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index d2aec64d1..998a0d92c 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -74,3 +74,12 @@ Deprecation Notices
 
   This is due to a lack of flexibility and reliance on a type unusable with
   C++ programs (struct rte_flow_desc).
+
+* cryptodev: several API and ABI changes are planned for rte_cryptodev
+  in v19.02:
+
+  - The size and layout of ``rte_cryptodev_sym_session`` will change
+    to fix existing issues.
+  - The size and layout of ``rte_cryptodev_qp_conf`` and syntax of
+    ``rte_cryptodev_queue_pair_setup`` will change to to allow to use
+    two different mempools for crypto and device private sessions.
-- 
2.13.6

Re: [dpdk-dev] [PATCH] doc: cryptodev deprecation notice for sym session changes

[Zhang, Roy Fan]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Konstantin
> Ananyev
> Sent: Thursday, October 11, 2018 3:20 PM
> To: dev@dpdk.org
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Subject: [dpdk-dev] [PATCH] doc: cryptodev deprecation notice for sym
> session changes
> 

Acked-by: Fan Zhang <roy.fan.zhang@intel.com>

Re: [dpdk-dev] [PATCH] doc: cryptodev deprecation notice for sym session changes

[Akhil Goyal]

On 10/11/2018 7:50 PM, Konstantin Ananyev wrote:
> 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.
>    rte_cryptodev_sym_session_set_user_data() just blindly copies
>    user provided data without checking memory boundaries violation.
>    To overcome that issue propose to add '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 propose to add '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 cryptodev_sym_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.
>    To overcome that situation propose to 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).
>
> 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>

Adding maintainers to ack this deprecation notice. These changes will 
impact all the PMDs and everyone
should agree to these changes.

from NXP dpaa_sec, dpaa2_sec, caam_jr PMDs:

Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
> ---
>   doc/guides/rel_notes/deprecation.rst | 9 +++++++++
>   1 file changed, 9 insertions(+)
>
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index d2aec64d1..998a0d92c 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -74,3 +74,12 @@ Deprecation Notices
>   
>     This is due to a lack of flexibility and reliance on a type unusable with
>     C++ programs (struct rte_flow_desc).
> +
> +* cryptodev: several API and ABI changes are planned for rte_cryptodev
> +  in v19.02:
> +
> +  - The size and layout of ``rte_cryptodev_sym_session`` will change
> +    to fix existing issues.
> +  - The size and layout of ``rte_cryptodev_qp_conf`` and syntax of
> +    ``rte_cryptodev_queue_pair_setup`` will change to to allow to use
> +    two different mempools for crypto and device private sessions.

Re: [dpdk-dev] [PATCH] doc: cryptodev deprecation notice for sym session changes

[Trahe, Fiona]

> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> Sent: Monday, November 12, 2018 5:04 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org; Ravi Kumar
> <ravi1.kumar@amd.com>; Jerin Jacob <jerin.jacob@caviumnetworks.com>; Anoob Joseph
> <anoob.joseph@caviumnetworks.com>; Doherty, Declan <declan.doherty@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; Tomasz Duszynski <tdu@semihalf.com>; Dmitri Epshtein <dima@marvell.com>;
> Natalie Samsonov <nsamsono@marvell.com>; Jay Zhou <jianjay.zhou@huawei.com>
> Subject: Re: [dpdk-dev] [PATCH] doc: cryptodev deprecation notice for sym session changes
> 
> 
> 
> On 10/11/2018 7:50 PM, Konstantin Ananyev wrote:
> > 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.
> >    rte_cryptodev_sym_session_set_user_data() just blindly copies
> >    user provided data without checking memory boundaries violation.
> >    To overcome that issue propose to add '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 propose to add '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 cryptodev_sym_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.
> >    To overcome that situation propose to 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).
> >
> > 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>
> 
> Adding maintainers to ack this deprecation notice. These changes will
> impact all the PMDs and everyone
> should agree to these changes.
> 
> from NXP dpaa_sec, dpaa2_sec, caam_jr PMDs:
> 
> Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
> > ---
> >   doc/guides/rel_notes/deprecation.rst | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> >
> > diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> > index d2aec64d1..998a0d92c 100644
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > @@ -74,3 +74,12 @@ Deprecation Notices
> >
> >     This is due to a lack of flexibility and reliance on a type unusable with
> >     C++ programs (struct rte_flow_desc).
> > +
> > +* cryptodev: several API and ABI changes are planned for rte_cryptodev
> > +  in v19.02:
> > +
> > +  - The size and layout of ``rte_cryptodev_sym_session`` will change
> > +    to fix existing issues.
> > +  - The size and layout of ``rte_cryptodev_qp_conf`` and syntax of
> > +    ``rte_cryptodev_queue_pair_setup`` will change to to allow to use
> > +    two different mempools for crypto and device private sessions.

Along with the naming changes agreed in the related RFC thread
Acked-by: Fiona Trahe <fiona.trahe@intel.com>

Re: [dpdk-dev] [PATCH] doc: cryptodev deprecation notice for sym session changes

[Joseph, Anoob]

Hi Akhil, Konstantin,

Wouldn't the new element, userdata, conflict with the one referred by

rte_cryptodev_sym_session_set_user_data()
rte_cryptodev_sym_session_get_user_data()

Do you mind a name change for either? Or do you have a clear picture of when one should be used over the other?

Thanks,
Anoob

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: 12 November 2018 17:34
> To: Konstantin Ananyev <konstantin.ananyev@intel.com>; dev@dpdk.org; Ravi
> Kumar <ravi1.kumar@amd.com>; Jacob, Jerin
> <Jerin.JacobKollanukkaran@cavium.com>; Joseph, Anoob
> <Anoob.Joseph@cavium.com>; Declan Doherty <declan.doherty@intel.com>;
> Fiona Trahe <fiona.trahe@intel.com>; Tomasz Duszynski <tdu@semihalf.com>;
> Dmitri Epshtein <dima@marvell.com>; Natalie Samsonov
> <nsamsono@marvell.com>; Jay Zhou <jianjay.zhou@huawei.com>
> Subject: Re: [dpdk-dev] [PATCH] doc: cryptodev deprecation notice for sym
> session changes
> 
> External Email
> 
> On 10/11/2018 7:50 PM, Konstantin Ananyev wrote:
> > 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.
> >    rte_cryptodev_sym_session_set_user_data() just blindly copies
> >    user provided data without checking memory boundaries violation.
> >    To overcome that issue propose to add '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 propose to add '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 cryptodev_sym_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.
> >    To overcome that situation propose to 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).
> >
> > 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>
> 
> Adding maintainers to ack this deprecation notice. These changes will impact all
> the PMDs and everyone should agree to these changes.
> 
> from NXP dpaa_sec, dpaa2_sec, caam_jr PMDs:
> 
> Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
> > ---
> >   doc/guides/rel_notes/deprecation.rst | 9 +++++++++
> >   1 file changed, 9 insertions(+)
> >
> > diff --git a/doc/guides/rel_notes/deprecation.rst
> > b/doc/guides/rel_notes/deprecation.rst
> > index d2aec64d1..998a0d92c 100644
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > @@ -74,3 +74,12 @@ Deprecation Notices
> >
> >     This is due to a lack of flexibility and reliance on a type unusable with
> >     C++ programs (struct rte_flow_desc).
> > +
> > +* cryptodev: several API and ABI changes are planned for
> > +rte_cryptodev
> > +  in v19.02:
> > +
> > +  - The size and layout of ``rte_cryptodev_sym_session`` will change
> > +    to fix existing issues.
> > +  - The size and layout of ``rte_cryptodev_qp_conf`` and syntax of
> > +    ``rte_cryptodev_queue_pair_setup`` will change to to allow to use
> > +    two different mempools for crypto and device private sessions.

Re: [dpdk-dev] [PATCH] doc: cryptodev deprecation notice for sym session changes

[Ananyev, Konstantin]

Hi Anoob,

> 
> Hi Akhil, Konstantin,
> 
> Wouldn't the new element, userdata, conflict with the one referred by
> 
> rte_cryptodev_sym_session_set_user_data()
> rte_cryptodev_sym_session_get_user_data()
> 
> Do you mind a name change for either? Or do you have a clear picture of when one should be used over the other?

Yes, Fiona also pointed to that naming collision.
Current suggestion is to name this new element 'opaque_data' or so.
Konstantin

> 
> Thanks,
> Anoob
> 
> > -----Original Message-----
> > From: Akhil Goyal <akhil.goyal@nxp.com>
> > Sent: 12 November 2018 17:34
> > To: Konstantin Ananyev <konstantin.ananyev@intel.com>; dev@dpdk.org; Ravi
> > Kumar <ravi1.kumar@amd.com>; Jacob, Jerin
> > <Jerin.JacobKollanukkaran@cavium.com>; Joseph, Anoob
> > <Anoob.Joseph@cavium.com>; Declan Doherty <declan.doherty@intel.com>;
> > Fiona Trahe <fiona.trahe@intel.com>; Tomasz Duszynski <tdu@semihalf.com>;
> > Dmitri Epshtein <dima@marvell.com>; Natalie Samsonov
> > <nsamsono@marvell.com>; Jay Zhou <jianjay.zhou@huawei.com>
> > Subject: Re: [dpdk-dev] [PATCH] doc: cryptodev deprecation notice for sym
> > session changes
> >
> > External Email
> >
> > On 10/11/2018 7:50 PM, Konstantin Ananyev wrote:
> > > 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.
> > >    rte_cryptodev_sym_session_set_user_data() just blindly copies
> > >    user provided data without checking memory boundaries violation.
> > >    To overcome that issue propose to add '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 propose to add '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 cryptodev_sym_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.
> > >    To overcome that situation propose to 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).
> > >
> > > 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>
> >
> > Adding maintainers to ack this deprecation notice. These changes will impact all
> > the PMDs and everyone should agree to these changes.
> >
> > from NXP dpaa_sec, dpaa2_sec, caam_jr PMDs:
> >
> > Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
> > > ---
> > >   doc/guides/rel_notes/deprecation.rst | 9 +++++++++
> > >   1 file changed, 9 insertions(+)
> > >
> > > diff --git a/doc/guides/rel_notes/deprecation.rst
> > > b/doc/guides/rel_notes/deprecation.rst
> > > index d2aec64d1..998a0d92c 100644
> > > --- a/doc/guides/rel_notes/deprecation.rst
> > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > @@ -74,3 +74,12 @@ Deprecation Notices
> > >
> > >     This is due to a lack of flexibility and reliance on a type unusable with
> > >     C++ programs (struct rte_flow_desc).
> > > +
> > > +* cryptodev: several API and ABI changes are planned for
> > > +rte_cryptodev
> > > +  in v19.02:
> > > +
> > > +  - The size and layout of ``rte_cryptodev_sym_session`` will change
> > > +    to fix existing issues.
> > > +  - The size and layout of ``rte_cryptodev_qp_conf`` and syntax of
> > > +    ``rte_cryptodev_queue_pair_setup`` will change to to allow to use
> > > +    two different mempools for crypto and device private sessions.

Re: [dpdk-dev] [PATCH] doc: cryptodev deprecation notice for sym session changes

[Joseph, Anoob]

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: 14 November 2018 15:38
> To: Joseph, Anoob <Anoob.Joseph@cavium.com>; Akhil Goyal
> <akhil.goyal@nxp.com>; dev@dpdk.org; Ravi Kumar <ravi1.kumar@amd.com>;
> Jacob, Jerin <Jerin.JacobKollanukkaran@cavium.com>; Doherty, Declan
> <declan.doherty@intel.com>; Trahe, Fiona <fiona.trahe@intel.com>; Tomasz
> Duszynski <tdu@semihalf.com>; Dmitri Epshtein <dima@marvell.com>; Natalie
> Samsonov <nsamsono@marvell.com>; Jay Zhou <jianjay.zhou@huawei.com>
> Cc: Athreya, Narayana Prasad <NarayanaPrasad.Athreya@cavium.com>; Verma,
> Shally <Shally.Verma@cavium.com>; Dwivedi, Ankur
> <Ankur.Dwivedi@cavium.com>
> Subject: RE: [dpdk-dev] [PATCH] doc: cryptodev deprecation notice for sym
> session changes
> 
> External Email
> 
> Hi Anoob,
> 
> >
> > Hi Akhil, Konstantin,
> >
> > Wouldn't the new element, userdata, conflict with the one referred by
> >
> > rte_cryptodev_sym_session_set_user_data()
> > rte_cryptodev_sym_session_get_user_data()
> >
> > Do you mind a name change for either? Or do you have a clear picture of when
> one should be used over the other?
> 
> Yes, Fiona also pointed to that naming collision.
> Current suggestion is to name this new element 'opaque_data' or so.
> Konstantin
> 
> >
> > Thanks,
> > Anoob
> >
> > > -----Original Message-----
> > > From: Akhil Goyal <akhil.goyal@nxp.com>
> > > Sent: 12 November 2018 17:34
> > > To: Konstantin Ananyev <konstantin.ananyev@intel.com>; dev@dpdk.org;
> > > Ravi Kumar <ravi1.kumar@amd.com>; Jacob, Jerin
> > > <Jerin.JacobKollanukkaran@cavium.com>; Joseph, Anoob
> > > <Anoob.Joseph@cavium.com>; Declan Doherty
> > > <declan.doherty@intel.com>; Fiona Trahe <fiona.trahe@intel.com>;
> > > Tomasz Duszynski <tdu@semihalf.com>; Dmitri Epshtein
> > > <dima@marvell.com>; Natalie Samsonov <nsamsono@marvell.com>; Jay
> > > Zhou <jianjay.zhou@huawei.com>
> > > Subject: Re: [dpdk-dev] [PATCH] doc: cryptodev deprecation notice
> > > for sym session changes
> > >
> > > External Email
> > >
> > > On 10/11/2018 7:50 PM, Konstantin Ananyev wrote:
> > > > 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.
> > > >    rte_cryptodev_sym_session_set_user_data() just blindly copies
> > > >    user provided data without checking memory boundaries violation.
> > > >    To overcome that issue propose to add '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 propose to add '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 cryptodev_sym_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.
> > > >    To overcome that situation propose to 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).
> > > >
> > > > 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>
> > >
> > > Adding maintainers to ack this deprecation notice. These changes
> > > will impact all the PMDs and everyone should agree to these changes.
> > >
> > > from NXP dpaa_sec, dpaa2_sec, caam_jr PMDs:
> > >
> > > Acked-by: Akhil Goyal <akhil.goyal@nxp.com>

With the naming changes,
Acked-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>

> > > > ---
> > > >   doc/guides/rel_notes/deprecation.rst | 9 +++++++++
> > > >   1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/doc/guides/rel_notes/deprecation.rst
> > > > b/doc/guides/rel_notes/deprecation.rst
> > > > index d2aec64d1..998a0d92c 100644
> > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > @@ -74,3 +74,12 @@ Deprecation Notices
> > > >
> > > >     This is due to a lack of flexibility and reliance on a type unusable with
> > > >     C++ programs (struct rte_flow_desc).
> > > > +
> > > > +* cryptodev: several API and ABI changes are planned for
> > > > +rte_cryptodev
> > > > +  in v19.02:
> > > > +
> > > > +  - The size and layout of ``rte_cryptodev_sym_session`` will change
> > > > +    to fix existing issues.
> > > > +  - The size and layout of ``rte_cryptodev_qp_conf`` and syntax of
> > > > +    ``rte_cryptodev_queue_pair_setup`` will change to to allow to use
> > > > +    two different mempools for crypto and device private sessions.

Re: [dpdk-dev] [PATCH] doc: cryptodev deprecation notice for sym session changes

[Thomas Monjalon]

14/11/2018 11:12, Joseph, Anoob:
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> > 
> > Hi Anoob,
> > 
> > >
> > > Hi Akhil, Konstantin,
> > >
> > > Wouldn't the new element, userdata, conflict with the one referred by
> > >
> > > rte_cryptodev_sym_session_set_user_data()
> > > rte_cryptodev_sym_session_get_user_data()
> > >
> > > Do you mind a name change for either? Or do you have a clear picture of when
> > one should be used over the other?
> > 
> > Yes, Fiona also pointed to that naming collision.
> > Current suggestion is to name this new element 'opaque_data' or so.
> > Konstantin
> > 
> > >
> > > Thanks,
> > > Anoob
> > >
> > > From: Akhil Goyal <akhil.goyal@nxp.com>
> > > > On 10/11/2018 7:50 PM, Konstantin Ananyev wrote:
> > > > > 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.
> > > > >    rte_cryptodev_sym_session_set_user_data() just blindly copies
> > > > >    user provided data without checking memory boundaries violation.
> > > > >    To overcome that issue propose to add '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 propose to add '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 cryptodev_sym_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.
> > > > >    To overcome that situation propose to 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).
> > > > >
> > > > > 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>
> > > >
> > > > Adding maintainers to ack this deprecation notice. These changes
> > > > will impact all the PMDs and everyone should agree to these changes.
> > > >
> > > > from NXP dpaa_sec, dpaa2_sec, caam_jr PMDs:
> > > >
> > > > Acked-by: Akhil Goyal <akhil.goyal@nxp.com>
> 
> With the naming changes,
> Acked-by: Anoob Joseph <anoob.joseph@caviumnetworks.com>

Applied