Re: reg. https://patches.dpdk.org/project/dpdk/list/?series=31200 - patch result

Re: reg. https://patches.dpdk.org/project/dpdk/list/?series=31200 - patch result

[Aaron Conole]

"Power, Ciara" <ciara.power@intel.com> writes:

> + Patrick
>
>  
>
> From: Power, Ciara 
> Sent: Tuesday, March 5, 2024 10:05 AM
> To: Sivaramakrishnan, VenkatX <venkatx.sivaramakrishnan@intel.com>; Akhil Goyal <gakhil@marvell.com>
> Cc: Ji, Kai <kai.ji@intel.com>; Aaron Conole <aconole@redhat.com>
> Subject: RE: reg. https://patches.dpdk.org/project/dpdk/list/?series=31200 - patch result
>
>  
>
> Hi folks,
>
>  
>
> Had a quick look, I can also see this:
>
> crypto/ipsec_mb: IPSec_MB version >= 1.4.0 is required, found version 1.2.0

This version of ipsec_mb is less than 1 year old.  Did this pass any
other CI testing?  I would be surprised if it did - I'm not sure any
downstream environments that would be using it already.

> I guess the installed PMD .so file isn’t created because they are not compiled in, due to the minimum version on
> environment not meeting the new requirements.

I don't see any such new requirements anywhere on the crypto tree.  The
only change I know about was for QAT to try and default to IPSec_MB 1.4,
but it is supposed to fall back to OpenSSL if that is unavailable.  Did
this change?

> CC’ing Aaron, who might know about upgrading that environment to ipsec-mb v1.4.
>
>  
>
> Thanks,
>
> Ciara
>
>  
>
> From: Sivaramakrishnan, VenkatX <venkatx.sivaramakrishnan@intel.com> 
> Sent: Monday, March 4, 2024 10:45 AM
> To: Akhil Goyal <gakhil@marvell.com>
> Cc: Ji, Kai <kai.ji@intel.com>; Power, Ciara <ciara.power@intel.com>
> Subject: reg. https://patches.dpdk.org/project/dpdk/list/?series=31200 - patch result
>
>  
>
> Hi Akhil,
>
>  
>
> I would like to provide details of the failures
>
>  
>
>  
>
> *
>
>  
>
>  
>
> Failures details:
>
> ============
>
> "Build and test" failed for "librte_crypto_ipsec_mb.so". 
>
> doc: remove outdated version details · ovsrobot/dpdk@f40ab34 (github.com)
>
> Error: cannot find librte_crypto_ipsec_mb.so.24.0 in install
>
>  
>
> Looks like, “ipsec mb” was not installed on the server. 
>
>  
>
> However, the patch changes are related to the Doc update. Hope that this will not impact patch merging 
>
>  
>
>                Thank you.
>
>  
>
> Best Regards,
>
> Venkat.

RE: reg. https://patches.dpdk.org/project/dpdk/list/?series=31200 - patch result

[Power, Ciara]

