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

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

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
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com