[Buildroot] [PATCH v2 3/3] dpdk: new package
Thomas Petazzoni
thomas.petazzoni at free-electrons.com
Wed Dec 9 22:36:03 UTC 2015
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
More information about the buildroot
mailing list