[-- Attachment #1: Type: text/plain, Size: 2339 bytes --]

Hi Aaron,

> -----Original Message-----
> From: Aaron Conole <aconole@redhat.com>
> Sent: Tuesday, March 5, 2024 6:37 PM
> To: Power, Ciara <ciara.power@intel.com>
> Cc: Sivaramakrishnan, VenkatX <venkatx.sivaramakrishnan@intel.com>; Akhil
> Goyal <gakhil@marvell.com>; Ji, Kai <kai.ji@intel.com>; probb@iol.unh.edu;
> dev@dpdk.org
> Subject: Re: reg. https://patches.dpdk.org/project/dpdk/list/?series=31200 -
> patch result
>
> "Power, Ciara" <ciara.power@intel.com> writes:
>
> > + Patrick
> >
> >
> >
> > From: Power, Ciara
> > Sent: Tuesday, March 5, 2024 10:05 AM
> > To: Sivaramakrishnan, VenkatX <venkatx.sivaramakrishnan@intel.com>;
> > Akhil Goyal <gakhil@marvell.com>
> > Cc: Ji, Kai <kai.ji@intel.com>; Aaron Conole <aconole@redhat.com>
> > Subject: RE: reg.
> > https://patches.dpdk.org/project/dpdk/list/?series=31200 - patch
> > result
> >
> >
> >
> > Hi folks,
> >
> >
> >
> > Had a quick look, I can also see this:
> >
> > crypto/ipsec_mb: IPSec_MB version >= 1.4.0 is required, found version
> > 1.2.0
>
> This version of ipsec_mb is less than 1 year old.  Did this pass any other CI
> testing?  I would be surprised if it did - I'm not sure any downstream
> environments that would be using it already.

We have been using 1.4 (and even 1.5 since it was released) for internal regression testing and development.
Other than that, the library would be tested by Intel-ipsec-mb team directly.
1.4 has been supported by the ipsec-mb SW PMDs since it was released, but now we would like to make it the required version,
to remove the various ifdef codepaths in PMD, and use the newer, more performant version of the library.



>
> > I guess the installed PMD .so file isn’t created because they are not
> > compiled in, due to the minimum version on environment not meeting the
> new requirements.
>
> I don't see any such new requirements anywhere on the crypto tree.  The only
> change I know about was for QAT to try and default to IPSec_MB 1.4, but it is
> supposed to fall back to OpenSSL if that is unavailable.  Did this change?

This patchset introduces the requirement, it is not yet on the crypto tree.
It is a SW PMD change only - currently they require 1.1 ipsec-mb, but we want to bump that to 1.4.
QAT dependencies are unchanged.

Thanks,
Ciara

[-- Attachment #2: Type: text/html, Size: 3305 bytes --]

Re: reg. https://patches.dpdk.org/project/dpdk/list/?series=31200 - patch result

[Aaron Conole]

"Power, Ciara" <ciara.power@intel.com> writes:

> Hi Aaron,
>
>> -----Original Message-----
>> From: Aaron Conole <aconole@redhat.com>
>> Sent: Tuesday, March 5, 2024 6:37 PM
>> To: Power, Ciara <ciara.power@intel.com>
>> Cc: Sivaramakrishnan, VenkatX <venkatx.sivaramakrishnan@intel.com>; Akhil
>> Goyal <gakhil@marvell.com>; Ji, Kai <kai.ji@intel.com>; probb@iol.unh.edu;
>> dev@dpdk.org
>> Subject: Re: reg. https://patches.dpdk.org/project/dpdk/list/?series=31200 -
>> patch result
>> 
>> "Power, Ciara" <ciara.power@intel.com> writes:
>> 
>> > + Patrick
>> >
>> >
>> >
>> > From: Power, Ciara
>> > Sent: Tuesday, March 5, 2024 10:05 AM
>> > To: Sivaramakrishnan, VenkatX <venkatx.sivaramakrishnan@intel.com>;
>> > Akhil Goyal <gakhil@marvell.com>
>> > Cc: Ji, Kai <kai.ji@intel.com>; Aaron Conole <aconole@redhat.com>
>> > Subject: RE: reg.
>> > https://patches.dpdk.org/project/dpdk/list/?series=31200 - patch
>> > result
>> >
>> >
>> >
>> > Hi folks,
>> >
>> >
>> >
>> > Had a quick look, I can also see this:
>> >
>> > crypto/ipsec_mb: IPSec_MB version >= 1.4.0 is required, found version
>> > 1.2.0
>> 
>> This version of ipsec_mb is less than 1 year old.  Did this pass any other CI
>> testing?  I would be surprised if it did - I'm not sure any downstream
>> environments that would be using it already.
>
> We have been using 1.4 (and even 1.5 since it was released) for internal regression testing and development.
> Other than that, the library would be tested by Intel-ipsec-mb team directly.
> 1.4 has been supported by the ipsec-mb SW PMDs since it was released, but now we would like to make it the required version,
> to remove the various ifdef codepaths in PMD, and use the newer, more performant version of the library.

While that is a good goal, this patch series would cause build issues on
some distributions (which is evident from the CI failures), and that
there are new requirements isn't as clearly documented.

AFAICT, it also shifts the requirements from either OpenSSL or IPSec-MB
to IPSec-MB.  Did I understand it correctly?

>> 
>> > I guess the installed PMD .so file isn’t created because they are not
>> > compiled in, due to the minimum version on environment not meeting the
>> new requirements.
>> 
>> I don't see any such new requirements anywhere on the crypto tree.  The only
>> change I know about was for QAT to try and default to IPSec_MB 1.4, but it is
>> supposed to fall back to OpenSSL if that is unavailable.  Did this change?
>
> This patchset introduces the requirement, it is not yet on the crypto tree.
> It is a SW PMD change only - currently they require 1.1 ipsec-mb, but we want to bump that to 1.4.
> QAT dependencies are unchanged.

In that case I think there needs to be some additional communications /
announcements since it is changing a dependency.

> Thanks,
> Ciara

RE: reg. https://patches.dpdk.org/project/dpdk/list/?series=31200 - patch result

[Power, Ciara]

Hi Aaron,

> -----Original Message-----
> From: Aaron Conole <aconole@redhat.com>
> Sent: Wednesday, March 6, 2024 2:57 PM
> To: Power, Ciara <ciara.power@intel.com>
> Cc: Sivaramakrishnan, VenkatX <venkatx.sivaramakrishnan@intel.com>; Akhil
> Goyal <gakhil@marvell.com>; Ji, Kai <kai.ji@intel.com>; probb@iol.unh.edu;
> dev@dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
> Thomas Monjalon <thomas@monjalon.net>; David Marchand
> <dmarchan@redhat.com>; Kevin Traynor <ktraynor@redhat.com>
> Subject: Re: reg. https://patches.dpdk.org/project/dpdk/list/?series=31200 -
> patch result
> 
> "Power, Ciara" <ciara.power@intel.com> writes:
> 
> > Hi Aaron,
> >
> >> -----Original Message-----
> >> From: Aaron Conole <aconole@redhat.com>
> >> Sent: Tuesday, March 5, 2024 6:37 PM
> >> To: Power, Ciara <ciara.power@intel.com>
> >> Cc: Sivaramakrishnan, VenkatX <venkatx.sivaramakrishnan@intel.com>;
> >> Akhil Goyal <gakhil@marvell.com>; Ji, Kai <kai.ji@intel.com>;
> >> probb@iol.unh.edu; dev@dpdk.org
> >> Subject: Re: reg.
> >> https://patches.dpdk.org/project/dpdk/list/?series=31200 - patch
> >> result
> >>
> >> "Power, Ciara" <ciara.power@intel.com> writes:
> >>
> >> > + Patrick
> >> >
> >> >
> >> >
> >> > From: Power, Ciara
> >> > Sent: Tuesday, March 5, 2024 10:05 AM
> >> > To: Sivaramakrishnan, VenkatX <venkatx.sivaramakrishnan@intel.com>;
> >> > Akhil Goyal <gakhil@marvell.com>
> >> > Cc: Ji, Kai <kai.ji@intel.com>; Aaron Conole <aconole@redhat.com>
> >> > Subject: RE: reg.
> >> > https://patches.dpdk.org/project/dpdk/list/?series=31200 - patch
> >> > result
> >> >
> >> >
> >> >
> >> > Hi folks,
> >> >
> >> >
> >> >
> >> > Had a quick look, I can also see this:
> >> >
> >> > crypto/ipsec_mb: IPSec_MB version >= 1.4.0 is required, found
> >> > version
> >> > 1.2.0
> >>
> >> This version of ipsec_mb is less than 1 year old.  Did this pass any
> >> other CI testing?  I would be surprised if it did - I'm not sure any
> >> downstream environments that would be using it already.
> >
> > We have been using 1.4 (and even 1.5 since it was released) for internal
> regression testing and development.
> > Other than that, the library would be tested by Intel-ipsec-mb team directly.
> > 1.4 has been supported by the ipsec-mb SW PMDs since it was released,
> > but now we would like to make it the required version, to remove the various
> ifdef codepaths in PMD, and use the newer, more performant version of the
> library.
> 
> While that is a good goal, this patch series would cause build issues on some
> distributions (which is evident from the CI failures), and that there are new
> requirements isn't as clearly documented.

Yes, the CI failures need to be resolved, some questions/discussion on that in the other thread.

Usually, for a version bump we would have documented in PMD documentation, and the release notes.
We did similar before when moving from 0.53 to 1.0 as part of this work previously for 21.11:
https://github.com/DPDK/dpdk/commit/c75542ae42000062b55cb03643575cd13b66aeaf
https://github.com/DPDK/dpdk/commit/918fd2f1466b0e3b21a033df7012a77a83665582


> 
> AFAICT, it also shifts the requirements from either OpenSSL or IPSec-MB to
> IPSec-MB.  Did I understand it correctly?

No, bumping the ipsec-mb version from 1.1 to 1.4 for the ipsec-mb SW PMDs doesn't involve any Openssl dependency changes - these SW PMDs have only ever used the ipsec-mb library.
This would affect snow3g, Kasumi, zuc, aesni_mb, aesni_gcm and chachapoly PMDs - they would need v1.4 to be compiled.


> 
> >>
> >> > I guess the installed PMD .so file isn’t created because they are
> >> > not compiled in, due to the minimum version on environment not
> >> > meeting the
> >> new requirements.
> >>
> >> I don't see any such new requirements anywhere on the crypto tree.
> >> The only change I know about was for QAT to try and default to
> >> IPSec_MB 1.4, but it is supposed to fall back to OpenSSL if that is unavailable.
> Did this change?
> >
> > This patchset introduces the requirement, it is not yet on the crypto tree.
> > It is a SW PMD change only - currently they require 1.1 ipsec-mb, but we want
> to bump that to 1.4.
> > QAT dependencies are unchanged.
> 
> In that case I think there needs to be some additional communications /
> announcements since it is changing a dependency.

Sure, we can look to do that if needed.
Apart from the release notes + PMD doc changes, what other comms/announcements
do you suggest we do to bump the ipsec-mb PMD version from 1.0 to 1.4?

 
Thanks,
Ciara

Re: reg. https://patches.dpdk.org/project/dpdk/list/?series=31200 - patch result

[Aaron Conole]

"Power, Ciara" <ciara.power@intel.com> writes:

> Hi Aaron,
>
>> -----Original Message-----
>> From: Aaron Conole <aconole@redhat.com>
>> Sent: Wednesday, March 6, 2024 2:57 PM
>> To: Power, Ciara <ciara.power@intel.com>
>> Cc: Sivaramakrishnan, VenkatX <venkatx.sivaramakrishnan@intel.com>; Akhil
>> Goyal <gakhil@marvell.com>; Ji, Kai <kai.ji@intel.com>; probb@iol.unh.edu;
>> dev@dpdk.org; De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>;
>> Thomas Monjalon <thomas@monjalon.net>; David Marchand
>> <dmarchan@redhat.com>; Kevin Traynor <ktraynor@redhat.com>
>> Subject: Re: reg. https://patches.dpdk.org/project/dpdk/list/?series=31200 -
>> patch result
>> 
>> "Power, Ciara" <ciara.power@intel.com> writes:
>> 
>> > Hi Aaron,
>> >
>> >> -----Original Message-----
>> >> From: Aaron Conole <aconole@redhat.com>
>> >> Sent: Tuesday, March 5, 2024 6:37 PM
>> >> To: Power, Ciara <ciara.power@intel.com>
>> >> Cc: Sivaramakrishnan, VenkatX <venkatx.sivaramakrishnan@intel.com>;
>> >> Akhil Goyal <gakhil@marvell.com>; Ji, Kai <kai.ji@intel.com>;
>> >> probb@iol.unh.edu; dev@dpdk.org
>> >> Subject: Re: reg.
>> >> https://patches.dpdk.org/project/dpdk/list/?series=31200 - patch
>> >> result
>> >>
>> >> "Power, Ciara" <ciara.power@intel.com> writes:
>> >>
>> >> > + Patrick
>> >> >
>> >> >
>> >> >
>> >> > From: Power, Ciara
>> >> > Sent: Tuesday, March 5, 2024 10:05 AM
>> >> > To: Sivaramakrishnan, VenkatX <venkatx.sivaramakrishnan@intel.com>;
>> >> > Akhil Goyal <gakhil@marvell.com>
>> >> > Cc: Ji, Kai <kai.ji@intel.com>; Aaron Conole <aconole@redhat.com>
>> >> > Subject: RE: reg.
>> >> > https://patches.dpdk.org/project/dpdk/list/?series=31200 - patch
>> >> > result
>> >> >
>> >> >
>> >> >
>> >> > Hi folks,
>> >> >
>> >> >
>> >> >
>> >> > Had a quick look, I can also see this:
>> >> >
>> >> > crypto/ipsec_mb: IPSec_MB version >= 1.4.0 is required, found
>> >> > version
>> >> > 1.2.0
>> >>
>> >> This version of ipsec_mb is less than 1 year old.  Did this pass any
>> >> other CI testing?  I would be surprised if it did - I'm not sure any
>> >> downstream environments that would be using it already.
>> >
>> > We have been using 1.4 (and even 1.5 since it was released) for internal
>> regression testing and development.
>> > Other than that, the library would be tested by Intel-ipsec-mb team directly.
>> > 1.4 has been supported by the ipsec-mb SW PMDs since it was released,
>> > but now we would like to make it the required version, to remove the various
>> ifdef codepaths in PMD, and use the newer, more performant version of the
>> library.
>> 
>> While that is a good goal, this patch series would cause build issues on some
>> distributions (which is evident from the CI failures), and that there are new
>> requirements isn't as clearly documented.
>
> Yes, the CI failures need to be resolved, some questions/discussion on
> that in the other thread.
>
> Usually, for a version bump we would have documented in PMD
> documentation, and the release notes.
> We did similar before when moving from 0.53 to 1.0 as part of this
> work previously for 21.11:
> https://github.com/DPDK/dpdk/commit/c75542ae42000062b55cb03643575cd13b66aeaf
> https://github.com/DPDK/dpdk/commit/918fd2f1466b0e3b21a033df7012a77a83665582
>
>
>> 
>> AFAICT, it also shifts the requirements from either OpenSSL or IPSec-MB to
>> IPSec-MB.  Did I understand it correctly?
>
> No, bumping the ipsec-mb version from 1.1 to 1.4 for the ipsec-mb SW
> PMDs doesn't involve any Openssl dependency changes - these SW PMDs
> have only ever used the ipsec-mb library.
> This would affect snow3g, Kasumi, zuc, aesni_mb, aesni_gcm and
> chachapoly PMDs - they would need v1.4 to be compiled.

Ahh, okay - thanks that makes sense to me.  I guess one issue is that
the current github robot will be based on 22.04, which is only shipping
with ipsec-mb 1.2.  I know that 23.10 includes ipsec-mb 1.4, but this is
not an LTS so we likely wouldn't support it.  24.04 (the next LTS to
release) will use 1.5, so when we move it shouldn't be an issue any
longer.

Perhaps we can look for a PPA or even build from scratch during the
build the ipsec-mb to link against.  This will at least give us the
right coverage.  I did a quick glance, but didn't see that there is any
kind of 'jammy' (the 22.04 code name) ppa which covers this version, but
maybe the approach is to set one up and then use it until we move to
24.04 in the future.

WDYT?

>
>> 
>> >>
>> >> > I guess the installed PMD .so file isn’t created because they are
>> >> > not compiled in, due to the minimum version on environment not
>> >> > meeting the
>> >> new requirements.
>> >>
>> >> I don't see any such new requirements anywhere on the crypto tree.
>> >> The only change I know about was for QAT to try and default to
>> >> IPSec_MB 1.4, but it is supposed to fall back to OpenSSL if that is unavailable.
>> Did this change?
>> >
>> > This patchset introduces the requirement, it is not yet on the crypto tree.
>> > It is a SW PMD change only - currently they require 1.1 ipsec-mb, but we want
>> to bump that to 1.4.
>> > QAT dependencies are unchanged.
>> 
>> In that case I think there needs to be some additional communications /
>> announcements since it is changing a dependency.
>
> Sure, we can look to do that if needed.
> Apart from the release notes + PMD doc changes, what other comms/announcements
> do you suggest we do to bump the ipsec-mb PMD version from 1.0 to 1.4?
>
>  
> Thanks,
> Ciara