Re: [ovs-dev] [PATCH] rpm-fedora: Add missing dist library
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 >>> Sent: Wednesday, July 15, 2020 2:28 PM >>> To: Van Haaren, Harry ; Ilya Maximets >>> ; William Tu ; Gregory Rose >>> ; Stokes, Ian >>> 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 > Sent: Wednesday, July 15, 2020 1:22 PM > To: Van Haaren, Harry ; Ilya Maximets > ; William Tu ; Gregory Rose > ; Stokes, Ian > 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 >>> Sent: Wednesday, July 15, 2020 10:16 AM >>> To: William Tu ; Gregory Rose > ; >>> Van Haaren, Harry ; Stokes, Ian >>> >>> Cc: d...@openvswitch.org; Ilya Maximets ; >>> 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 >> --- >> 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.
Re: [ovs-dev] [PATCH] rpm-fedora: Add missing dist library
On 7/15/2020 8:21 AM, Van Haaren, Harry wrote: -Original Message- From: Ilya Maximets Sent: Wednesday, July 15, 2020 2:28 PM To: Van Haaren, Harry ; Ilya Maximets ; William Tu ; Gregory Rose ; Stokes, Ian 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 Sent: Wednesday, July 15, 2020 1:22 PM To: Van Haaren, Harry ; Ilya Maximets ; William Tu ; Gregory Rose ; Stokes, Ian 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 Sent: Wednesday, July 15, 2020 10:16 AM To: William Tu ; Gregory Rose ; Van Haaren, Harry ; Stokes, Ian Cc: d...@openvswitch.org; Ilya Maximets ; 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 --- 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 = # Define avx512 library, with CPU ISA specific flags lib_LTLIBRARIES += lib/libopenvswitchavx512.la lib_libopenvswitchavx512_la_CFLAGS = # plain vswitch library ha
Re: [ovs-dev] [PATCH] rpm-fedora: Add missing dist library
> -Original Message- > From: Ilya Maximets > Sent: Wednesday, July 15, 2020 2:28 PM > To: Van Haaren, Harry ; Ilya Maximets > ; William Tu ; Gregory Rose > ; Stokes, Ian > 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 > >> Sent: Wednesday, July 15, 2020 1:22 PM > >> To: Van Haaren, Harry ; Ilya Maximets > >> ; William Tu ; Gregory Rose > >> ; Stokes, Ian > >> 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 > Sent: Wednesday, July 15, 2020 10:16 AM > To: William Tu ; Gregory Rose > >> ; > Van Haaren, Harry ; Stokes, Ian > > Cc: d...@openvswitch.org; Ilya Maximets ; > 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 > >>> --- > >>> 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.o
Re: [ovs-dev] [PATCH] rpm-fedora: Add missing dist library
On 7/14/2020 4:33 PM, 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 --- 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? William That's a good question - but apparently not since the rpm checker doesn't cause an error because they're missing. Or - I missed something. spec files aren't my specialty. I'm just trying to get a build done. - Greg ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] rpm-fedora: Add missing dist library
On 7/15/20 2:36 PM, Van Haaren, Harry wrote: >> -Original Message- >> From: Ilya Maximets >> Sent: Wednesday, July 15, 2020 1:22 PM >> To: Van Haaren, Harry ; Ilya Maximets >> ; William Tu ; Gregory Rose >> ; Stokes, Ian >> 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 Sent: Wednesday, July 15, 2020 10:16 AM To: William Tu ; Gregory Rose >> ; Van Haaren, Harry ; Stokes, Ian Cc: d...@openvswitch.org; Ilya Maximets ; 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 >>> --- >>> 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
Re: [ovs-dev] [PATCH] rpm-fedora: Add missing dist library
> -Original Message- > From: Ilya Maximets > Sent: Wednesday, July 15, 2020 1:22 PM > To: Van Haaren, Harry ; Ilya Maximets > ; William Tu ; Gregory Rose > ; Stokes, Ian > 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 > >> Sent: Wednesday, July 15, 2020 10:16 AM > >> To: William Tu ; Gregory Rose > ; > >> Van Haaren, Harry ; Stokes, Ian > >> > >> Cc: d...@openvswitch.org; Ilya Maximets ; > >> 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 > > --- > > 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. > > 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 t
Re: [ovs-dev] [PATCH] rpm-fedora: Add missing dist library
On 7/15/20 1:45 PM, Van Haaren, Harry wrote: >> -Original Message- >> From: Ilya Maximets >> Sent: Wednesday, July 15, 2020 10:16 AM >> To: William Tu ; Gregory Rose ; >> Van Haaren, Harry ; Stokes, Ian >> >> Cc: d...@openvswitch.org; Ilya Maximets ; >> 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 > --- > 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 (0x7f2a2c55d000) > > 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" be
Re: [ovs-dev] [PATCH] rpm-fedora: Add missing dist library
> -Original Message- > From: Ilya Maximets > Sent: Wednesday, July 15, 2020 10:16 AM > To: William Tu ; Gregory Rose ; > Van Haaren, Harry ; Stokes, Ian > > Cc: d...@openvswitch.org; Ilya Maximets ; > 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 > >>> --- > >>> 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. 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 (0x7f2a2c55d000) 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
Re: [ovs-dev] [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 >>> --- >>> 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. Harry, Ian, could you, please, take a look? Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] rpm-fedora: Add missing dist library
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 > >--- > > 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? William ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] rpm-fedora: Add missing dist library
On 7/14/2020 1:49 PM, Greg Rose wrote: libopenvswitchavx512.a is needed for the fedora rpm spec. Signed-off-by: Greg Rose --- 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 %{_sbindir}/ovs-bugtool %{_sbindir}/ovs-vswitchd %{_sbindir}/ovsdb-server I'm aware soft freeze is in effect, but this is a bug fix I believe. - Greg ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev