[v2,0/3] examples/ipsec-secgw: set default

  This patch set, sets the default code path in the ipsec-secgw
application to use the librte_ipsec.
The *_old test scripts have been modified to use the legacy code 
path.

Changes in v2:
-------------
The error messages for the -l option have been updated.
The pktest.sh script has been updated to drop the -l option.

Bernard Iremonger (3):
  examples/ipsec-secgw: set default to IPsec library mode
  examples/ipsec-secgw: add -l 0 parameter to old scripts
  examples/ipsec-secgw: update pktest.sh script

 doc/guides/rel_notes/release_19_11.rst             |  8 ++++
 doc/guides/sample_app_ug/ipsec_secgw.rst           |  6 ++-
 examples/ipsec-secgw/ipsec-secgw.c                 | 46 ++++++++++++++--------
 examples/ipsec-secgw/test/pkttest.sh               |  1 -
 .../ipsec-secgw/test/trs_3descbc_sha1_old_defs.sh  |  2 +-
 .../ipsec-secgw/test/trs_aescbc_sha1_old_defs.sh   |  2 +-
 .../ipsec-secgw/test/trs_aesctr_sha1_old_defs.sh   |  2 +-
 .../test/trs_aesgcm_inline_crypto_old_defs.sh      |  2 +-
 examples/ipsec-secgw/test/trs_aesgcm_old_defs.sh   |  2 +-
 .../ipsec-secgw/test/tun_3descbc_sha1_old_defs.sh  |  2 +-
 .../ipsec-secgw/test/tun_aescbc_sha1_old_defs.sh   |  2 +-
 .../ipsec-secgw/test/tun_aesctr_sha1_old_defs.sh   |  2 +-
 .../test/tun_aesgcm_inline_crypto_old_defs.sh      |  2 +-
 examples/ipsec-secgw/test/tun_aesgcm_old_defs.sh   |  2 +-
 14 files changed, 52 insertions(+), 29 deletions(-)
  

Hi Bernard,

> 
> This patch set, sets the default code path in the ipsec-secgw
> application to use the librte_ipsec.
> The *_old test scripts have been modified to use the legacy code
> path.
> 
> Changes in v2:
> -------------
> The error messages for the -l option have been updated.
> The pktest.sh script has been updated to drop the -l option.

The patches looks good to me.
Just one thing - shouldn't we also update the docs (AG for ipsec-secgw)?
Konstantin

> 
> Bernard Iremonger (3):
>   examples/ipsec-secgw: set default to IPsec library mode
>   examples/ipsec-secgw: add -l 0 parameter to old scripts
>   examples/ipsec-secgw: update pktest.sh script
> 
>  doc/guides/rel_notes/release_19_11.rst             |  8 ++++
>  doc/guides/sample_app_ug/ipsec_secgw.rst           |  6 ++-
>  examples/ipsec-secgw/ipsec-secgw.c                 | 46 ++++++++++++++--------
>  examples/ipsec-secgw/test/pkttest.sh               |  1 -
>  .../ipsec-secgw/test/trs_3descbc_sha1_old_defs.sh  |  2 +-
>  .../ipsec-secgw/test/trs_aescbc_sha1_old_defs.sh   |  2 +-
>  .../ipsec-secgw/test/trs_aesctr_sha1_old_defs.sh   |  2 +-
>  .../test/trs_aesgcm_inline_crypto_old_defs.sh      |  2 +-
>  examples/ipsec-secgw/test/trs_aesgcm_old_defs.sh   |  2 +-
>  .../ipsec-secgw/test/tun_3descbc_sha1_old_defs.sh  |  2 +-
>  .../ipsec-secgw/test/tun_aescbc_sha1_old_defs.sh   |  2 +-
>  .../ipsec-secgw/test/tun_aesctr_sha1_old_defs.sh   |  2 +-
>  .../test/tun_aesgcm_inline_crypto_old_defs.sh      |  2 +-
>  examples/ipsec-secgw/test/tun_aesgcm_old_defs.sh   |  2 +-
>  14 files changed, 52 insertions(+), 29 deletions(-)
> 
> --
> 2.7.4
  

Hi Konstantin,

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Tuesday, October 1, 2019 6:31 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; dev@dpdk.org;
> akhil.goyal@nxp.com
> Subject: RE: [PATCH v2 0/3] examples/ipsec-secgw: set default
> 
> 
> Hi Bernard,
> 
> >
> > This patch set, sets the default code path in the ipsec-secgw
> > application to use the librte_ipsec.
> > The *_old test scripts have been modified to use the legacy code path.
> >
> > Changes in v2:
> > -------------
> > The error messages for the -l option have been updated.
> > The pktest.sh script has been updated to drop the -l option.
> 
> The patches looks good to me.
> Just one thing - shouldn't we also update the docs (AG for ipsec-secgw)?
> Konstantin

The 19.11 release notes and the sample application guide for ipsec-secgw have been updated.
The updates are in the first patch (0001) with ipsec-secgw.c updates.


