[ovs-dev] [PATCH] rpm-fedora: Add missing dist library

[Van Haaren, Harry]

> -----Original Message-----
> From: Ilya Maximets <i.maximets at ovn.org>
> Sent: Wednesday, July 15, 2020 10:16 AM
> To: William Tu <u9012063 at gmail.com>; Gregory Rose <gvrose8192 at gmail.com>;
> Van Haaren, Harry <harry.van.haaren at intel.com>; Stokes, Ian
> <ian.stokes at intel.com>
> Cc: dev at openvswitch.org; Ilya Maximets <i.maximets at ovn.org>;
> i.maximets at ovn.org
> Subject: Re: [PATCH] rpm-fedora: Add missing dist library
> 
> On 7/15/20 1:33 AM, William Tu wrote:
> > On Tue, Jul 14, 2020 at 03:40:39PM -0700, Gregory Rose wrote:
> >>
> >>
> >> On 7/14/2020 1:49 PM, Greg Rose wrote:
> >>> libopenvswitchavx512.a is needed for the fedora rpm spec.
> >>>
> >>> Signed-off-by: Greg Rose <gvrose8192 at gmail.com>
> >>> ---
> >>>  rhel/openvswitch-fedora.spec.in | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/rhel/openvswitch-fedora.spec.in b/rhel/openvswitch-
> fedora.spec.in
> >>> index 7bc8c34..154b49e 100644
> >>> --- a/rhel/openvswitch-fedora.spec.in
> >>> +++ b/rhel/openvswitch-fedora.spec.in
> >>> @@ -457,6 +457,7 @@ fi
> >>>  %{_bindir}/ovs-pki
> >>>  %{_bindir}/vtep-ctl
> >>>  %{_libdir}/lib*.so.*
> >>> +%{_libdir}/libopenvswitchavx512.a
> > How come before the avx512 patch series, we don't need to put
> libopenvswitch.a here?
> > And now we need to add libopenvswitchavx512.a?
> > Do we need to also add other .a files such as libsflow.a, libopenvswitch.a?
> 
> It seems like the real issue is that rpm build requests to build shared
> libraries only, but we're building this static library for some reason.
> 
> We should not include it into the package.  I think, we need to fix the
> build process and avoid building it in a first place.

Hey folks!

As you know the AVX512 patchset enables CPU ISA detection. ISA detection
and building is a little more complex, and indeed this is the first time that it
is being done in OVS - so we're all on a learning curve here.

OVS supports an  --enable-shared   build mode, where OVS itself is built as a
shared object. When this is enabled, components in OVS are built as .so files,
and LD handles linking them together. This is a pretty standard shared build
usage, and likely very familiar to everyone.

When enabling CPU ISA detection in OVS, we were very careful to not build
the whole OVS code with CPU specific CFLAGS like -mavx512f. To limit scope of
these CFLAGS, it is common practice to build a static library. The resulting .a
archive file (containing CPU specific ISA) is then static linked into the vswitchd binary.

The approach of  building application as shared or static, but always statically linking
in ISA specialized static libraries into the result is common in other packet processing
projects. For example, DPDK's ethdev drivers take a similar approach.

As a result, it is expected that static archive files will be linked into the vswitchd binary, and the
above patch solves exactly that for the .spec file. Regardless of the type of build (shared/static) of
OVS, the same .a files will require linking, so I see this as a potential fix.


If OVS community feels that all object files must be .so if using --enable-shared,
then we can revert to the previous method quite easily, just removing the -static flag
should do from lib/automake.mk. Commenting the below line achieves this:
# lib_libopenvswitchavx512_la_LDFLAGS = -static

With that above change, the shared build of OVS links to a separate .so for avx512.
$ ldd lib/.libs/libopenvswitch.so
libopenvswitchavx512.so.0 => ~/path/to/ovs/lib/.libs/libopenvswitchavx512.so.0 (0x00007f2a2c55d000)

Note in this case, the .spec file handles the .so with the %{_libdir}/lib*.so.* glob,
so no changes would be required due to the wildcarding in place already.


> Harry, Ian, could you, please, take a look?

Thanks for flagging, detailed response above. We have two solutions:
1) Keep static linking as today, and add the proposed line in the .spec file as per patch
2) Remove the -static flag, doing shared builds of ISA optimized code and linking with LD.

I took approach 1) in the patchset, but switching to 2) is a one-line change as above.

Developers/testers: please run "make clean" and "boot.sh" before configure, the build gets
confused without make clean, and will give strange linker errors on the SO version otherwise.


> Best regards, Ilya Maximets.

Regards, -Harry