Re: [Buildroot] [PATCH 1/1] package/dpdk: add 24.03

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello Vincent,

Thanks for your patch, nice to see a Buildroot patch from you! See some
comments below.

On Thu, 15 Aug 2024 23:26:58 +0200
Vincent Jardin <vjardin@free.fr> wrote:

> Importantly, this commit does not enforce the use of UIO or VFIO
> kernel frameworks, as DPDK's architecture supports userland memory
> mappings that do not require these technologies. By maintaining this
> flexibility, DPDK can operate with a broader range of hardware and
> software configurations, making it suitable for diverse Buildroot's
> deployment scenarios.

Do you intend to follow-up with additional patches supporting the other
use-cases?

> Signed-off-by: Vincent Jardin <vjardin@free.fr>
> ---
>  package/Config.in      |  1 +
>  package/dpdk/Config.in | 14 ++++++++++++++
>  package/dpdk/dpdk.hash |  1 +
>  package/dpdk/dpdk.mk   | 39 +++++++++++++++++++++++++++++++++++++++
>  4 files changed, 55 insertions(+)

For all new packages, we require an entry in the DEVELOPERS file to be
added.

> diff --git a/package/dpdk/Config.in b/package/dpdk/Config.in
> new file mode 100644
> index 0000000000..56dcb42a33
> --- /dev/null
> +++ b/package/dpdk/Config.in
> @@ -0,0 +1,14 @@
> +config BR2_PACKAGE_DPDK
> +	bool "dpdk"
> +	depends on BR2_TOOLCHAIN_HAS_THREADS # glibc or uClibc toolchain required

The comment does not make sense: the only library that can be compiled
without thread support is uClibc. glibc and musl both always have
thread support.

> +	select BR2_PACKAGE_HOST_PYTHON_PYELFTOOLS

Not needed, we typically don't select host packages.

> +	select BR2_PACKAGE_LIBBSD

When you "select" an option, you need to replicate its "depends on", so
in this case, you need to add:

        depends on BR2_PACKAGE_LIBBSD_ARCH_SUPPORTS # libbsd
        depends on !BR2_STATIC_LIBS # libbsd
        depends on BR2_TOOLCHAIN_HAS_THREADS # libbsd
        depends on BR2_USE_WCHAR # libbsd

> +	select BR2_PACKAGE_LIBEXECINFO

Selecting libexecinfo only makes sense for non glibc toolchains, so
this should be:

	select BR2_PACKAGE_LIBEXECINFO if !BR2_TOOLCHAIN_USES_GLIBC

also, you need to "depends on BR2_USE_WCHAR", or rather because you
already have it due to libbsd, you need to do:

	depends on BR2_USE_WCHAR # libbsd, libexecinfo

> +	select BR2_PACKAGE_JANSSON
> +	select BR2_PACKAGE_LIBPCAP
> +	select BR2_PACKAGE_ZLIB
> +	help
> +	  DPDK (Data Plane Development Kit) is a set of libraries
> +	  and drivers for fast packet processing.
> +
> +	  http://dpdk.org/

You need to add a Config.in comment about the dependencies:

comment "dpdk needs a toolchain w/ dynamic library, threads, wchar"
	depends on BR2_PACKAGE_LIBBSD_ARCH_SUPPORTS
	depends on BR2_STATIC_LIBS || !BR2_TOOLCHAIN_HAS_THREADS || !BR2_USE_WCHAR

> diff --git a/package/dpdk/dpdk.hash b/package/dpdk/dpdk.hash
> new file mode 100644
> index 0000000000..fd8ab5a6aa
> --- /dev/null
> +++ b/package/dpdk/dpdk.hash
> @@ -0,0 +1 @@

Please indicate where the hash comes from, in a comment. Calculated
locally? Verified with some upstream provided hash? Crypto signature?
See other .hash files for example.

> +sha256  33ed973b3945af4f5923096ddca250b401dc80be3b5c6393b4e4fe43a1a6c2ad  dpdk-24.03.tar.xz

Also, please add the hashes of the license files.

