| Message ID | 1503679232-11135-2-git-send-email-ian.stokes@intel.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Fri, Aug 25, 2017 at 05:40:23PM +0100, Ian Stokes wrote: > LibIpsecMB is required to enable the use of vdev cryptodev devices in > DPDK. This patch adds a condition to check for the library when it is > detected that ONFIG_RTE_LIBRTE_PMD_AESNI_MB=y is enabled in the DPDK > config. > > Signed-off-by: Ian Stokes <ian.stokes@intel.com> > --- > acinclude.m4 | 13 +++++++++++++ > 1 files changed, 13 insertions(+), 0 deletions(-) > > diff --git a/acinclude.m4 b/acinclude.m4 > index aeb594a..8c14367 100644 > --- a/acinclude.m4 > +++ b/acinclude.m4 > @@ -271,6 +271,19 @@ AC_DEFUN([OVS_CHECK_DPDK], [ > ], [], > [AC_DEFINE([DPDK_PDUMP], [1], [DPDK pdump enabled in OVS.])]) > ]) > + AC_COMPILE_IFELSE([ > + AC_LANG_PROGRAM( > + [ > + #include <rte_config.h> > +#if RTE_LIBRTE_PMD_AESNI_MB > +#error > +#endif > + ], []) > + ], [], > + [AC_SEARCH_LIBS([init_mb_mgr_sse],[IPSec_MB],[],[AC_MSG_ERROR([unable to find lib_IPSec_MB in ${LDFLAGS}, install the dependency package])]) > + DPDK_EXTRA_LIB="-lIPSec_MB" > + AC_DEFINE([PMD_AESNI_MB], [1], [PMD_AESNI_MB support detected in DPDK.])]) > + It is a little unusual to make a test fail when a feature (RTE_LIBRTE_PMD_AESNI_MB in this case) is detected. This makes the feature prone to being detected if something unrelated fails in the toolchain. Usually, one would either use the opposite approach--that is, fail if RTE_LIBRTE_PMD_AESNI_MB is not declared--or something based on AC_CHECK_DECL or AC_LINK_IFELSE using some symbol that RTE_LIBRTE_PMD_AESNI_MB makes available. Also, I recommend adding an explanation of this dependency to the documentation, otherwise this will be confusing to users. Thanks, Ben.
> On Fri, Aug 25, 2017 at 05:40:23PM +0100, Ian Stokes wrote: > > LibIpsecMB is required to enable the use of vdev cryptodev devices in > > DPDK. This patch adds a condition to check for the library when it is > > detected that ONFIG_RTE_LIBRTE_PMD_AESNI_MB=y is enabled in the DPDK > > config. > > > > Signed-off-by: Ian Stokes <ian.stokes@intel.com> > > --- > > acinclude.m4 | 13 +++++++++++++ > > 1 files changed, 13 insertions(+), 0 deletions(-) > > > > diff --git a/acinclude.m4 b/acinclude.m4 index aeb594a..8c14367 100644 > > --- a/acinclude.m4 > > +++ b/acinclude.m4 > > @@ -271,6 +271,19 @@ AC_DEFUN([OVS_CHECK_DPDK], [ > > ], [], > > [AC_DEFINE([DPDK_PDUMP], [1], [DPDK pdump enabled in OVS.])]) > > ]) > > + AC_COMPILE_IFELSE([ > > + AC_LANG_PROGRAM( > > + [ > > + #include <rte_config.h> > > +#if RTE_LIBRTE_PMD_AESNI_MB > > +#error > > +#endif > > + ], []) > > + ], [], > > + > [AC_SEARCH_LIBS([init_mb_mgr_sse],[IPSec_MB],[],[AC_MSG_ERROR([unable to > find lib_IPSec_MB in ${LDFLAGS}, install the dependency package])]) > > + DPDK_EXTRA_LIB="-lIPSec_MB" > > + AC_DEFINE([PMD_AESNI_MB], [1], [PMD_AESNI_MB support detected > > +in DPDK.])]) > > + > > It is a little unusual to make a test fail when a feature > (RTE_LIBRTE_PMD_AESNI_MB in this case) is detected. This makes the > feature prone to being detected if something unrelated fails in the > toolchain. Usually, one would either use the opposite approach--that is, > fail if RTE_LIBRTE_PMD_AESNI_MB is not declared--or something based on > AC_CHECK_DECL or AC_LINK_IFELSE using some symbol that > RTE_LIBRTE_PMD_AESNI_MB makes available. Thanks Ben, Looking at this your right, this was my first foray into modifying the acinclude to include an external library, but I take you point, I'll clean this up in the RFC v3. On a side note, are you ok with a separate library being required to enable a feature? Part of this patch was to gather feedback on this, that a user would have to download compile and populate the IPsec library for DPDK. The knock on effect being that the crypto dev functionality can then be used in OVS DPDK can also use that functionality then. > > Also, I recommend adding an explanation of this dependency to the > documentation, otherwise this will be confusing to users. I think the dependency for IPsec MB is added to the DPDK install section in this patchset, if not I'll definitely add it. > > Thanks, > > Ben.
On Wed, Nov 01, 2017 at 04:01:10PM +0000, Stokes, Ian wrote: > > On Fri, Aug 25, 2017 at 05:40:23PM +0100, Ian Stokes wrote: > > > LibIpsecMB is required to enable the use of vdev cryptodev devices in > > > DPDK. This patch adds a condition to check for the library when it is > > > detected that ONFIG_RTE_LIBRTE_PMD_AESNI_MB=y is enabled in the DPDK > > > config. > > > > > > Signed-off-by: Ian Stokes <ian.stokes@intel.com> > > > --- > > > acinclude.m4 | 13 +++++++++++++ > > > 1 files changed, 13 insertions(+), 0 deletions(-) > > > > > > diff --git a/acinclude.m4 b/acinclude.m4 index aeb594a..8c14367 100644 > > > --- a/acinclude.m4 > > > +++ b/acinclude.m4 > > > @@ -271,6 +271,19 @@ AC_DEFUN([OVS_CHECK_DPDK], [ > > > ], [], > > > [AC_DEFINE([DPDK_PDUMP], [1], [DPDK pdump enabled in OVS.])]) > > > ]) > > > + AC_COMPILE_IFELSE([ > > > + AC_LANG_PROGRAM( > > > + [ > > > + #include <rte_config.h> > > > +#if RTE_LIBRTE_PMD_AESNI_MB > > > +#error > > > +#endif > > > + ], []) > > > + ], [], > > > + > > [AC_SEARCH_LIBS([init_mb_mgr_sse],[IPSec_MB],[],[AC_MSG_ERROR([unable to > > find lib_IPSec_MB in ${LDFLAGS}, install the dependency package])]) > > > + DPDK_EXTRA_LIB="-lIPSec_MB" > > > + AC_DEFINE([PMD_AESNI_MB], [1], [PMD_AESNI_MB support detected > > > +in DPDK.])]) > > > + > > > > It is a little unusual to make a test fail when a feature > > (RTE_LIBRTE_PMD_AESNI_MB in this case) is detected. This makes the > > feature prone to being detected if something unrelated fails in the > > toolchain. Usually, one would either use the opposite approach--that is, > > fail if RTE_LIBRTE_PMD_AESNI_MB is not declared--or something based on > > AC_CHECK_DECL or AC_LINK_IFELSE using some symbol that > > RTE_LIBRTE_PMD_AESNI_MB makes available. > > Thanks Ben, Looking at this your right, this was my first foray into modifying the acinclude to include an external library, but I take you point, I'll clean this up in the RFC v3. > > On a side note, are you ok with a separate library being required to enable a feature? > Part of this patch was to gather feedback on this, that a user would have to download compile and populate the IPsec library for DPDK. > The knock on effect being that the crypto dev functionality can then be used in OVS DPDK can also use that functionality then. I guess it's not reasonable for OVS to add this functionality in-tree, so a separate library is probably the only choice. To my mind, the bigger question is, should the library be optional or required? It is confusing to end users if they have to be aware of all of the list of features that are built or not built into their binary, so it's nice if a library can be required. On the other hand, sometimes that requirement puts an unreasonable burden on developers or packagers, so it's necessary to have some balance. > > Also, I recommend adding an explanation of this dependency to the > > documentation, otherwise this will be confusing to users. > > I think the dependency for IPsec MB is added to the DPDK install section in this patchset, if not I'll definitely add it. OK.
diff --git a/acinclude.m4 b/acinclude.m4 index aeb594a..8c14367 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -271,6 +271,19 @@ AC_DEFUN([OVS_CHECK_DPDK], [ ], [], [AC_DEFINE([DPDK_PDUMP], [1], [DPDK pdump enabled in OVS.])]) ]) + AC_COMPILE_IFELSE([ + AC_LANG_PROGRAM( + [ + #include <rte_config.h> +#if RTE_LIBRTE_PMD_AESNI_MB +#error +#endif + ], []) + ], [], + [AC_SEARCH_LIBS([init_mb_mgr_sse],[IPSec_MB],[],[AC_MSG_ERROR([unable to find lib_IPSec_MB in ${LDFLAGS}, install the dependency package])]) + DPDK_EXTRA_LIB="-lIPSec_MB" + AC_DEFINE([PMD_AESNI_MB], [1], [PMD_AESNI_MB support detected in DPDK.])]) + # On some systems we have to add -ldl to link with dpdk #
LibIpsecMB is required to enable the use of vdev cryptodev devices in DPDK. This patch adds a condition to check for the library when it is detected that ONFIG_RTE_LIBRTE_PMD_AESNI_MB=y is enabled in the DPDK config. Signed-off-by: Ian Stokes <ian.stokes@intel.com> --- acinclude.m4 | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)