[Buildroot] [PATCH v2 3/3] dpdk: new package

[Thomas Petazzoni <thomas.petazzoni@free-electrons.com>]

Jan,

On Wed,  9 Dec 2015 15:11:53 +0100, Jan Viktorin wrote:
> This patch introduces support of the DPDK library (www.dpdk.org) into
> Buildroot. DPDK is a library for high-speed packet sending/receiving
> while bypassing the Linux Kernel. It allows to reach a high throughput
> for 10-100 Gbps networks on the x86 platform.

Great! I'm not using DPDK myself, but I believe it's good to have this
supported in Buildroot.

> diff --git a/package/dpdk/Config.in b/package/dpdk/Config.in
> new file mode 100644
> index 0000000..a4935ae
> --- /dev/null
> +++ b/package/dpdk/Config.in
> @@ -0,0 +1,47 @@
> +config BR2_PACKAGE_DPDK
> +       bool "dpdk"
> +       depends on BR2_x86_i686 || BR2_x86_64 || BR2_powerpc_power8 || BR2_ARM_CPU_ARMV7A || BR2_aarch64 || BR2_aarch64_be

Why only BR2_x86_i686 ? I'm pretty sure it can work on many other 32
bits x86 CPUs. Did you want to say "at least i686" ?

Please wrap lines at 72 characters.

> +       depends on BR2_TOOLCHAIN_USES_GLIBC

You need a dependency on BR2_LINUX_KERNEL here, since your .mk file is
building some kernel modules.

> +       help
> +         DPDK is a set of libraries and drivers for fast packet processing. It was designed to run on
> +         any processors knowing Intel x86 has been the first CPU to be supported. Ports for other CPUs
> +         like IBM Power 8 and ARM are under progress. It runs mostly in Linux userland. A FreeBSD port
> +         is now available for a subset of DPDK features.

Wrapping please :)

> +
> +	 Notes:
> +	 * To build the included Linux Kernel drivers, it is necessary to enable CONFIG_PCI_MSI,
> +	   CONFIG_UIO.
> +	 * To build the PCAP PMD properly, you need to enable the libpcap manually.
> +	 * To install the python scripts (dpdk_nic_bind.py, cpu_layout.py), enable
> +	   the python2 interpreter for the target.
> +
> +         http://www.dpdk.org/
> +
> +if BR2_PACKAGE_DPDK
> +
> +config BR2_PACKAGE_DPDK_CONFIG
> +	string "Configuration"

Why do you make this a visible option? Is there any reason for the user
to be able to chnage this value ?

> +	default "i686-native-linuxapp-gcc" \
> +		if BR2_x86_i686
> +	default "x86_64-native-linuxapp-gcc" \
> +		if BR2_x86_64
> +	default "ppc_64-power8-native-linuxapp-gcc" \

So it's a PowerPC 64 configuration? BR2_powerpc_power8 by itself
doesn't say if it's 32 bits or 64 bits, so maybe you need
BR2_powerpc_power8 && BR2_ARCH_IS_64.

> +		if BR2_powerpc_power8

> diff --git a/package/dpdk/dpdk.hash b/package/dpdk/dpdk.hash
> new file mode 100644
> index 0000000..c78ec85
> --- /dev/null
> +++ b/package/dpdk/dpdk.hash
> @@ -0,0 +1,4 @@
> +# Locally calculated
> +sha256	f7b322867a45f99afd9c8fbacdc56e1621676f9ca0f046656ec85eb6a99a3440  dpdk-2.1.0.tar.gz
> +sha256	530074d4eaefe1f717e7411e6a74e4ba0fa619af304c5e74e1097e51d33cc19e  dpdk-2.2.0-rc2.tar.gz
> +sha256	7caf52554c0f724a09e9342ee6670b324a77dade5cd0b96ff5b66957ed1bc1f9  dpdk-2.2.0-rc3.tar.gz

You no longer offer a version choice, so there's no reason to have all
those hashes. Keep just the one matching the version used in dpdk.mk.

> +DPDK_VERSION = 2.2.0-rc3
> +DPDK_SITE = http://dpdk.org/browse/dpdk/snapshot
> +DPDK_SOURCE = dpdk-$(DPDK_VERSION).tar.gz
> +
> +DPDK_LICENSE = BSD

This is not sufficient, since as you said previously, it's not only
under BSD. Also BSD is not specific enough, it should be BSD-2c or
BSD-3c.

> +DPDK_LICENSE_FILES = LICENSE.LGPL LICENSE.GPL

So those contain the GPL and LGPL texts, but DPDK_LICENSE does not
reference the GPL and LGPL licenses. And the BSD license text is not
there. I believe this licensing information needs to be improved a bit.

> +DPDK_INSTALL_STAGING = YES
> +
> +DPDK_DEPENDENCIES += linux
> +
> +ifeq ($(BR2_PACKAGE_LIBPCAP),y)
> +DPDK_DEPENDENCIES += libpcap
> +endif
> +
> +ifeq ($(BR2_SHARED_LIBS),y)
> +define DPDK_ENABLE_SHARED_LIBS
> +	@echo CONFIG_RTE_BUILD_COMBINE_LIBS=y >> $(@D)/build/.config
> +	@echo CONFIG_RTE_BUILD_SHARED_LIB=y >> $(@D)/build/.config
> +endef
> +
> +DPDK_POST_CONFIGURE_HOOKS += DPDK_ENABLE_SHARED_LIBS
> +endif

I think this piece of code does not take into account the fact that
BR2_SHARED_STATIC_LIBS=y means that both shared and static libraries
must be enabled. Remember that there is a three state choice:

 - BR2_STATIC_LIBS -> static libraries only (no shared libs)
 - BR2_SHARED_LIBS -> shared libraries only (no static libs)
 - BR2_SHARED_STATIC_LIBS -> both static and shared libraries