> > Bernard Iremonger (3):
> >   examples/ipsec-secgw: set default to IPsec library mode
> >   examples/ipsec-secgw: add -l 0 parameter to old scripts
> >   examples/ipsec-secgw: update pktest.sh script
> >
> >  doc/guides/rel_notes/release_19_11.rst             |  8 ++++
> >  doc/guides/sample_app_ug/ipsec_secgw.rst           |  6 ++-
> >  examples/ipsec-secgw/ipsec-secgw.c                 | 46 ++++++++++++++-------
> -
> >  examples/ipsec-secgw/test/pkttest.sh               |  1 -
> >  .../ipsec-secgw/test/trs_3descbc_sha1_old_defs.sh  |  2 +-
> >  .../ipsec-secgw/test/trs_aescbc_sha1_old_defs.sh   |  2 +-
> >  .../ipsec-secgw/test/trs_aesctr_sha1_old_defs.sh   |  2 +-
> >  .../test/trs_aesgcm_inline_crypto_old_defs.sh      |  2 +-
> >  examples/ipsec-secgw/test/trs_aesgcm_old_defs.sh   |  2 +-
> >  .../ipsec-secgw/test/tun_3descbc_sha1_old_defs.sh  |  2 +-
> >  .../ipsec-secgw/test/tun_aescbc_sha1_old_defs.sh   |  2 +-
> >  .../ipsec-secgw/test/tun_aesctr_sha1_old_defs.sh   |  2 +-
> >  .../test/tun_aesgcm_inline_crypto_old_defs.sh      |  2 +-
> >  examples/ipsec-secgw/test/tun_aesgcm_old_defs.sh   |  2 +-
> >  14 files changed, 52 insertions(+), 29 deletions(-)
> >
> > --
> > 2.7.4

Regards,

Bernard.
  

> -----Original Message-----
> From: Iremonger, Bernard
> Sent: Wednesday, October 2, 2019 10:00 AM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; dev@dpdk.org; akhil.goyal@nxp.com
> Subject: RE: [PATCH v2 0/3] examples/ipsec-secgw: set default
> 
> Hi Konstantin,
> 
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Tuesday, October 1, 2019 6:31 PM
> > To: Iremonger, Bernard <bernard.iremonger@intel.com>; dev@dpdk.org;
> > akhil.goyal@nxp.com
> > Subject: RE: [PATCH v2 0/3] examples/ipsec-secgw: set default
> >
> >
> > Hi Bernard,
> >
> > >
> > > This patch set, sets the default code path in the ipsec-secgw
> > > application to use the librte_ipsec.
> > > The *_old test scripts have been modified to use the legacy code path.
> > >
> > > Changes in v2:
> > > -------------
> > > The error messages for the -l option have been updated.
> > > The pktest.sh script has been updated to drop the -l option.
> >
> > The patches looks good to me.
> > Just one thing - shouldn't we also update the docs (AG for ipsec-secgw)?
> > Konstantin
> 
> The 19.11 release notes and the sample application guide for ipsec-secgw have been updated.
> The updates are in the first patch (0001) with ipsec-secgw.c updates.

Ah, ok sorry for the noise then :)
Konstantin

> 
> 
> > > Bernard Iremonger (3):
> > >   examples/ipsec-secgw: set default to IPsec library mode
> > >   examples/ipsec-secgw: add -l 0 parameter to old scripts
> > >   examples/ipsec-secgw: update pktest.sh script
> > >
> > >  doc/guides/rel_notes/release_19_11.rst             |  8 ++++
> > >  doc/guides/sample_app_ug/ipsec_secgw.rst           |  6 ++-
> > >  examples/ipsec-secgw/ipsec-secgw.c                 | 46 ++++++++++++++-------
> > -
> > >  examples/ipsec-secgw/test/pkttest.sh               |  1 -
> > >  .../ipsec-secgw/test/trs_3descbc_sha1_old_defs.sh  |  2 +-
> > >  .../ipsec-secgw/test/trs_aescbc_sha1_old_defs.sh   |  2 +-
> > >  .../ipsec-secgw/test/trs_aesctr_sha1_old_defs.sh   |  2 +-
> > >  .../test/trs_aesgcm_inline_crypto_old_defs.sh      |  2 +-
> > >  examples/ipsec-secgw/test/trs_aesgcm_old_defs.sh   |  2 +-
> > >  .../ipsec-secgw/test/tun_3descbc_sha1_old_defs.sh  |  2 +-
> > >  .../ipsec-secgw/test/tun_aescbc_sha1_old_defs.sh   |  2 +-
> > >  .../ipsec-secgw/test/tun_aesctr_sha1_old_defs.sh   |  2 +-
> > >  .../test/tun_aesgcm_inline_crypto_old_defs.sh      |  2 +-
> > >  examples/ipsec-secgw/test/tun_aesgcm_old_defs.sh   |  2 +-
> > >  14 files changed, 52 insertions(+), 29 deletions(-)
> > >
> > > --
> > > 2.7.4
> 
> Regards,
> 
> Bernard.
  

Hi All,

This patchset would need ack from more vendors as it will impact user experience
on a key example application which is normally demonstrated to customers.

IPSec library is still evolving and there are new functionality added every release.
Atleast from NXP side we are not OK with this change.

I would hold this patch till RC2 atleast.

Regards,
Akhil

