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

Jan Viktorin viktorin at rehivetech.com
Wed Mar 23 12:50:32 UTC 2016


Hello Thomas,,

thank you for comments. You can find your previous review here:

[1] http://lists.busybox.net/pipermail/buildroot/2015-December/146614.html
[2] http://lists.busybox.net/pipermail/buildroot/2015-December/146630.html

On Tue, 22 Mar 2016 21:12:32 +0100
Thomas Petazzoni <thomas.petazzoni at free-electrons.com> wrote:

> Hello,
> 
> On Tue, 22 Mar 2016 11:36:26 +0100, Jan Viktorin wrote:
> 
> > diff --git a/package/dpdk/Config.in b/package/dpdk/Config.in
> > new file mode 100644
> > index 0000000..a42271e
> > --- /dev/null
> > +++ b/package/dpdk/Config.in
> > @@ -0,0 +1,53 @@
> > +config BR2_PACKAGE_DPDK
> > +       bool "dpdk"  
> 
> Indentation of properties should be done with one tab (ditto in the
> following lines)
> 
> > +       depends on (BR2_i386 && !BR2_x86_i386 && !BR2_x86_i486 \
> > +		      && !BR2_x86_i586 && !BR2_x86_x1000) \
> > +                  || BR2_ARM_CPU_ARMV7A || BR2_aarch64 || BR2_aarch64_be  
> 
> I'm wondering why you have this list of architecture dependencies. Is
> it because there is really some architecture specific code, or is it a
> left-over from the times we didn't had the BR2_TOOLCHAIN_HAS_x
> options ?

Shortly: i686+ | ARMv7-a | AArch64, based on your suggestion [2]. Should
I put a comment there?

> 
> If that's really needed, please add a blind option like this:
> 
> config BR2_PACKAGE_DPDK_ARCH_SUPPORTS
> 	bool
> 	default y if ...

OK, seems reasonable.

> 
> That you can re-use as a dependency, both for the comment (see below)
> and the main option.
> 
> > +       depends on BR2_TOOLCHAIN_USES_GLIBC  
> 
> Then you need to have a comment like:
> 
> comment "dpdk needs a toolchain w/ glibc"
> 	depends on !BR2_TOOLCHAIN_USES_GLIBC
> 	depends on BR2_TOOLCHAIN_HAS_SYNC_...

OK, will fix.

> 
> > +       depends on BR2_TOOLCHAIN_HAS_SYNC_1
> > +       depends on BR2_TOOLCHAIN_HAS_SYNC_2
> > +       depends on BR2_TOOLCHAIN_HAS_SYNC_4
> > +       depends on BR2_TOOLCHAIN_HAS_SYNC_8  
> 
> Cosmetic nit, but I would prefer to see:
> 
> 	depends on BR2_TOOLCHAIN_HAS_SYNC_1 && \
> 		BR2_TOOLCHAIN_HAS_SYNC_2 && \
> 		BR2_TOOLCHAIN_HAS_SYNC_4 && \
> 		BR2_TOOLCHAIN_HAS_SYNC_8

Well, no problem, I just wonder why... as it's less readable and quite
ugly :).

> 
> 	
> 
> > +       help
> > +	 DPDK is a set of libraries and drivers for fast packet processing. It
> > +	 was designed to run on any processors, however, 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.  
> 
> Indentation of the help text should be one tab + *two* spaces.

The help text is right tab+ss, but the options are accidently indented
by spaces. Will fix.

> 
> > +
> > +	 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.
> > +	 * You may need to install the python2 interpreter if you want to use
> > +	   scripts dpdk_nic_bind.py and cpu_layout.py
> > +
> > +         http://www.dpdk.org/
> > +
> > +if BR2_PACKAGE_DPDK
> > +
> > +config BR2_PACKAGE_DPDK_CONFIG
> > +	string "Configuration"
> > +	default "i686-native-linuxapp-gcc" \
> > +		if BR2_x86_i686
> > +	default "x86_64-native-linuxapp-gcc" \
> > +		if BR2_x86_64
> > +	default "arm-armv7a-linuxapp-gcc" \
> > +		if BR2_ARM_CPU_ARMV7A
> > +	default "arm64-armv8a-linuxapp-gcc" \
> > +		if BR2_aarch64 || BR2_aarch64_be  
> 
> I don't remember if we discussed this: is there any reason to make this
> configurable? I.e is it necessary to make it a visible Config.in option
> as opposed to a blind one?

