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