> -----Original Message-----
> From: Bernard Iremonger <bernard.iremonger@intel.com>
> Sent: Tuesday, October 1, 2019 8:48 PM
> To: dev@dpdk.org; konstantin.ananyev@intel.com; Akhil Goyal
> <akhil.goyal@nxp.com>
> Cc: Bernard Iremonger <bernard.iremonger@intel.com>
> Subject: [PATCH v2 0/3] examples/ipsec-secgw: set default
> 
> This patch set, sets the default code path in the ipsec-secgw
> application to use the librte_ipsec.
> The *_old test scripts have been modified to use the legacy code
> path.
> 
> Changes in v2:
> -------------
> The error messages for the -l option have been updated.
> The pktest.sh script has been updated to drop the -l option.
> 
> Bernard Iremonger (3):
>   examples/ipsec-secgw: set default to IPsec library mode
>   examples/ipsec-secgw: add -l 0 parameter to old scripts
>   examples/ipsec-secgw: update pktest.sh script
> 
>  doc/guides/rel_notes/release_19_11.rst             |  8 ++++
>  doc/guides/sample_app_ug/ipsec_secgw.rst           |  6 ++-
>  examples/ipsec-secgw/ipsec-secgw.c                 | 46 ++++++++++++++--------
>  examples/ipsec-secgw/test/pkttest.sh               |  1 -
>  .../ipsec-secgw/test/trs_3descbc_sha1_old_defs.sh  |  2 +-
>  .../ipsec-secgw/test/trs_aescbc_sha1_old_defs.sh   |  2 +-
>  .../ipsec-secgw/test/trs_aesctr_sha1_old_defs.sh   |  2 +-
>  .../test/trs_aesgcm_inline_crypto_old_defs.sh      |  2 +-
>  examples/ipsec-secgw/test/trs_aesgcm_old_defs.sh   |  2 +-
>  .../ipsec-secgw/test/tun_3descbc_sha1_old_defs.sh  |  2 +-
>  .../ipsec-secgw/test/tun_aescbc_sha1_old_defs.sh   |  2 +-
>  .../ipsec-secgw/test/tun_aesctr_sha1_old_defs.sh   |  2 +-
>  .../test/tun_aesgcm_inline_crypto_old_defs.sh      |  2 +-
>  examples/ipsec-secgw/test/tun_aesgcm_old_defs.sh   |  2 +-
>  14 files changed, 52 insertions(+), 29 deletions(-)
> 
> --
> 2.7.4
  

Hi Akhil,

With this patch applied the legacy code path in the ipsec-secgw application is still available. The default code path is now to use librte_ipsec.
The "-l 0" option at startup allows the legacy code path to be used.
Both code paths are still available.

Regards,

Bernard 

> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> Sent: Friday, October 11, 2019 1:40 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; dev@dpdk.org;
> Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> anoobj@marvell.com; jerinj@marvell.com; 'Thomas Monjalon'
> <thomas@monjalon.net>
> Subject: RE: [PATCH v2 0/3] examples/ipsec-secgw: set default
> 
> Hi All,
> 
> This patchset would need ack from more vendors as it will impact user
> experience on a key example application which is normally demonstrated to
> customers.
> 
> IPSec library is still evolving and there are new functionality added every
> release.
> Atleast from NXP side we are not OK with this change.
> 
> I would hold this patch till RC2 atleast.
> 
> Regards,
> Akhil
> 
> > -----Original Message-----
> > From: Bernard Iremonger <bernard.iremonger@intel.com>
> > Sent: Tuesday, October 1, 2019 8:48 PM
> > To: dev@dpdk.org; konstantin.ananyev@intel.com; Akhil Goyal
> > <akhil.goyal@nxp.com>
> > Cc: Bernard Iremonger <bernard.iremonger@intel.com>
> > Subject: [PATCH v2 0/3] examples/ipsec-secgw: set default
> >
> > This patch set, sets the default code path in the ipsec-secgw
> > application to use the librte_ipsec.
> > The *_old test scripts have been modified to use the legacy code path.
> >
> > Changes in v2:
> > -------------
> > The error messages for the -l option have been updated.
> > The pktest.sh script has been updated to drop the -l option.
> >
> > Bernard Iremonger (3):
> >   examples/ipsec-secgw: set default to IPsec library mode
> >   examples/ipsec-secgw: add -l 0 parameter to old scripts
> >   examples/ipsec-secgw: update pktest.sh script
> >
> >  doc/guides/rel_notes/release_19_11.rst             |  8 ++++
> >  doc/guides/sample_app_ug/ipsec_secgw.rst           |  6 ++-
> >  examples/ipsec-secgw/ipsec-secgw.c                 | 46 ++++++++++++++-------
> -
> >  examples/ipsec-secgw/test/pkttest.sh               |  1 -
> >  .../ipsec-secgw/test/trs_3descbc_sha1_old_defs.sh  |  2 +-
> >  .../ipsec-secgw/test/trs_aescbc_sha1_old_defs.sh   |  2 +-
> >  .../ipsec-secgw/test/trs_aesctr_sha1_old_defs.sh   |  2 +-
> >  .../test/trs_aesgcm_inline_crypto_old_defs.sh      |  2 +-
> >  examples/ipsec-secgw/test/trs_aesgcm_old_defs.sh   |  2 +-
> >  .../ipsec-secgw/test/tun_3descbc_sha1_old_defs.sh  |  2 +-
> >  .../ipsec-secgw/test/tun_aescbc_sha1_old_defs.sh   |  2 +-
> >  .../ipsec-secgw/test/tun_aesctr_sha1_old_defs.sh   |  2 +-
> >  .../test/tun_aesgcm_inline_crypto_old_defs.sh      |  2 +-
> >  examples/ipsec-secgw/test/tun_aesgcm_old_defs.sh   |  2 +-
> >  14 files changed, 52 insertions(+), 29 deletions(-)
> >
> > --
> > 2.7.4
  

11/10/2019 14:40, Akhil Goyal:
> Hi All,
> 
> This patchset would need ack from more vendors as it will impact user experience
> on a key example application which is normally demonstrated to customers.
> 
> IPSec library is still evolving and there are new functionality added every release.
> Atleast from NXP side we are not OK with this change.

What can be changed in the library to make it acceptable as a default in this example?
  

> 
> 11/10/2019 14:40, Akhil Goyal:
> > Hi All,
> >
> > This patchset would need ack from more vendors as it will impact user
> experience
> > on a key example application which is normally demonstrated to customers.
> >
> > IPSec library is still evolving and there are new functionality added every
> release.
> > Atleast from NXP side we are not OK with this change.
> 
> What can be changed in the library to make it acceptable as a default in this
> example?
> 

