On 7/15/20 5:55 PM, Gregory Rose wrote: > > On 7/15/2020 8:21 AM, Van Haaren, Harry wrote: >>> -----Original Message----- >>> From: Ilya Maximets <[email protected]> >>> Sent: Wednesday, July 15, 2020 2:28 PM >>> To: Van Haaren, Harry <[email protected]>; Ilya Maximets >>> <[email protected]>; William Tu <[email protected]>; Gregory Rose >>> <[email protected]>; Stokes, Ian <[email protected]> >>> Cc: [email protected] >>> 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 <[email protected]> >>>>> Sent: Wednesday, July 15, 2020 1:22 PM >>>>> To: Van Haaren, Harry <[email protected]>; Ilya Maximets >>>>> <[email protected]>; William Tu <[email protected]>; Gregory Rose >>>>> <[email protected]>; Stokes, Ian <[email protected]> >>>>> Cc: [email protected] >>>>> 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 <[email protected]> >>>>>>> Sent: Wednesday, July 15, 2020 10:16 AM >>>>>>> To: William Tu <[email protected]>; Gregory Rose >>>>> <[email protected]>; >>>>>>> Van Haaren, Harry <[email protected]>; Stokes, Ian >>>>>>> <[email protected]> >>>>>>> Cc: [email protected]; Ilya Maximets <[email protected]>; >>>>>>> [email protected] >>>>>>> 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 <[email protected]> >>>>>>>>>> --- >>>>>>>>>> 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? > > Correct. That is the initial issue. Not knowing why this suddenly > occurred I added it to the dist to fix the bug. However, I think > this patch from Roi Dayan is better. >
OK. It looks like it's fine to just ignore while packaging. I'll take another look at the patch from Roi tomorrow. > https://mail.openvswitch.org/pipermail/ovs-dev/2020-July/372955.html > > Thanks, > > - Greg > >> >> >>>>>> 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 > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