You've already agreed on this [2] and it seems to be a good compromise.
There are multiple configurations per CPU architecture and a user might
want to use a custom one.

> 
> > +
> > +config BR2_PACKAGE_DPDK_TEST
> > +	bool "Install tests suite"
> > +	select BR2_PACKAGE_PYTHON_PEXPECT if BR2_PACKAGE_PYTHON
> > +	help
> > +	  Install all DPDK tests. If you want to run the tests by the included
> > +	  autotest.py script you need to enable python manually.  
> 
> Is the test suite usable without Python ?

Yes, it's compiled as a single binary _test_. I use it that way. The
python scripts are helpful for automatization.

> 
> > diff --git a/package/dpdk/dpdk.mk b/package/dpdk/dpdk.mk
> > new file mode 100644
> > index 0000000..02860fd
> > --- /dev/null
> > +++ b/package/dpdk/dpdk.mk
> > @@ -0,0 +1,109 @@
> > +################################################################################
> > +#
> > +# dpdk
> > +#
> > +################################################################################
> > +
> > +DPDK_VERSION = 16.04-rc1  
> 
> We normally don't package release candidate / beta version. Are you
> confident that the final 16.04 release will be done before Buildroot
> ships its own release at the end of May ?

The release of 16.04 should be out early in April
(http://dpdk.org/dev/roadmap).

> 
> > +DPDK_SITE = http://dpdk.org/browse/dpdk/snapshot
> > +DPDK_SOURCE = dpdk-$(DPDK_VERSION).tar.gz  
> 
> Not needed, that's the default value.

OK.

> 
> > +
> > +DPDK_LICENSE = BSD (core), GPLv2+ (Linux drivers)  
> 
> BSD should be more specific, like BSD-2c, BSD-3c, etc.

BSD-3c, thanks.

> 
> > +DPDK_LICENSE_FILES = GNUmakefile LICENSE.GPL
> > +DPDK_INSTALL_STAGING = YES
> > +
> > +DPDK_DEPENDENCIES += linux  
> 
> If you need the Linux kernel to be built, then you need to also have:
> 
> 	depends on BR2_LINUX_KERNEL
> 
> in your Config.in, as well as a corresponding comment:
> 
> comment "dpdk needs the Linux kernel to be built"
> 	depends on !BR2_LINUX_KERNEL

OK.

> 
> > +
> > +ifeq ($(BR2_PACKAGE_LIBPCAP),y)
> > +DPDK_DEPENDENCIES += libpcap
> > +endif
> > +
> > +ifeq ($(BR2_SHARED_LIBS),y)
> > +define DPDK_ENABLE_SHARED_LIBS
> > +	$(call KCONFIG_ENABLE_OPT,CONFIG_RTE_BUILD_SHARED_LIB,\
> > +			$(@D)/build/.config)
> > +endef
> > +
> > +DPDK_POST_CONFIGURE_HOOKS += DPDK_ENABLE_SHARED_LIBS
> > +endif
> > +
> > +# We're building a kernel module without using the kernel-module infra,
> > +# so we need to tell we want module support in the kernel
> > +ifeq ($(BR2_PACKAGE_DPDK),y)
> > +LINUX_NEEDS_MODULES = y
> > +endif  
> 
> This is no longer the "right" way of doing this. Just select
> BR2_LINUX_NEEDS_MODULES in Config.in.

Cool, I didn't know that.

> 
> > +
> > +DPDK_CONFIG = $(call qstrip,$(BR2_PACKAGE_DPDK_CONFIG))
> > +
> > +ifeq ($(BR2_PACKAGE_DPDK_EXAMPLES),y)
> > +# Build of DPDK examples is not very straight-forward. It requires to have
> > +# the SDK and runtime installed on same place to reference it by RTE_SDK.
> > +# We place it locally in the build directory.
> > +define DPDK_BUILD_EXAMPLES
> > +	$(MAKE) -C $(@D) DESTDIR=$(@D)/examples-sdk \
> > +		CROSS=$(TARGET_CROSS) install-sdk install-runtime
> > +	$(MAKE) -C $(@D) RTE_KERNELDIR=$(LINUX_DIR) CROSS=$(TARGET_CROSS) \
> > +		RTE_SDK=$(@D)/examples-sdk/usr/local/share/dpdk \
> > +		T=$(DPDK_CONFIG) examples
> > +endef
> > +
> > +DPDK_EXAMPLES_PATH = $(@D)/examples-sdk/usr/local/share/dpdk/examples
> > +
> > +# Installation of examples is not supported in DPDK so we do it explicitly
> > +# here. As the binaries and libraries do not have a single or regular location
> > +# where to find them after build, we search for them by find.
> > +define DPDK_INSTALL_EXAMPLES
> > +	$(INSTALL) -m 0755 -d $(TARGET_DIR)/usr/local/bin
> > +	$(INSTALL) -m 0755 -d $(TARGET_DIR)/usr/local/lib  
> 
> In Buildroot, everything is installed in /usr, not /usr/local. Why

Just testing the reviewers here :). Well, at first I wanted to easily
check that it installs the right binaries and this is better checked in
an empty directory then in the /usr/bin with a great amount of other
things. I will put those into /usr/bin.

