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