| Message ID | 1449670313-3613-4-git-send-email-viktorin@rehivetech.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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
On Wed, 9 Dec 2015 23:36:03 +0100 Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > 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. Hello Thomas, thanks for your review... > > > 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" ? Well, I'd say "at least i686", however, the general reason was the list of default configurations available in DPDK (only gcc-based configurations are listed): defconfig_arm64-armv8a-linuxapp-gcc defconfig_arm64-thunderx-linuxapp-gcc defconfig_arm64-xgene1-linuxapp-gcc defconfig_arm-armv7a-linuxapp-gcc defconfig_i686-native-linuxapp-gcc defconfig_ppc_64-power8-linuxapp-gcc defconfig_tile-tilegx-linuxapp-gcc defconfig_x86_64-ivshmem-linuxapp-gcc defconfig_x86_64-native-bsdapp-gcc defconfig_x86_64-native-linuxapp-gcc defconfig_x86_x32-native-linuxapp-gcc > > Please wrap lines at 72 characters. I know... ;) I've discovered a great helper today ":set colorcolumn=80". > > > + depends on BR2_TOOLCHAIN_USES_GLIBC > > You need a dependency on BR2_LINUX_KERNEL here, since your .mk file is > building some kernel modules. OK. > > > + 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 :) Already fixed internally, thanks. > > > + > > + 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 ? DPDK provides a list of default configurations. I can imagine somebody would like to use a custom one for this (added probably by a custom patch to Buildroot). Also, DPDK will evolve overtime and this enables to specify a configuration we didn't have in the list. Moreover, as you can see from the list of default configurations above, for ARMv8, there are 3 possible configurations... > > > + 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. Yes, 64 bits. Anyway, I cannot test this, so it is just a blind shoot. > > > + 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. OK. So you don't expect somebody sticks on a certain version of a package? This happens... It does not make sense for -rc versions, of course. > > > +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. BSD-3c Can the DPDK_LICENSE contain more licenses? (RTFM..., I am lazy, maybe tomorrow :).) > > > +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. How? The BSD license text is not in the DPDK upstream, no idea why. > > > +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 Does BR2_SHARED_STATIC_LIBSBR2_SHARED_STATIC_LIBS mean, that I MUST provide both shared and static version? In that case, you are right and I will have hard time of figure out, how to persuade DPDK to give me those... But, as I've mentioned in the cover letter, the install system has changed recently in DPDK, so it's possible I can do this better... > > What does BUILD_COMBINE_LIBS does ? Good question... My answer is that it magically builds and works with this option :). I've done this quite a long time ago, I have to check it again, my bad. > > 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. Wow :). Is there a better way to do this? BTW, the fact there is a .config file does not mean that it is the same system as in the kconfig. It is not... > > > +# We create symlink named $(DPDK_CONFIG) to the build directory > > a symlink OK. > > > +# 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. OK. > > > +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? Nothing is installed here. Actually, it installs things for itself (not into standard paths or anything...). This is the only magic sequence that builds the DPDK in the way I need for Buildroot. Think of it in a way like s/install/build/. Yes, it is strange. I hope that it is no more an issue with the recent changes in DPDK. > > 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. True. > > Also, is the MAKE1 instead of MAKE intentional? Yes, that is an intention. I have issues with parallel builds. > > > +endef > > + > > +ifeq ($(BR2_SHARED_LIBS),n) > > A BR2_<foo> boolean variable can never be 'n'. It can be 'y' or empty. OK. This is better (however, less readable): ifneq ($(BR2_SHARED_LIBS),y) isn't it? > > > +# 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. Do you mean only for the second one? > > > +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. So my assumption (MUST) on the top of this e-mail is right. But, DPDK provides either *.so or *.a. Never both. Or, I don't know, how to configure it for this. In fact, the shared builds are sometimes claimed (I've seen that written somewhere but it was not in the docs) to be broken (they work for my cases) and DPDK applications are usually build statically for x86. > > > > + > > +define DPDK_INSTALL_TARGET_LIBS > > + $(INSTALL) -m 0755 -D -d $(STAGING_DIR)/usr/lib > > Should be $(TARGET_DIR). OK. > > > + $(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). This looks like a copy & paste mistake. Same as above, it should be TARGET_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. Do you mean: $(TARGET_DIR)/usr/bin/dpdk_nic_bind.py? > > > +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. OK. > > > + $(INSTALL) -m 0755 -D $(@D)/build/app/testpmd $(TARGET_DIR)/usr/bin > > Use a complete path for the destination. Same as above. > > > +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? No. I think the *.py files can be ommited when there is no 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. So, I'd install the binaries only. And if there is a python interpreter I add the *.py files. I also enforce the PEXPECT package in Config.in in that case. This sounds promising. > > > +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 Regards Jan
Hello Jan, On Thu, 10 Dec 2015 00:27:28 +0100, Jan Viktorin wrote: > > > 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" ? > > Well, I'd say "at least i686", however, the general reason was the list > of default configurations available in DPDK (only gcc-based > configurations are listed): > > defconfig_arm64-armv8a-linuxapp-gcc > defconfig_arm64-thunderx-linuxapp-gcc > defconfig_arm64-xgene1-linuxapp-gcc > defconfig_arm-armv7a-linuxapp-gcc > defconfig_i686-native-linuxapp-gcc > defconfig_ppc_64-power8-linuxapp-gcc > defconfig_tile-tilegx-linuxapp-gcc > defconfig_x86_64-ivshmem-linuxapp-gcc > defconfig_x86_64-native-bsdapp-gcc > defconfig_x86_64-native-linuxapp-gcc > defconfig_x86_x32-native-linuxapp-gcc For x86, I think you should just exclude the CPUs < i686, like: depends on (BR2_i386 && !BR2_x86_i386 && !BR2_x86_i486 && !BR2_x86_i586 && !BR2_x86_x1000) || ... > > > +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 ? > > DPDK provides a list of default configurations. I can imagine somebody > would like to use a custom one for this (added probably by a custom > patch to Buildroot). Also, DPDK will evolve overtime and this enables to > specify a configuration we didn't have in the list. Moreover, as you can > see from the list of default configurations above, for ARMv8, there are > 3 possible configurations... Ok, makes sense. > > > + 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. > > Yes, 64 bits. Anyway, I cannot test this, so it is just a blind shoot. You can decide to not support it. > > > + 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. > > OK. So you don't expect somebody sticks on a certain version of a > package? This happens... It does not make sense for -rc versions, of > course. I don't understand what you mean here. dpdk.mk references 2.2.0-rc3 only, so only the hash for 2.2.0-rc3 should be in the .hash file. There is no reason to have hashes for other versions. > > > +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. > > BSD-3c > > Can the DPDK_LICENSE contain more licenses? (RTFM..., I am lazy, maybe tomorrow :).) Yes, it can contain more licenses. Here is an example from Buildroot: ATTR_LICENSE = GPLv2+ (programs), LGPLv2.1+ (libraries) > > > +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. > > How? The BSD license text is not in the DPDK upstream, no idea why. You can put a source file which carries a BSD license header in DPDK_LICENSE_FILES. Usually, we try to choose a fairly small source file. See for example: E2FSPROGS_LICENSE_FILES = COPYING lib/uuid/COPYING lib/ss/mit-sipb-copyright.h lib/et/internal.h > > 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 > > Does BR2_SHARED_STATIC_LIBSBR2_SHARED_STATIC_LIBS mean, that I MUST > provide both shared and static version? Normally yes. However in practice, not all packages fully comply with this. BR2_SHARED_LIBS sometimes lead to packages building/installing both the shared and static variants. What is however non-negotiable is that BR2_STATIC_LIBS should not build any shared library. > In that case, you are right and > I will have hard time of figure out, how to persuade DPDK to give me > those... But, as I've mentioned in the cover letter, the install system > has changed recently in DPDK, so it's possible I can do this better... I think you can do BR2_STATIC_LIBS builds/installs static libs only, and BR2_STATIC_SHARED_LIBS or BR2_SHARED_LIBS builds/installs shared libs only. It's probably good enough for now. > > 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. > > Wow :). Is there a better way to do this? BTW, the fact there is > a .config file does not mean that it is the same system as in the > kconfig. It is not... Even if it is not kconfig based, you can still use the sort of logic as the KCONFIG_ENABLE_OPT, KCONFIG_SET_OPT and KCONFIG_DISABLE_OPT (see pkg-utils.mk). > > > +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? > > Nothing is installed here. Actually, it installs things for itself (not > into standard paths or anything...). This is the only magic sequence > that builds the DPDK in the way I need for Buildroot. Think of it in a > way like s/install/build/. Yes, it is strange. I hope that it is no > more an issue with the recent changes in DPDK. Just add a comment above to explain this. Thanks :) > > Also, is the MAKE1 instead of MAKE intentional? > > Yes, that is an intention. I have issues with parallel builds. Ok. > > > +ifeq ($(BR2_SHARED_LIBS),n) > > > > A BR2_<foo> boolean variable can never be 'n'. It can be 'y' or empty. > > OK. This is better (however, less readable): > > ifneq ($(BR2_SHARED_LIBS),y) > > isn't it? No: ifeq ($(BR2_SHARED_LIBS),) or alternatively: ifeq ($(BR2_STATIC_LIBS),y) depending on the behavior you want for BR2_SHARED_STATIC_LIBS. > > > > > > +# 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. > > Do you mean only for the second one? For both. > > > +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. > > So my assumption (MUST) on the top of this e-mail is right. But, DPDK > provides either *.so or *.a. Never both. Or, I don't know, how to > configure it for this. In fact, the shared builds are sometimes claimed > (I've seen that written somewhere but it was not in the docs) to be > broken (they work for my cases) and DPDK applications are usually build > statically for x86. I think it's OK if BR2_SHARED_STATIC_LIBS is not implemented perfectly for dpdk. Just assume BR2_SHARED_STATIC_LIBS is like BR2_SHARED_LIBS. > > > +define DPDK_INSTALL_TARGET_LIBS > > > + $(INSTALL) -m 0755 -D -d $(STAGING_DIR)/usr/lib > > > > Should be $(TARGET_DIR). > > OK. And no -D here. > > > + $(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. > > Do you mean: $(TARGET_DIR)/usr/bin/dpdk_nic_bind.py? Yes. Thanks! Thomas
diff --git a/package/Config.in b/package/Config.in index 77a92d2..fd07d5d 100644 --- a/package/Config.in +++ b/package/Config.in @@ -999,6 +999,7 @@ menu "Networking" source "package/cgic/Config.in" source "package/cppzmq/Config.in" source "package/czmq/Config.in" + source "package/dpdk/Config.in" source "package/filemq/Config.in" source "package/flickcurl/Config.in" source "package/fmlib/Config.in" 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 + depends on BR2_TOOLCHAIN_USES_GLIBC + 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. + + 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" + 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" \ + if BR2_powerpc_power8 + default "arm-armv7a-linuxapp-gcc" \ + if BR2_ARM_CPU_ARMV7A + default "arm64-armv8a-linuxapp-gcc" \ + if BR2_aarch64 || BR2_aarch64_be + +config BR2_PACKAGE_DPDK_TOOLS_TESTPMD + bool "Install testpmd" + default y + help + Install application for general testing of DPDK and PMD drivers. + +config BR2_PACKAGE_DPDK_TOOLS_TEST + bool "Install tests suite" + help + Install all DPDK tests. If you want to run the tests by the included + autotest.py script you need to enable python manually. + +endif 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 diff --git a/package/dpdk/dpdk.mk b/package/dpdk/dpdk.mk new file mode 100644 index 0000000..2d8a5a3 --- /dev/null +++ b/package/dpdk/dpdk.mk @@ -0,0 +1,112 @@ +################################################################################ +# +# dpdk +# +################################################################################ + +DPDK_VERSION = 2.2.0-rc3 +DPDK_SITE = http://dpdk.org/browse/dpdk/snapshot +DPDK_SOURCE = dpdk-$(DPDK_VERSION).tar.gz + +DPDK_LICENSE = BSD +DPDK_LICENSE_FILES = LICENSE.LGPL LICENSE.GPL +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 + +# 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 + + +DPDK_CONFIG = $(call qstrip,$(BR2_PACKAGE_DPDK_CONFIG)) + +# We create symlink named $(DPDK_CONFIG) to the build directory +# 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) +endef + +define DPDK_BUILD_CMDS + $(MAKE1) -C $(@D) T=$(DPDK_CONFIG) RTE_KERNELDIR=$(LINUX_DIR) CROSS=$(TARGET_CROSS) install +endef + +ifeq ($(BR2_SHARED_LIBS),n) +# 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 +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 + +define DPDK_INSTALL_TARGET_LIBS + $(INSTALL) -m 0755 -D -d $(STAGING_DIR)/usr/lib + $(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 + $(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 +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 + $(INSTALL) -m 0755 -D $(@D)/build/app/testpmd $(TARGET_DIR)/usr/bin +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 +endif +endif + +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))
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. The package compiles and installs DPDK libraries on the target and staging and allows to compiler other applications depending on the DPDK library. It can also install some basic tools the DPDK provides (testpmd, python scripts, test suite). The patch assumes DPDK 2.2.0 (rc3) which introduces the ARM architecture support. The included hashes are calculated locally by downloading the tar.gz archives by hand. The DPDK project is licensed under BSD license (see http://dpdk.org/), however, there is no license file in the root of the project. Moreover, there are parts (Linux Kernel modules) which are GPLv2. So the license specification might look strange. Signed-off-by: Jan Viktorin <viktorin@rehivetech.com> --- v2: (mostly suggestions by Arnout) * simplified Config.in - avoid version, source and config selection * improved dependency on libpcap * user python scripts are included if python package is enabled * installing of tests includes autotest*.py scripts (we do not care about the python here, assuming the user will install it manually when testing) * minor coding style fixes * depends on python-pexpect package * using version 2.2.0-rc3 --- package/Config.in | 1 + package/dpdk/Config.in | 47 +++++++++++++++++++++ package/dpdk/dpdk.hash | 4 ++ package/dpdk/dpdk.mk | 112 +++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 164 insertions(+) create mode 100644 package/dpdk/Config.in create mode 100644 package/dpdk/dpdk.hash create mode 100644 package/dpdk/dpdk.mk