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 <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?
>
> 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
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