We are observing performance issues with ipsec library. So would request other
Vendors to confirm if they are OK with the performance numbers.
  

Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal [mailto:akhil.goyal@nxp.com]
> Sent: Tuesday, October 15, 2019 7:43 AM
> To: Thomas Monjalon <thomas@monjalon.net>
> Cc: Iremonger, Bernard <bernard.iremonger@intel.com>; dev@dpdk.org;
> Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> anoobj@marvell.com; jerinj@marvell.com
> Subject: RE: [PATCH v2 0/3] examples/ipsec-secgw: set default
> 
> 
> >
> > 11/10/2019 14:40, Akhil Goyal:
> > > Hi All,
> > >
> > > This patchset would need ack from more vendors as it will impact
> > > user
> > experience
> > > on a key example application which is normally demonstrated to
> customers.
> > >
> > > IPSec library is still evolving and there are new functionality
> > > added every
> > release.
> > > Atleast from NXP side we are not OK with this change.
> >
> > What can be changed in the library to make it acceptable as a default
> > in this example?
> >
> 
> We are observing performance issues with ipsec library. So would request
> other Vendors to confirm if they are OK with the performance numbers.

Could you give some details on the performance issues you are seeing.

Regards,

Bernard.
  

> > >
> > > 11/10/2019 14:40, Akhil Goyal:
> > > > Hi All,
> > > >
> > > > This patchset would need ack from more vendors as it will impact
> > > > user
> > > experience
> > > > on a key example application which is normally demonstrated to
> > customers.
> > > >
> > > > IPSec library is still evolving and there are new functionality
> > > > added every
> > > release.
> > > > Atleast from NXP side we are not OK with this change.
> > >
> > > What can be changed in the library to make it acceptable as a default
> > > in this example?
> > >
> >
> > We are observing performance issues with ipsec library. So would request
> > other Vendors to confirm if they are OK with the performance numbers.
> 
> Could you give some details on the performance issues you are seeing.
> 

We were observing about 4-5% drop when using the ipsec-lib instead of the
Legacy code path. We would again measure it on RC1. That is why I say, I will
Hold this patch till RC2, unless some other vendor also confirms that.
  

Hi Akhil,

Marvell is NOT OK with this change in this cycle. The library is still emerging and the boundary between ipsec-secgw and lib ipsec still hasn't emerged clearly. Moreover, lib_ipsec mode isn't even working with the default conf (I believe the fix for this still in discussion. The issue was reported by a Marvell engineer some time back). There are questionable features getting pushed, for which Marvell's concerns were subdued.

I would expect this taken up after lib ipsec is found stable for atleast one release cycle.

As for the performance, we found 2-3% drop with the lib ipsec in the last release (lookaside crypto mode). Yet to try out with latest codebase. With some of the recent submissions, I expect an even higher performance hit. Also, eventmode ipsec-secgw (already submitted RFC) is using non lib ipsec mode. We don't have plans to move that to lib ipsec mode until lib ipsec is made stable.

Thanks,
Anoob

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Friday, October 11, 2019 6:10 PM
> To: Bernard Iremonger <bernard.iremonger@intel.com>; dev@dpdk.org;
> konstantin.ananyev@intel.com; Anoob Joseph <anoobj@marvell.com>;
> Jerin Jacob Kollanukkaran <jerinj@marvell.com>; 'Thomas Monjalon'
> <thomas@monjalon.net>
> Subject: [EXT] RE: [PATCH v2 0/3] examples/ipsec-secgw: set default
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hi All,
> 
> This patchset would need ack from more vendors as it will impact user
> experience on a key example application which is normally demonstrated to
> customers.
> 
> IPSec library is still evolving and there are new functionality added every
> release.
> Atleast from NXP side we are not OK with this change.
> 
> I would hold this patch till RC2 atleast.
> 
> Regards,
> Akhil
> 
> > -----Original Message-----
> > From: Bernard Iremonger <bernard.iremonger@intel.com>
> > Sent: Tuesday, October 1, 2019 8:48 PM
> > To: dev@dpdk.org; konstantin.ananyev@intel.com; Akhil Goyal
> > <akhil.goyal@nxp.com>
> > Cc: Bernard Iremonger <bernard.iremonger@intel.com>
> > Subject: [PATCH v2 0/3] examples/ipsec-secgw: set default
> >
> > This patch set, sets the default code path in the ipsec-secgw
> > application to use the librte_ipsec.
> > The *_old test scripts have been modified to use the legacy code path.
> >
> > Changes in v2:
> > -------------
> > The error messages for the -l option have been updated.
> > The pktest.sh script has been updated to drop the -l option.
> >
> > Bernard Iremonger (3):
> >   examples/ipsec-secgw: set default to IPsec library mode
> >   examples/ipsec-secgw: add -l 0 parameter to old scripts
> >   examples/ipsec-secgw: update pktest.sh script
> >
> >  doc/guides/rel_notes/release_19_11.rst             |  8 ++++
> >  doc/guides/sample_app_ug/ipsec_secgw.rst           |  6 ++-
> >  examples/ipsec-secgw/ipsec-secgw.c                 | 46 ++++++++++++++-------
> -
> >  examples/ipsec-secgw/test/pkttest.sh               |  1 -
> >  .../ipsec-secgw/test/trs_3descbc_sha1_old_defs.sh  |  2 +-
> >  .../ipsec-secgw/test/trs_aescbc_sha1_old_defs.sh   |  2 +-
> >  .../ipsec-secgw/test/trs_aesctr_sha1_old_defs.sh   |  2 +-
> >  .../test/trs_aesgcm_inline_crypto_old_defs.sh      |  2 +-
> >  examples/ipsec-secgw/test/trs_aesgcm_old_defs.sh   |  2 +-
> >  .../ipsec-secgw/test/tun_3descbc_sha1_old_defs.sh  |  2 +-
> >  .../ipsec-secgw/test/tun_aescbc_sha1_old_defs.sh   |  2 +-
> >  .../ipsec-secgw/test/tun_aesctr_sha1_old_defs.sh   |  2 +-
> >  .../test/tun_aesgcm_inline_crypto_old_defs.sh      |  2 +-
> >  examples/ipsec-secgw/test/tun_aesgcm_old_defs.sh   |  2 +-
> >  14 files changed, 52 insertions(+), 29 deletions(-)
> >
> > --
> > 2.7.4
  