> diff --git a/package/dpdk/dpdk.mk b/package/dpdk/dpdk.mk
> new file mode 100644
> index 0000000000..56adcf1d00
> --- /dev/null
> +++ b/package/dpdk/dpdk.mk
> @@ -0,0 +1,39 @@
> +################################################################################
> +#
> +# DPDK

lower case

> +#
> +################################################################################
> +
> +# DPDK_VERSION = main
> +# DPDK_SITE = https://dpdk.org/git/dpdk
> +# DPDK_SITE_METHOD = git

Please drop those 3 lines.

> +DPDK_VERSION ?= 24.03

Please turn ?= into just =.

> +DPDK_SOURCE = dpdk-$(DPDK_VERSION).tar.xz
> +DPDK_SITE = http://fast.dpdk.org/rel
> +DPDK_LICENSE = BSD-3-Clause
> +DPDK_LICENSE_FILES = license/bsd-3-clause.txt license/README license/bsd-2-clause.txt license/exceptions.txt license/gpl-2.0.txt license/isc.txt license/lgpl-2.1.txt license/mit.txt

How can the license be just BSD-3-Clause when there are license files
for BSD-2-Clause, GPL-2.0, ISC, LGPL-2.1, MIT, and some exceptions?

Also, since the LICENSE_FILES variable is long, please format it as
such:

DPDK_LICENSE_FILES = \
	license/bsd-3-clause.txt \
	... \
	... \

> +DPDK_DEPENDENCIES += host-pkgconf
> +DPDK_DEPENDENCIES += host-python-pyelftools
> +DPDK_DEPENDENCIES += libbsd
> +DPDK_DEPENDENCIES += libexecinfo
> +DPDK_DEPENDENCIES += jansson
> +DPDK_DEPENDENCIES += libpcap
> +DPDK_DEPENDENCIES += zlib

Just one assignment, and use = instead of += since these are
unconditional:

DPDK_DEPENDENCIES = \
	host-pkgconf \
	host-python-pyelftools \
	libbsd \
	...

Please add libexecinfo in the dependencies only if
BR2_TOOLCHAIN_USES_GLIBC is false:

ifeq ($(BR2_TOOLCHAIN_USES_GLIBC),)
DPDK_DEPENDENCIES += libexecinfo
endif

> +#not yet DPDK_DEPENDENCIES += openssl
> +#not yet DPDK_DEPENDENCIES += libbpf

Please drop commented code.

> +
> +DPDK_MARCH = $(BR2_ARCH)

Are you sure all values of $(BR2_ARCH) are supported as
-Dcpu_instruction_set values? What is -Dcpu_instruction_set actually
doing?

> +DPDK_MTUNE = $(BR2_ARCH) # not used yet

Please drop if it's not used.

> +GCC_TARGET_CPU=$(BR2_GCC_TARGET_ARCH)

Please drop, GCC_TARGET_CPU does not belong to this package.

> +# see meson_options.txt from DPDK

Comment not needed.

> +#
> +DPDK_CONF_OPTS += -Ddeveloper_mode=enabled

What does it do? Are we sure we want it enabled unconditionally?

> +DPDK_CONF_OPTS += -Dcpu_instruction_set=$(DPDK_MARCH)
> +
> +# platform can be: native, all, cn9k, cn10k
> +DPDK_CONF_OPTS += -Dplatform=generic

Well, the comment says that platform can be "native", "all", "cn9k",
"cn10k"... and you're setting it to "generic". Doesn't really make
sense.

Also, isn't libpcap only needed for the "generic" platform? Can one
select only one platform, or a list of platforms?

If one can only select one platform, then maybe we need to have a
choice..endchoice in Config.in:

choice
	prompt "Platform"
	default BR2_PACKAGE_DPDK_PLATFORM_GENERIC

config BR2_PACKAGE_DPDK_PLATFORM_GENERIC
	bool "generic"
	select BR2_PACKAGE_LIBPCAP

endchoice

which of course can be extended later with additional platforms.

Could you rework your patch according to those suggestions? Thanks a
lot!

Thanks!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot