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

Wed Jul 15 15:55:37 UTC 2020

On 7/15/2020 8:21 AM, Van Haaren, Harry wrote:
>> -----Original Message-----
>> From: Ilya Maximets <i.maximets at ovn.org>
>> Sent: Wednesday, July 15, 2020 2:28 PM
>> To: Van Haaren, Harry <harry.van.haaren at intel.com>; Ilya Maximets
>> <i.maximets at ovn.org>; William Tu <u9012063 at gmail.com>; Gregory Rose
>> <gvrose8192 at gmail.com>; Stokes, Ian <ian.stokes at intel.com>
>> Cc: dev at 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.maximets at ovn.org>
>>>> Sent: Wednesday, July 15, 2020 1:22 PM
>>>> To: Van Haaren, Harry <harry.van.haaren at intel.com>; Ilya Maximets
>>>> <i.maximets at ovn.org>; William Tu <u9012063 at gmail.com>; Gregory Rose
>>>> <gvrose8192 at gmail.com>; Stokes, Ian <ian.stokes at intel.com>
>>>> Cc: dev at 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.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.
>>>>
>>>> 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.

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
>>>>>
>>>
> 