> > > > > Hi All,
> > > > >
> > > > > This patchset would need ack from more vendors as it will impact
> > > > > user
> > > > experience
> > > > > on a key example application which is normally demonstrated to
> > > customers.
> > > > >
> > > > > IPSec library is still evolving and there are new functionality
> > > > > added every
> > > > release.
> > > > > Atleast from NXP side we are not OK with this change.
> > > >
> > > > What can be changed in the library to make it acceptable as a default
> > > > in this example?
> > > >
> > >
> > > We are observing performance issues with ipsec library. So would request
> > > other Vendors to confirm if they are OK with the performance numbers.
> >
> > Could you give some details on the performance issues you are seeing.
> >
> 
> We were observing about 4-5% drop when using the ipsec-lib instead of the
> Legacy code path. We would again measure it on RC1. 

Yeh, I remember you already mentioned that few months ago...
Though we don't have access to NXP HW, so obviously we do need
some help from you guys here to address it:
configuration/test details so we can try to reproduce it
probably some insight from you what in particular causing that slowdown..
patches would be even better :)
Konstantin

> That is why I say, I will
> Hold this patch till RC2, unless some other vendor also confirms that.
  

Hi Akhil,

> -----Original Message-----
> From: Akhil Goyal <akhil.goyal@nxp.com>
> Sent: Tuesday, October 15, 2019 4:05 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>
> Cc: dev@dpdk.org; Ananyev, Konstantin <konstantin.ananyev@intel.com>;
> anoobj@marvell.com; jerinj@marvell.com
> Subject: RE: [PATCH v2 0/3] examples/ipsec-secgw: set default
> 
> 
> 
> > > >
> > > > 11/10/2019 14:40, Akhil Goyal:
> > > > > Hi All,
> > > > >
> > > > > This patchset would need ack from more vendors as it will impact
> > > > > user
> > > > experience
> > > > > on a key example application which is normally demonstrated to
> > > customers.
> > > > >
> > > > > IPSec library is still evolving and there are new functionality
> > > > > added every
> > > > release.
> > > > > Atleast from NXP side we are not OK with this change.
> > > >
> > > > What can be changed in the library to make it acceptable as a
> > > > default in this example?
> > > >
> > >
> > > We are observing performance issues with ipsec library. So would
> > > request other Vendors to confirm if they are OK with the performance
> numbers.
> >
> > Could you give some details on the performance issues you are seeing.
> >
> 
> We were observing about 4-5% drop when using the ipsec-lib instead of the
> Legacy code path. We would again measure it on RC1. That is why I say, I will
> Hold this patch till RC2, unless some other vendor also confirms that.

Is there any update on performance measurements on 19.11-rc1 ?

Regards,

Bernard.
  

Hi Bernard,
> 
> Hi Akhil,
> 
> >
> >
> > > > >
> > > > > 11/10/2019 14:40, Akhil Goyal:
> > > > > > Hi All,
> > > > > >
> > > > > > This patchset would need ack from more vendors as it will impact
> > > > > > user
> > > > > experience
> > > > > > on a key example application which is normally demonstrated to
> > > > customers.
> > > > > >
> > > > > > IPSec library is still evolving and there are new functionality
> > > > > > added every
> > > > > release.
> > > > > > Atleast from NXP side we are not OK with this change.
> > > > >
> > > > > What can be changed in the library to make it acceptable as a
> > > > > default in this example?
> > > > >
> > > >
> > > > We are observing performance issues with ipsec library. So would
> > > > request other Vendors to confirm if they are OK with the performance
> > numbers.
> > >
> > > Could you give some details on the performance issues you are seeing.
> > >
> >
> > We were observing about 4-5% drop when using the ipsec-lib instead of the
> > Legacy code path. We would again measure it on RC1. That is why I say, I will
> > Hold this patch till RC2, unless some other vendor also confirms that.
> 
> Is there any update on performance measurements on 19.11-rc1 ?
> 
The performance impact of this patch is huge ~10% w.r.t. 19.11-rc1 base on NXP hardware.

We cannot merge this. Anoob also reported performance issues on Marvell hardware.



> Regards,
> 
> Bernard.
  

Hi Akhil,