What does BUILD_COMBINE_LIBS does ?

Also, these >> means that if you do "make dpdk-reconfigure", those
lines will keep being added and added and added at the end of
the .config file.

> +# We create symlink named $(DPDK_CONFIG) to the build directory

a symlink

> +# to avoid calling install which behaves strange in DPDK build system.
> +define DPDK_CONFIGURE_CMDS
> +	$(MAKE) -C $(@D) T=$(DPDK_CONFIG) RTE_KERNELDIR=$(LINUX_DIR) CROSS=$(TARGET_CROSS) config
> +	@ln -sv build $(@D)/$(DPDK_CONFIG)

Don't use @ here.

> +endef
> +
> +define DPDK_BUILD_CMDS
> +	$(MAKE1) -C $(@D) T=$(DPDK_CONFIG) RTE_KERNELDIR=$(LINUX_DIR) CROSS=$(TARGET_CROSS) install

So you're doing the installation during the build step? This is not
good. But I see that later you're doing the installation manually. What
is being installed here then? The kernel modules?

Since the package is not "standard", it would probably be good to
include a few more comments in the .mk file to explain what is going on.

Also, is the MAKE1 instead of MAKE intentional?

> +endef
> +
> +ifeq ($(BR2_SHARED_LIBS),n)

A BR2_<foo> boolean variable can never be 'n'. It can be 'y' or empty.

> +# Install static libs only (DPDK compiles either *.so or *.a)
> +define DPDK_INSTALL_STAGING_LIBS
> +	$(INSTALL) -m 0755 -D -d $(STAGING_DIR)/usr/lib
> +	$(INSTALL) -m 0644 -D $(@D)/build/lib/*.a $(STAGING_DIR)/usr/lib

No -D in this case.

> +endef
> +
> +else
> +# Install shared libs only (DPDK compiles either *.so or *.a)
> +define DPDK_INSTALL_STAGING_LIBS
> +	$(INSTALL) -m 0755 -D -d $(STAGING_DIR)/usr/lib
> +	$(INSTALL) -m 0644 -D $(@D)/build/lib/*.so* $(STAGING_DIR)/usr/lib
> +endef

This logic again assumes that we install either shared *or* static
libraries. But if BR2_SHARED_STATIC_LIBS=y, we need to install both.


> +
> +define DPDK_INSTALL_TARGET_LIBS
> +	$(INSTALL) -m 0755 -D -d $(STAGING_DIR)/usr/lib

Should be $(TARGET_DIR).

> +	$(INSTALL) -m 0644 -D $(@D)/build/lib/*.so* $(TARGET_DIR)/usr/lib
> +endef
> +endif
> +
> +ifeq ($(BR2_PACKAGE_PYTHON),y)
> +define DPDK_INSTALL_TARGET_PYSCRIPTS
> +	$(INSTALL) -m 0755 -D -d $(STAGING_DIR)/usr/bin

Not needed (and wrong since you're using STAGING_DIR).

> +	$(INSTALL) -m 0755 -D $(@D)/tools/dpdk_nic_bind.py $(TARGET_DIR)/usr/bin
> +	$(INSTALL) -m 0755 -D $(@D)/tools/cpu_layout.py $(TARGET_DIR)/usr/bin

Use full paths for the destination.

> +endef
> +
> +DPDK_DEPENDENCIES += python
> +endif
> +
> +ifeq ($(BR2_PACKAGE_DPDK_TOOLS_TESTPMD),y)
> +define DPDK_INSTALL_TARGET_TESTPMD
> +	$(INSTALL) -m 0755 -D -d $(TARGET_DIR)/usr/bin

Not needed.

> +	$(INSTALL) -m 0755 -D $(@D)/build/app/testpmd $(TARGET_DIR)/usr/bin

Use a complete path for the destination.

> +endef
> +endif
> +
> +ifeq ($(BR2_PACKAGE_DPDK_TOOLS_TEST),y)
> +define DPDK_INSTALL_TARGET_TEST
> +	$(INSTALL) -m 0755 -D -d $(TARGET_DIR)/usr/dpdk
> +	$(INSTALL) -m 0755 -D $(@D)/build/app/test $(TARGET_DIR)/usr/dpdk
> +	$(INSTALL) -m 0755 -D $(@D)/app/test/*.py $(TARGET_DIR)/usr/dpdk
> +endef
> +
> +ifeq ($(BR2_PACKAGE_PYTHON),y)
> +DPDK_DEPENDENCIES += python-pexpect

Not good, you cannot add a package to the dependencies if it is not
enabled in the configuration. Is there anyway any point in installing
the .py files if you don't have a Python interpreter?

If you really want to handle it this way, you can do:

	select BR2_PACKAGE_PYTHON_PEXPECT if BR2_PACKAGE_PYTHON

under the BR2_PACKAGE_DPDK_TOOLS_TEST option.

> +define DPDK_INSTALL_STAGING_CMDS
> +	$(INSTALL) -m 0755 -D -d $(STAGING_DIR)/usr/include
> +	$(INSTALL) -m 0644 -D $(@D)/build/include/*.h $(STAGING_DIR)/usr/include
> +	$(DPDK_INSTALL_STAGING_LIBS)
> +endef
> +
> +define DPDK_INSTALL_TARGET_CMDS
> +	$(DPDK_INSTALL_TARGET_LIBS)
> +	$(DPDK_INSTALL_TARGET_PYSCRIPTS)
> +	$(DPDK_INSTALL_TARGET_TESTPMD)
> +	$(DPDK_INSTALL_TARGET_TEST)
> +endef
> +
> +$(eval $(generic-package))

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com