> should this package be different? Also, to create directories, we
> typically use mkdir -p.

Will use mkdir -p.

> 
> > +	for f in `find $(DPDK_EXAMPLES_PATH) -executable -type f   \
> > +			-name '[a-z]*.so*' | grep '\/lib\/.*'`; do \
> > +		$(INSTALL) -m 0755 -D $$f                          \
> > +			$(TARGET_DIR)/usr/local/lib/`basename $$f`;\
> > +	done
> > +	for f in `find $(DPDK_EXAMPLES_PATH) -executable -type f   \
> > +			! -name '*.so*' | grep '\/app\/.*'`; do    \
> > +		$(INSTALL) -m 0755 -D $$f                          \
> > +			$(TARGET_DIR)/usr/local/bin/`basename $$f`;\
> > +	done  
> 
> It's a bit annoying to have this logic. Could it be simplified to use
> -path instead of -name and avoid the grep ?

OK, will try.

> 
> Also, you need to add || exit 1 after the $(INSTALL) to exit the for
> loop in case of failure.

OK, will fix.

> 
> Ideally (but separately from this patch), you could contribute to
> upstream dpdk a way to install examples.

True. First of all, I am happy to do it automatically at the moment. Of
course, this is not the right place to solve it.

I'll try to patch this upstream and/or move this into a DPDK patch
temporarily.

> 
> > +endef
> > +
> > +# Build of the power example is broken (at least for 16.04).
> > +define DPDK_DISABLE_POWER
> > +	$(call KCONFIG_DISABLE_OPT,CONFIG_RTE_LIBRTE_POWER,\
> > +			$(@D)/build/.config)
> > +endef
> > +
> > +DPDK_POST_CONFIGURE_HOOKS += DPDK_DISABLE_POWER
> > +endif
> > +
> > +define DPDK_CONFIGURE_CMDS
> > +	$(MAKE) -C $(@D) T=$(DPDK_CONFIG) RTE_KERNELDIR=$(LINUX_DIR) \
> > +			   CROSS=$(TARGET_CROSS) config
> > +endef
> > +
> > +define DPDK_BUILD_CMDS
> > +	$(MAKE) -C $(@D) RTE_KERNELDIR=$(LINUX_DIR) CROSS=$(TARGET_CROSS)
> > +	$(DPDK_BUILD_EXAMPLES)
> > +endef
> > +
> > +define DPDK_INSTALL_STAGING_CMDS
> > +	$(MAKE) -C $(@D) DESTDIR=$(STAGING_DIR) prefix=/usr \
> > +		 CROSS=$(TARGET_CROSS) install-sdk
> > +endef
> > +
> > +ifeq ($(BR2_PACKAGE_DPDK_TEST),y)
> > +define DPDK_INSTALL_TARGET_TEST
> > +	$(INSTALL) -m 0755 -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
> > +endif
> > +
> > +define DPDK_INSTALL_TARGET_CMDS
> > +	$(MAKE) -C $(@D) DESTDIR=$(TARGET_DIR) prefix=/usr \
> > +		CROSS=$(TARGET_CROSS) install-runtime
> > +	$(DPDK_INSTALL_TARGET_TEST)
> > +	$(DPDK_INSTALL_EXAMPLES)
> > +endef
> > +
> > +$(eval $(generic-package))  
> 
> Other than that, looks good to me.

Thank you!
Jan

> 
> Thomas



-- 
   Jan Viktorin                  E-mail: Viktorin at RehiveTech.com
   System Architect              Web:    www.RehiveTech.com
   RehiveTech
   Brno, Czech Republic


More information about the buildroot mailing list