> > > > > >
> > > > > > 11/10/2019 14:40, Akhil Goyal:
> > > > > > > Hi All,
> > > > > > >
> > > > > > > This patchset would need ack from more vendors as it will impact
> > > > > > > user
> > > > > > experience
> > > > > > > on a key example application which is normally demonstrated to
> > > > > customers.
> > > > > > >
> > > > > > > IPSec library is still evolving and there are new functionality
> > > > > > > added every
> > > > > > release.
> > > > > > > Atleast from NXP side we are not OK with this change.
> > > > > >
> > > > > > What can be changed in the library to make it acceptable as a
> > > > > > default in this example?
> > > > > >
> > > > >
> > > > > We are observing performance issues with ipsec library. So would
> > > > > request other Vendors to confirm if they are OK with the performance
> > > numbers.
> > > >
> > > > Could you give some details on the performance issues you are seeing.
> > > >
> > >
> > > We were observing about 4-5% drop when using the ipsec-lib instead of the
> > > Legacy code path. We would again measure it on RC1. That is why I say, I will
> > > Hold this patch till RC2, unless some other vendor also confirms that.
> >
> > Is there any update on performance measurements on 19.11-rc1 ?
> >
> The performance impact of this patch is huge ~10% w.r.t. 19.11-rc1 base on NXP hardware.
> 
> We cannot merge this. Anoob also reported performance issues on Marvell hardware.

Sure, 10% is a lot, so more than understandable.
Though, I think we do need to decide our future goals for it.
I see two main options here:
1.  Make lib code-path on par with legacy one in terms of performance,
     deprecate and then remove legacy code-path.
     Till that happen (deprecation/removal) to minimize code divergence,
      forbid to add new features to legacy code path only.
     New features should be added to both paths, or library code path.
Obviously that one looks like a preferred option to me,
but it requires some effort from all interested parties (Intel, NXP, Marvell, ...).
If everyone is ok with it, then I think it would be good to have some draft timeline here.
If you guys are not interested in this effort, then the only other approach I can think about:  
2. split ipsec-secgw app into 2 (one using librte_ipsec, second using raw devices (legacy one)).
    We probably can still try to keep some code shared by 2 apps:
    (configuration/initialization/session management (SAD, SPD)),
    but actual packet processing path will be different.
I really don't like that option, but I think we need to come-up with clear decision,
one way or another. 

Thanks
Konstantin
  

Hi Konstantin,

> 
> Hi Akhil,
> 
> > > > > > >
> > > > > > > 11/10/2019 14:40, Akhil Goyal:
> > > > > > > > Hi All,
> > > > > > > >
> > > > > > > > This patchset would need ack from more vendors as it will impact
> > > > > > > > user
> > > > > > > experience
> > > > > > > > on a key example application which is normally demonstrated to
> > > > > > customers.
> > > > > > > >
> > > > > > > > IPSec library is still evolving and there are new functionality
> > > > > > > > added every
> > > > > > > release.
> > > > > > > > Atleast from NXP side we are not OK with this change.
> > > > > > >
> > > > > > > What can be changed in the library to make it acceptable as a
> > > > > > > default in this example?
> > > > > > >
> > > > > >
> > > > > > We are observing performance issues with ipsec library. So would
> > > > > > request other Vendors to confirm if they are OK with the performance
> > > > numbers.
> > > > >
> > > > > Could you give some details on the performance issues you are seeing.
> > > > >
> > > >
> > > > We were observing about 4-5% drop when using the ipsec-lib instead of
> the
> > > > Legacy code path. We would again measure it on RC1. That is why I say, I
> will
> > > > Hold this patch till RC2, unless some other vendor also confirms that.
> > >
> > > Is there any update on performance measurements on 19.11-rc1 ?
> > >
> > The performance impact of this patch is huge ~10% w.r.t. 19.11-rc1 base on
> NXP hardware.
> >
> > We cannot merge this. Anoob also reported performance issues on Marvell
> hardware.
> 
> Sure, 10% is a lot, so more than understandable.
> Though, I think we do need to decide our future goals for it.
> I see two main options here:
> 1.  Make lib code-path on par with legacy one in terms of performance,
>      deprecate and then remove legacy code-path.
>      Till that happen (deprecation/removal) to minimize code divergence,
>       forbid to add new features to legacy code path only.
>      New features should be added to both paths, or library code path.
> Obviously that one looks like a preferred option to me,
> but it requires some effort from all interested parties (Intel, NXP, Marvell, ...).
> If everyone is ok with it, then I think it would be good to have some draft
> timeline here.
> If you guys are not interested in this effort, then the only other approach I can
> think about:
> 2. split ipsec-secgw app into 2 (one using librte_ipsec, second using raw devices
> (legacy one)).
>     We probably can still try to keep some code shared by 2 apps:
>     (configuration/initialization/session management (SAD, SPD)),
>     but actual packet processing path will be different.
> I really don't like that option, but I think we need to come-up with clear decision,
> one way or another.
> 

IMO, Option 1 is the only way forward. From NXP side, we can start our work on this post 19.11 release and should target in 20.02 release.

Regards,
Akhil
  

