> -----Original Message-----
> From: Ilya Maximets <i.maxim...@ovn.org>
> Sent: Wednesday, July 15, 2020 2:28 PM
> To: Van Haaren, Harry <harry.van.haa...@intel.com>; Ilya Maximets
> <i.maxim...@ovn.org>; William Tu <u9012...@gmail.com>; Gregory Rose
> <gvrose8...@gmail.com>; Stokes, Ian <ian.sto...@intel.com>
> Cc: d...@openvswitch.org
> Subject: Re: [PATCH] rpm-fedora: Add missing dist library
>
> On 7/15/20 2:36 PM, Van Haaren, Harry wrote:
> >> -----Original Message-----
> >> From: Ilya Maximets <i.maxim...@ovn.org>
> >> Sent: Wednesday, July 15, 2020 1:22 PM
> >> To: Van Haaren, Harry <harry.van.haa...@intel.com>; Ilya Maximets
> >> <i.maxim...@ovn.org>; William Tu <u9012...@gmail.com>; Gregory Rose
> >> <gvrose8...@gmail.com>; Stokes, Ian <ian.sto...@intel.com>
> >> Cc: d...@openvswitch.org
> >> Subject: Re: [PATCH] rpm-fedora: Add missing dist library
> >>
> >> On 7/15/20 1:45 PM, Van Haaren, Harry wrote:
> >>>> -----Original Message-----
> >>>> From: Ilya Maximets <i.maxim...@ovn.org>
> >>>> Sent: Wednesday, July 15, 2020 10:16 AM
> >>>> To: William Tu <u9012...@gmail.com>; Gregory Rose
> >> <gvrose8...@gmail.com>;
> >>>> Van Haaren, Harry <harry.van.haa...@intel.com>; Stokes, Ian
> >>>> <ian.sto...@intel.com>
> >>>> Cc: d...@openvswitch.org; Ilya Maximets <i.maxim...@ovn.org>;
> >>>> i.maxim...@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 <gvrose8...@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.
> >>
> >> Isn't it better to link libopenvswitchavx512.a to libopenvswitch.{a,so}
> >> instead
> >> of linking it to the vswitchd binary? This should allow us to not
> >> distribute
> >> it separately inside the package.
> >>
> >> There are few issues here:
> >> 1. libopenvswitchavx512.a is a confusing name, because it contains only one
> >> object
> >> lib/dpif-netdev-lookup-avx512-gather.c and not the whole library.
> >> 2. vswitchd by itself doesn't use dpif-netdev-lookup-* stuff. These
> >> things are
> >> only used by libopenvswitch. So, linking of libopenvswitchavx512.a into
> >> vswitchd doesn't make much logical sense.
> >> 3. If someone will try to create a different application on top of
> libopenvswitch
> >> shared library, they will need to additionally statically link
> >> libopenvswitchavx512.a. Otherwise build will fail, right?
> >
> > Apologies - a mistake on my side in the email description above.
> > All 3 of your concerns are resolved by the correction I think?
> > Clarifying here:
> >
> > libopenvswitchavx512 is linked into libopenvswitch.
> >
> > vswitchd always links against libopenvswitch.
> > Other binaries also link against libopenvswitch.
> >
> > No specific changes are required to pick up the avx512 for binaries (eg
> vswitchd).
> > This is apparent from the build-system changes - no patches to vswitchd
> makefiles.
>
> Hmm. OK. So, we should not ship this static library as it already linked
> into libopenvswitch and should not be used by anyone.
>
> Maybe I'm missing something obvious, but I see following while building
> with static libs:
>
> - https://travis-ci.org/github/openvswitch/ovs/jobs/707655283#L2179
> This line doesn't seem to include libopenvswitchavx512 anyhow.
>
> - And I do see libopenvswitchavx512.a in a linker command while linking ovs-
> vswitchd:
> https://travis-ci.org/github/openvswitch/ovs/jobs/707655283#L2572
Correct. From the automake.mk file;
# plain vswitch library compiled as normal library, without CPU specific ISA
CFLAGS
lib_LTLIBRARIES += lib/libopenvswitch.la
lib_libopenvswitch_la_SOURCES = <all lib/* source files here>
# Define avx512 library, with CPU ISA specific flags
lib_LTLIBRARIES += lib/libopenvswitchavx512.la
lib_libopenvswitchavx512_la_CFLAGS = <CPU ISA CFLAGS here, -mavx512f etc>
# plain vswitch library has a dependency on vswitchavx512
lib_libopenvswitch_la_LIBADD += lib/libopenvswitchavx512.la
I'm losing track of the actual issue here. What is the concern that we're
discussing?
If packaging ignores the static archive libopenvswitchavx512.a then all is OK?
> >>> 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
> >>>
> >
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