Hi Akhil,
> >
> > > > > > > >
> > > > > > > > 11/10/2019 14:40, Akhil Goyal:
> > > > > > > > > Hi All,
> > > > > > > > >
> > > > > > > > > This patchset would need ack from more vendors as it will impact
> > > > > > > > > user
> > > > > > > > experience
> > > > > > > > > on a key example application which is normally demonstrated to
> > > > > > > customers.
> > > > > > > > >
> > > > > > > > > IPSec library is still evolving and there are new functionality
> > > > > > > > > added every
> > > > > > > > release.
> > > > > > > > > Atleast from NXP side we are not OK with this change.
> > > > > > > >
> > > > > > > > What can be changed in the library to make it acceptable as a
> > > > > > > > default in this example?
> > > > > > > >
> > > > > > >
> > > > > > > We are observing performance issues with ipsec library. So would
> > > > > > > request other Vendors to confirm if they are OK with the performance
> > > > > numbers.
> > > > > >
> > > > > > Could you give some details on the performance issues you are seeing.
> > > > > >
> > > > >
> > > > > We were observing about 4-5% drop when using the ipsec-lib instead of
> > the
> > > > > Legacy code path. We would again measure it on RC1. That is why I say, I
> > will
> > > > > Hold this patch till RC2, unless some other vendor also confirms that.
> > > >
> > > > Is there any update on performance measurements on 19.11-rc1 ?
> > > >
> > > The performance impact of this patch is huge ~10% w.r.t. 19.11-rc1 base on
> > NXP hardware.
> > >
> > > We cannot merge this. Anoob also reported performance issues on Marvell
> > hardware.
> >
> > Sure, 10% is a lot, so more than understandable.
> > Though, I think we do need to decide our future goals for it.
> > I see two main options here:
> > 1.  Make lib code-path on par with legacy one in terms of performance,
> >      deprecate and then remove legacy code-path.
> >      Till that happen (deprecation/removal) to minimize code divergence,
> >       forbid to add new features to legacy code path only.
> >      New features should be added to both paths, or library code path.
> > Obviously that one looks like a preferred option to me,
> > but it requires some effort from all interested parties (Intel, NXP, Marvell, ...).
> > If everyone is ok with it, then I think it would be good to have some draft
> > timeline here.
> > If you guys are not interested in this effort, then the only other approach I can
> > think about:
> > 2. split ipsec-secgw app into 2 (one using librte_ipsec, second using raw devices
> > (legacy one)).
> >     We probably can still try to keep some code shared by 2 apps:
> >     (configuration/initialization/session management (SAD, SPD)),
> >     but actual packet processing path will be different.
> > I really don't like that option, but I think we need to come-up with clear decision,
> > one way or another.
> >
> 
> IMO, Option 1 is the only way forward. From NXP side, we can start our work on this post 19.11 release and should target in 20.02 release.

Great to hear.
Thanks for clarification.
Konstantin
  

Hi Akhil, Konstantin,

Please see inline.

Thanks,
Anoob

> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Sent: Monday, November 4, 2019 8:55 PM
> To: Akhil Goyal <akhil.goyal@nxp.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>; Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org; Anoob Joseph <anoobj@marvell.com>; Jerin Jacob
> Kollanukkaran <jerinj@marvell.com>; dpdk-techboard <techboard@dpdk.org>
> Subject: [EXT] RE: [PATCH v2 0/3] examples/ipsec-secgw: set default
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hi Akhil,
> 
> > > > > > >
> > > > > > > 11/10/2019 14:40, Akhil Goyal:
> > > > > > > > Hi All,
> > > > > > > >
> > > > > > > > This patchset would need ack from more vendors as it will
> > > > > > > > impact user
> > > > > > > experience
> > > > > > > > on a key example application which is normally
> > > > > > > > demonstrated to
> > > > > > customers.
> > > > > > > >
> > > > > > > > IPSec library is still evolving and there are new
> > > > > > > > functionality added every
> > > > > > > release.
> > > > > > > > Atleast from NXP side we are not OK with this change.
> > > > > > >
> > > > > > > What can be changed in the library to make it acceptable as
> > > > > > > a default in this example?
> > > > > > >
> > > > > >
> > > > > > We are observing performance issues with ipsec library. So
> > > > > > would request other Vendors to confirm if they are OK with the
> > > > > > performance
> > > > numbers.
> > > > >
> > > > > Could you give some details on the performance issues you are seeing.
> > > > >
> > > >
> > > > We were observing about 4-5% drop when using the ipsec-lib instead
> > > > of the Legacy code path. We would again measure it on RC1. That is
> > > > why I say, I will Hold this patch till RC2, unless some other vendor also
> confirms that.
> > >
> > > Is there any update on performance measurements on 19.11-rc1 ?
> > >
> > The performance impact of this patch is huge ~10% w.r.t. 19.11-rc1 base on
> NXP hardware.
> >
> > We cannot merge this. Anoob also reported performance issues on Marvell
> hardware.
> 
> Sure, 10% is a lot, so more than understandable.
> Though, I think we do need to decide our future goals for it.
> I see two main options here:
> 1.  Make lib code-path on par with legacy one in terms of performance,
>      deprecate and then remove legacy code-path.
>      Till that happen (deprecation/removal) to minimize code divergence,
>       forbid to add new features to legacy code path only.
>      New features should be added to both paths, or library code path.
> Obviously that one looks like a preferred option to me, but it requires some
> effort from all interested parties (Intel, NXP, Marvell, ...).
> If everyone is ok with it, then I think it would be good to have some draft
> timeline here.
> If you guys are not interested in this effort, then the only other approach I can
> think about:

[Anoob] I would say this is the right option. But then, there are features getting added only to lib ipsec mode. There are features like "fallback session" or anti replay which is only added for lib ipsec mode and not for non lib ipsec mode. Such features are good to have but would cost extra cycles in the datapath. Since it is only added for lib ipsec mode, the perf divergence between lib ipsec mode and non lib ipsec mode is fairly obvious.

So what is the solution? Both the modes need to be at par if our end strategy is going to deprecate one. If we need to reach a state where we can do apples to apples comparison, new features should be added to both modes and there should be a consensus on the feature implementations.

Also, looking at the "fallback session" feature, the feature was "pushed" without properly addressing the issues. The solution was hardly suitable for inline ipsec and the comments were ignored. Marvell is not ok with adding checks in datapath for an incomplete feature. Marvell withdrew the objections when it was conveyed that the review comments were deemed invaluable. Also because it was added to lib ipsec path which is not being used currently for our benchmarks. Marvell will be submitting an RFC for supporting fallback session with few changes in the rte_security library. When we attempt that, we can take care of only non lib ipsec mode as lib ipsec mode already has one and Marvell cannot work on two different fronts, when the other contributors themselves don't care about other established modes. 

Also considering that lib ipsec is under constant change, (and is still experimental), I would not be too excited about making it the default immediately. I see that there are usages supported by the non lib ipsec mode, which is not supported by lib ipsec mode.

For example, ipv4 & ipv6 using same SPI is valid for non lib ipsec, but not for lib ipsec. Because of this, the default conf doesn't even run when launched with lib ipsec mode. I'm not sure whether we should rush with making it the default when the whole idea is still "experimental".

> 2. split ipsec-secgw app into 2 (one using librte_ipsec, second using raw devices
> (legacy one)).
>     We probably can still try to keep some code shared by 2 apps:
>     (configuration/initialization/session management (SAD, SPD)),
>     but actual packet processing path will be different.
> I really don't like that option, but I think we need to come-up with clear
> decision, one way or another.
> 
> Thanks
> Konstantin
> 
> 
> 
> 
>
  

> > > > > > > >
> > > > > > > > 11/10/2019 14:40, Akhil Goyal:
> > > > > > > > > Hi All,
> > > > > > > > >
> > > > > > > > > This patchset would need ack from more vendors as it will
> > > > > > > > > impact user
> > > > > > > > experience
> > > > > > > > > on a key example application which is normally
> > > > > > > > > demonstrated to
> > > > > > > customers.
> > > > > > > > >
> > > > > > > > > IPSec library is still evolving and there are new
> > > > > > > > > functionality added every
> > > > > > > > release.
> > > > > > > > > Atleast from NXP side we are not OK with this change.
> > > > > > > >
> > > > > > > > What can be changed in the library to make it acceptable as
> > > > > > > > a default in this example?
> > > > > > > >
> > > > > > >
> > > > > > > We are observing performance issues with ipsec library. So
> > > > > > > would request other Vendors to confirm if they are OK with the
> > > > > > > performance
> > > > > numbers.
> > > > > >
> > > > > > Could you give some details on the performance issues you are seeing.
> > > > > >
> > > > >
> > > > > We were observing about 4-5% drop when using the ipsec-lib instead
> > > > > of the Legacy code path. We would again measure it on RC1. That is
> > > > > why I say, I will Hold this patch till RC2, unless some other vendor also
> > confirms that.
> > > >
> > > > Is there any update on performance measurements on 19.11-rc1 ?
> > > >
> > > The performance impact of this patch is huge ~10% w.r.t. 19.11-rc1 base on
> > NXP hardware.
> > >
> > > We cannot merge this. Anoob also reported performance issues on Marvell
> > hardware.
> >
> > Sure, 10% is a lot, so more than understandable.
> > Though, I think we do need to decide our future goals for it.
> > I see two main options here:
> > 1.  Make lib code-path on par with legacy one in terms of performance,
> >      deprecate and then remove legacy code-path.
> >      Till that happen (deprecation/removal) to minimize code divergence,
> >       forbid to add new features to legacy code path only.
> >      New features should be added to both paths, or library code path.
> > Obviously that one looks like a preferred option to me, but it requires some
> > effort from all interested parties (Intel, NXP, Marvell, ...).
> > If everyone is ok with it, then I think it would be good to have some draft
> > timeline here.
> > If you guys are not interested in this effort, then the only other approach I can
> > think about:
> 
> [Anoob] I would say this is the right option. But then, there are features getting added only to lib ipsec mode. There are features like
> "fallback session" or anti replay which is only added for lib ipsec mode and not for non lib ipsec mode. Such features are good to have
> but would cost extra cycles in the datapath. Since it is only added for lib ipsec mode, the perf divergence between lib ipsec mode and
> non lib ipsec mode is fairly obvious.

AFAIK, all these features are optional and can be disabled.
For performance comparison, I'd expect we run lib mode with extra features disabled. 

> 
> So what is the solution? Both the modes need to be at par if our end strategy is going to deprecate one. If we need to reach a state
> where we can do apples to apples comparison, new features should be added to both modes and there should be a consensus on the
> feature implementations.
> 
> Also, looking at the "fallback session" feature, the feature was "pushed" without properly addressing the issues. The solution was
> hardly suitable for inline ipsec and the comments were ignored. Marvell is not ok with adding checks in datapath for an incomplete
> feature. Marvell withdrew the objections when it was conveyed that the review comments were deemed invaluable. Also because it
> was added to lib ipsec path which is not being used currently for our benchmarks. Marvell will be submitting an RFC for supporting
> fallback session with few changes in the rte_security library. When we attempt that, we can take care of only non lib ipsec mode as lib
> ipsec mode already has one and Marvell cannot work on two different fronts, when the other contributors themselves don't care
> about other established modes.
> 
> Also considering that lib ipsec is under constant change, (and is still experimental), I would not be too excited about making it the
> default immediately. I see that there are usages supported by the non lib ipsec mode, which is not supported by lib ipsec mode.
> 
> For example, ipv4 & ipv6 using same SPI is valid for non lib ipsec, but not for lib ipsec. Because of this, the default conf doesn't even
> run when launched with lib ipsec mode. I'm not sure whether we should rush with making it the default when the whole idea is still
> "experimental".
> 
> > 2. split ipsec-secgw app into 2 (one using librte_ipsec, second using raw devices
> > (legacy one)).
> >     We probably can still try to keep some code shared by 2 apps:
> >     (configuration/initialization/session management (SAD, SPD)),
> >     but actual packet processing path will be different.
> > I really don't like that option, but I think we need to come-up with clear
> > decision, one way or another.
> >
> > Thanks
> > Konstantin
> >
> >
> >
> >
> >