fix for hidden corei7 requirement in binary packages
diff mbox series

Message ID ord0h3gy6w.fsf@lxoliva.fsfla.org
State New
Headers show
Series
  • fix for hidden corei7 requirement in binary packages
Related show

Commit Message

Alexandre Oliva Aug. 17, 2019, 11:38 p.m. UTC
Hi,

After upgrading some old Phenom servers from Fedora/Freed-ora 29 to
30's, to ceph-14.2.2, I was surprised by very early crashes of both
ceph-mon and ceph-osd.  After ruling out disk and memory corruption, I
investigated a bit noticed all of them crashed during pre-main() init
section processing, at an instruction not available on the Phenom X6
processors that support sse4a, but not e.g. sse4.1.

It turns out that much of librte is built with -march=corei7.  That's a
little excessive, considering that x86/rte_memcpy.h would be happy
enough with -msse4.1, but not with earlier sse versions that Fedora is
supposed to target.

I understand rte_memcpy.h is meant for better performance, inlining
fixed-size and known-alignment implementations of memcpy into users.
Alas, that requires setting a baseline target processor, and you'll only
get as efficient an implementation as what's built in.

I noticed an attempt for dynamic selection, but GCC rightfully won't
inline across different target flags, so we'd have to give up inlining
to get better dynamic behavior.  The good news is that glibc already
offers dynamic selection of memcpy implementations, so hopefully the
impact of this change won't be much worse than that of enabling dynamic
selection, without the complications.

If that's not good enough, compiling ceph with flags that enable SSE4.1,
AVX2 or AVX512, or with a -march flag that implicitly enables them,
would restore current performance, but without that, you will (with the
patch below) get a package that runs on a broader range of processors,
that the base distro (through the compiler's baseline flags) chooses to
support.  It's not nice when you install a package on a processor that's
supposed to be supported and suddenly you're no longer sure it is ;-)

Perhaps building a shared librte, so that one could build and install
builds targeting different ISA versions, without having to rebuild all
of ceph, would be a reasonable way to address the better tuning of these
performance-critical bits.



src/spdk/dpdk:

Comments

Brad Hubbard Aug. 19, 2019, 1:24 a.m. UTC | #1
+dev@ceph

On Sun, Aug 18, 2019 at 9:50 AM Alexandre Oliva <oliva@gnu.org> wrote:
>
> Hi,
>
> After upgrading some old Phenom servers from Fedora/Freed-ora 29 to
> 30's, to ceph-14.2.2, I was surprised by very early crashes of both
> ceph-mon and ceph-osd.  After ruling out disk and memory corruption, I
> investigated a bit noticed all of them crashed during pre-main() init
> section processing, at an instruction not available on the Phenom X6
> processors that support sse4a, but not e.g. sse4.1.
>
> It turns out that much of librte is built with -march=corei7.  That's a
> little excessive, considering that x86/rte_memcpy.h would be happy
> enough with -msse4.1, but not with earlier sse versions that Fedora is
> supposed to target.
>
> I understand rte_memcpy.h is meant for better performance, inlining
> fixed-size and known-alignment implementations of memcpy into users.
> Alas, that requires setting a baseline target processor, and you'll only
> get as efficient an implementation as what's built in.
>
> I noticed an attempt for dynamic selection, but GCC rightfully won't
> inline across different target flags, so we'd have to give up inlining
> to get better dynamic behavior.  The good news is that glibc already
> offers dynamic selection of memcpy implementations, so hopefully the
> impact of this change won't be much worse than that of enabling dynamic
> selection, without the complications.
>
> If that's not good enough, compiling ceph with flags that enable SSE4.1,
> AVX2 or AVX512, or with a -march flag that implicitly enables them,
> would restore current performance, but without that, you will (with the
> patch below) get a package that runs on a broader range of processors,
> that the base distro (through the compiler's baseline flags) chooses to
> support.  It's not nice when you install a package on a processor that's
> supposed to be supported and suddenly you're no longer sure it is ;-)
>
> Perhaps building a shared librte, so that one could build and install
> builds targeting different ISA versions, without having to rebuild all
> of ceph, would be a reasonable way to address the better tuning of these
> performance-critical bits.
>
>
>
> src/spdk/dpdk:
>
> diff --git a/lib/librte_eal/common/include/arch/x86/rte_memcpy.h b/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
> index 7b758094d..ce714bf02 100644
> --- a/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
> +++ b/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
> @@ -40,6 +40,16 @@ extern "C" {
>  static __rte_always_inline void *
>  rte_memcpy(void *dst, const void *src, size_t n);
>
> +#ifndef RTE_MACHINE_CPUFLAG_SSE4_1
> +
> +static __rte_always_inline void *
> +rte_memcpy(void *dst, const void *src, size_t n)
> +{
> +  return memcpy(dst, src, n);
> +}
> +
> +#else /* RTE_MACHINE_CPUFLAG_SSE4_1 */
> +
>  #ifdef RTE_MACHINE_CPUFLAG_AVX512F
>
>  #define ALIGNMENT_MASK 0x3F
> @@ -869,6 +879,8 @@ rte_memcpy(void *dst, const void *src, size_t n)
>                 return rte_memcpy_generic(dst, src, n);
>  }
>
> +#endif /* RTE_MACHINE_CPUFLAG_SSE4_1 */
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/mk/machine/default/rte.vars.mk b/mk/machine/default/rte.vars.mk
> index df08d3b03..6bf695849 100644
> --- a/mk/machine/default/rte.vars.mk
> +++ b/mk/machine/default/rte.vars.mk
> @@ -27,4 +27,4 @@
>  # CPU_LDFLAGS =
>  # CPU_ASFLAGS =
>
> -MACHINE_CFLAGS += -march=corei7
> +# MACHINE_CFLAGS += -march=corei7
>
>
> --
> Alexandre Oliva, freedom fighter  he/him   https://FSFLA.org/blogs/lxo
> Be the change, be Free!                 FSF Latin America board member
> GNU Toolchain Engineer                        Free Software Evangelist
> Hay que enGNUrecerse, pero sin perder la terGNUra jamás - Che GNUevara
kefu chai Aug. 19, 2019, 5:04 a.m. UTC | #2
On Mon, Aug 19, 2019 at 9:24 AM Brad Hubbard <bhubbard@redhat.com> wrote:
>
> +dev@ceph
>
> On Sun, Aug 18, 2019 at 9:50 AM Alexandre Oliva <oliva@gnu.org> wrote:
> >
> > Hi,
> >
> > After upgrading some old Phenom servers from Fedora/Freed-ora 29 to
> > 30's, to ceph-14.2.2, I was surprised by very early crashes of both
> > ceph-mon and ceph-osd.  After ruling out disk and memory corruption, I
> > investigated a bit noticed all of them crashed during pre-main() init
> > section processing, at an instruction not available on the Phenom X6
> > processors that support sse4a, but not e.g. sse4.1.
> >
> > It turns out that much of librte is built with -march=corei7.  That's a
> > little excessive, considering that x86/rte_memcpy.h would be happy
> > enough with -msse4.1, but not with earlier sse versions that Fedora is
> > supposed to target.
> >
> > I understand rte_memcpy.h is meant for better performance, inlining
> > fixed-size and known-alignment implementations of memcpy into users.
> > Alas, that requires setting a baseline target processor, and you'll only
> > get as efficient an implementation as what's built in.
> >
> > I noticed an attempt for dynamic selection, but GCC rightfully won't
> > inline across different target flags, so we'd have to give up inlining
> > to get better dynamic behavior.  The good news is that glibc already
> > offers dynamic selection of memcpy implementations, so hopefully the
> > impact of this change won't be much worse than that of enabling dynamic
> > selection, without the complications.
> >
> > If that's not good enough, compiling ceph with flags that enable SSE4.1,
> > AVX2 or AVX512, or with a -march flag that implicitly enables them,
> > would restore current performance, but without that, you will (with the
> > patch below) get a package that runs on a broader range of processors,
> > that the base distro (through the compiler's baseline flags) chooses to
> > support.  It's not nice when you install a package on a processor that's
> > supposed to be supported and suddenly you're no longer sure it is ;-)
> >
> > Perhaps building a shared librte, so that one could build and install
> > builds targeting different ISA versions, without having to rebuild all
> > of ceph, would be a reasonable way to address the better tuning of these
> > performance-critical bits.
> >
> >
> >
> > src/spdk/dpdk:
> >
> > diff --git a/lib/librte_eal/common/include/arch/x86/rte_memcpy.h b/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
> > index 7b758094d..ce714bf02 100644
> > --- a/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
> > +++ b/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
> > @@ -40,6 +40,16 @@ extern "C" {
> >  static __rte_always_inline void *
> >  rte_memcpy(void *dst, const void *src, size_t n);
> >
> > +#ifndef RTE_MACHINE_CPUFLAG_SSE4_1
> > +
> > +static __rte_always_inline void *
> > +rte_memcpy(void *dst, const void *src, size_t n)
> > +{
> > +  return memcpy(dst, src, n);
> > +}
> > +
> > +#else /* RTE_MACHINE_CPUFLAG_SSE4_1 */
> > +
> >  #ifdef RTE_MACHINE_CPUFLAG_AVX512F
> >
> >  #define ALIGNMENT_MASK 0x3F
> > @@ -869,6 +879,8 @@ rte_memcpy(void *dst, const void *src, size_t n)
> >                 return rte_memcpy_generic(dst, src, n);
> >  }
> >
> > +#endif /* RTE_MACHINE_CPUFLAG_SSE4_1 */
> > +
> >  #ifdef __cplusplus
> >  }
> >  #endif
> > diff --git a/mk/machine/default/rte.vars.mk b/mk/machine/default/rte.vars.mk
> > index df08d3b03..6bf695849 100644
> > --- a/mk/machine/default/rte.vars.mk
> > +++ b/mk/machine/default/rte.vars.mk
> > @@ -27,4 +27,4 @@
> >  # CPU_LDFLAGS =
> >  # CPU_ASFLAGS =
> >
> > -MACHINE_CFLAGS += -march=corei7
> > +# MACHINE_CFLAGS += -march=corei7


Hi Alexandre, thanks for the bug report and patch. the bug is tracked
by https://tracker.ceph.com/issues/41330, and a fix is posted at
https://github.com/ceph/ceph/pull/29728

> >
> >
> > --
> > Alexandre Oliva, freedom fighter  he/him   https://FSFLA.org/blogs/lxo
> > Be the change, be Free!                 FSF Latin America board member
> > GNU Toolchain Engineer                        Free Software Evangelist
> > Hay que enGNUrecerse, pero sin perder la terGNUra jamás - Che GNUevara
>
>
>
> --
> Cheers,
> Brad
> _______________________________________________
> Dev mailing list -- dev@ceph.io
> To unsubscribe send an email to dev-leave@ceph.io
kefu chai Aug. 19, 2019, 7 a.m. UTC | #3
+ Tone Zhang

On Mon, Aug 19, 2019 at 1:04 PM kefu chai <tchaikov@gmail.com> wrote:
>
> On Mon, Aug 19, 2019 at 9:24 AM Brad Hubbard <bhubbard@redhat.com> wrote:
> >
> > +dev@ceph
> >
> > On Sun, Aug 18, 2019 at 9:50 AM Alexandre Oliva <oliva@gnu.org> wrote:
> > >
> > > Hi,
> > >
> > > After upgrading some old Phenom servers from Fedora/Freed-ora 29 to
> > > 30's, to ceph-14.2.2, I was surprised by very early crashes of both
> > > ceph-mon and ceph-osd.  After ruling out disk and memory corruption, I
> > > investigated a bit noticed all of them crashed during pre-main() init
> > > section processing, at an instruction not available on the Phenom X6
> > > processors that support sse4a, but not e.g. sse4.1.
> > >
> > > It turns out that much of librte is built with -march=corei7.  That's a
> > > little excessive, considering that x86/rte_memcpy.h would be happy
> > > enough with -msse4.1, but not with earlier sse versions that Fedora is
> > > supposed to target.
> > >
> > > I understand rte_memcpy.h is meant for better performance, inlining
> > > fixed-size and known-alignment implementations of memcpy into users.
> > > Alas, that requires setting a baseline target processor, and you'll only
> > > get as efficient an implementation as what's built in.
> > >
> > > I noticed an attempt for dynamic selection, but GCC rightfully won't
> > > inline across different target flags, so we'd have to give up inlining
> > > to get better dynamic behavior.  The good news is that glibc already
> > > offers dynamic selection of memcpy implementations, so hopefully the
> > > impact of this change won't be much worse than that of enabling dynamic
> > > selection, without the complications.
> > >
> > > If that's not good enough, compiling ceph with flags that enable SSE4.1,
> > > AVX2 or AVX512, or with a -march flag that implicitly enables them,
> > > would restore current performance, but without that, you will (with the
> > > patch below) get a package that runs on a broader range of processors,
> > > that the base distro (through the compiler's baseline flags) chooses to
> > > support.  It's not nice when you install a package on a processor that's
> > > supposed to be supported and suddenly you're no longer sure it is ;-)
> > >
> > > Perhaps building a shared librte, so that one could build and install
> > > builds targeting different ISA versions, without having to rebuild all
> > > of ceph, would be a reasonable way to address the better tuning of these
> > > performance-critical bits.
> > >
> > >
> > >
> > > src/spdk/dpdk:
> > >
> > > diff --git a/lib/librte_eal/common/include/arch/x86/rte_memcpy.h b/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
> > > index 7b758094d..ce714bf02 100644
> > > --- a/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
> > > +++ b/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
> > > @@ -40,6 +40,16 @@ extern "C" {
> > >  static __rte_always_inline void *
> > >  rte_memcpy(void *dst, const void *src, size_t n);
> > >
> > > +#ifndef RTE_MACHINE_CPUFLAG_SSE4_1
> > > +
> > > +static __rte_always_inline void *
> > > +rte_memcpy(void *dst, const void *src, size_t n)
> > > +{
> > > +  return memcpy(dst, src, n);
> > > +}
> > > +
> > > +#else /* RTE_MACHINE_CPUFLAG_SSE4_1 */
> > > +
> > >  #ifdef RTE_MACHINE_CPUFLAG_AVX512F
> > >
> > >  #define ALIGNMENT_MASK 0x3F
> > > @@ -869,6 +879,8 @@ rte_memcpy(void *dst, const void *src, size_t n)
> > >                 return rte_memcpy_generic(dst, src, n);
> > >  }
> > >
> > > +#endif /* RTE_MACHINE_CPUFLAG_SSE4_1 */
> > > +
> > >  #ifdef __cplusplus
> > >  }
> > >  #endif
> > > diff --git a/mk/machine/default/rte.vars.mk b/mk/machine/default/rte.vars.mk
> > > index df08d3b03..6bf695849 100644
> > > --- a/mk/machine/default/rte.vars.mk
> > > +++ b/mk/machine/default/rte.vars.mk
> > > @@ -27,4 +27,4 @@
> > >  # CPU_LDFLAGS =
> > >  # CPU_ASFLAGS =
> > >
> > > -MACHINE_CFLAGS += -march=corei7
> > > +# MACHINE_CFLAGS += -march=corei7
>
>
> Hi Alexandre, thanks for the bug report and patch. the bug is tracked
> by https://tracker.ceph.com/issues/41330, and a fix is posted at
> https://github.com/ceph/ceph/pull/29728

after a second thought, i think, probably, instead of patching SPDK to
cater the needs of older machines. a better option is to disable SPDK
when building packages for testing and for our official releases. for
instance, Phenom II X6 belongs to AMD's K10 family which was launched
over a decade ago[0]. while Bobcat family is still being produced [1].

because we don't test SPDK backend in our CI/CD process. and SPDK
backend is not being actively maintained. we could just build it in
our "make check" run though.

what do you think?

--
[0] https://en.wikipedia.org/wiki/Phenom_II
[1] https://en.wikipedia.org/wiki/Bobcat_(microarchitecture)

>
> > >
> > >
> > > --
> > > Alexandre Oliva, freedom fighter  he/him   https://FSFLA.org/blogs/lxo
> > > Be the change, be Free!                 FSF Latin America board member
> > > GNU Toolchain Engineer                        Free Software Evangelist
> > > Hay que enGNUrecerse, pero sin perder la terGNUra jamás - Che GNUevara
> >
> >
> >
> > --
> > Cheers,
> > Brad
> > _______________________________________________
> > Dev mailing list -- dev@ceph.io
> > To unsubscribe send an email to dev-leave@ceph.io
>
>
>
> --
> Regards
> Kefu Chai
Alexandre Oliva Aug. 20, 2019, 6:03 a.m. UTC | #4
On Aug 19, 2019, kefu chai <tchaikov@gmail.com> wrote:

> after a second thought, i think, probably, instead of patching SPDK to
> cater the needs of older machines. a better option is to disable SPDK
> when building packages for testing and for our official releases.

I have no real clue on what role or impact disabling SPDK plays in the
grand scheme of things, so I'm happy to defer to whoever does.  I just
wish to keep my home ceph cluster running on those machines until I have
good reason to replace them.  Unfortunately that's the last generation
of x86 hardware that can run with a Free Software BIOS, and OpenPower
isn't so much of an option for home use.  I hope retaining the ability
to build ceph so that it runs on such old and wise ;-) machines is not a
major problem.

Next in my wishlist is to try to fix the issues that I'm told are
getting in the way of ceph's running on 32-bit x86.  If anyone is
familiar with them and could give me a brain dump to get me started,
that would certainly be appreciated.  I don't really have 32-bit
machines running ceph daemons, but I have often recommended ceph to
people who'd like to run it on SBCs connected to USB storage, so I was
quite surprised and disappointed to find out even x86 wouldn't work any
more.  Plus, that messed up my uniform selection of packages on x86 and
x86-64 machines with a single meta-package with all the dependencies I
care for ;-)
kefu chai Aug. 20, 2019, 9:08 a.m. UTC | #5
On Tue, Aug 20, 2019 at 2:03 PM Alexandre Oliva <oliva@gnu.org> wrote:
>
> On Aug 19, 2019, kefu chai <tchaikov@gmail.com> wrote:
>
> > after a second thought, i think, probably, instead of patching SPDK to
> > cater the needs of older machines. a better option is to disable SPDK
> > when building packages for testing and for our official releases.
>
> I have no real clue on what role or impact disabling SPDK plays in the
> grand scheme of things, so I'm happy to defer to whoever does.  I just
> wish to keep my home ceph cluster running on those machines until I have
> good reason to replace them.  Unfortunately that's the last generation
> of x86 hardware that can run with a Free Software BIOS, and OpenPower
> isn't so much of an option for home use.  I hope retaining the ability
> to build ceph so that it runs on such old and wise ;-) machines is not a
> major problem.

not at this moment =)

>
> Next in my wishlist is to try to fix the issues that I'm told are
> getting in the way of ceph's running on 32-bit x86.  If anyone is
> familiar with them and could give me a brain dump to get me started,
> that would certainly be appreciated.  I don't really have 32-bit
> machines running ceph daemons, but I have often recommended ceph to
> people who'd like to run it on SBCs connected to USB storage, so I was
> quite surprised and disappointed to find out even x86 wouldn't work any
> more.  Plus, that messed up my uniform selection of packages on x86 and
> x86-64 machines with a single meta-package with all the dependencies I
> care for ;-)

i'd say it's a little bit off topic. i think we've fixed a bunch of
issues[0] related to armhf. and i managed to build ceph in a chroot
env for building armhf binaries. so i assume that Ceph does (or did)
build on certain 32bit platforms at least. but since we don't test
and/or build on 32bit platforms, this could change over the time. if
you are able to pinpoint an FTBFS issue or bug while using Ceph on 32
bit platforms. i can take a look at it.

---
[0] for instance, https://github.com/ceph/ceph/pull/25729
>
> --
> Alexandre Oliva, freedom fighter  he/him   https://FSFLA.org/blogs/lxo
> Be the change, be Free!                 FSF Latin America board member
> GNU Toolchain Engineer                        Free Software Evangelist
> Hay que enGNUrecerse, pero sin perder la terGNUra jamás - Che GNUevara
Alexandre Oliva Aug. 20, 2019, 7:26 p.m. UTC | #6
On Aug 20, 2019, kefu chai <tchaikov@gmail.com> wrote:

> On Tue, Aug 20, 2019 at 2:03 PM Alexandre Oliva <oliva@gnu.org> wrote:
>> to build ceph so that it runs on such old and wise ;-) machines is not a
>> major problem.

> not at this moment =)

I can hardly imagine why it should ever be.  Being disk-bound, and
considering that even very old CPUs hardly have trouble keeping up with
the much slower storage devices, I can't imagine a reason for ceph or
anything portable across multiple architectures to tech to *demand*
newer CPUs of any of the supported architectures.  Hopefully there will
always be some portable fallback to resort to.

>> Next in my wishlist is to try to fix the issues that I'm told are
>> getting in the way of ceph's running on 32-bit x86.  If anyone is
>> familiar with them and could give me a brain dump to get me started,
>> that would certainly be appreciated.

> if you are able to pinpoint an FTBFS issue or bug while using Ceph on
> 32 bit platforms. i can take a look at it.

Oh, thanks, I didn't mean to burden anyone with that, it's just
something that I care about and would be happy to undertake myself.  I
was just surprised that there weren't i686 ceph packages in Fedora 30,
and found comments in the spec file suggesting it didn't work, but I
didn't look into why yet.
Alexandre Oliva Aug. 24, 2019, 8:43 p.m. UTC | #7
On Aug 20, 2019, Alexandre Oliva <oliva@gnu.org> wrote:

>> if you are able to pinpoint an FTBFS issue or bug while using Ceph on
>> 32 bit platforms. i can take a look at it.

> Oh, thanks, I didn't mean to burden anyone with that, it's just
> something that I care about and would be happy to undertake myself.  I
> was just surprised that there weren't i686 ceph packages in Fedora 30,
> and found comments in the spec file suggesting it didn't work, but I
> didn't look into why yet.

Here's the patch I used to disable spdk in my rebuilds of the Fedora 30
packages.

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 3b95cc231d2c..ba4fc8506651 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -267,7 +267,7 @@ if(WITH_BLUESTORE)
   endif()
 endif()
 
-if(CMAKE_SYSTEM_PROCESSOR MATCHES "i386|i686|amd64|x86_64|AMD64|aarch64")
+if(CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64")
   option(WITH_SPDK "Enable SPDK" ON)
 else()
   option(WITH_SPDK "Enable SPDK" OFF)

With the following additional tweaks, I could even build 32-bit x86
packages.  The problems seem to be all related with int types.

For example, this one fixes an assumption that size_t and unsigned are
not the same type:

diff --git a/src/include/buffer.h b/src/include/buffer.h
index b8c78210eae0..8e010a150224 100644
--- a/src/include/buffer.h
+++ b/src/include/buffer.h
@@ -737,7 +737,7 @@ inline namespace v14_2_0 {
 
       void advance(int o) = delete;
       void advance(unsigned o);
-      void advance(size_t o) { advance(static_cast<unsigned>(o)); }
+      void advance(unsigned long o) { advance(static_cast<unsigned>(o)); }
       void seek(unsigned o);
       char operator*() const;
       iterator_impl& operator++();

This one was the simplest fix I could find to address problems in other
overloads that did not expect unsigned, only int64_t and uint64_t.
Though I'm not sure this change gets to external representations, I
figured it made sense to settle on a well-defined width instead of
letting it vary across platforms, and to go with the width used in the
prevalent, working platforms:

diff --git a/src/common/config_values.h b/src/common/config_values.h
index ab52060e4629..17eb11d0329d 100644
--- a/src/common/config_values.h
+++ b/src/common/config_values.h
@@ -50,7 +50,7 @@ public:
 #define OPTION_OPT_U32(name) uint64_t name;
 #define OPTION_OPT_U64(name) uint64_t name;
 #define OPTION_OPT_UUID(name) uuid_d name;
-#define OPTION_OPT_SIZE(name) size_t name;
+#define OPTION_OPT_SIZE(name) uint64_t name;
 #define OPTION(name, ty)       \
   public:                      \
     OPTION_##ty(name)          

This is probably the only fallout from the above:

diff --git a/src/osd/PrimaryLogPG.cc b/src/osd/PrimaryLogPG.cc
index 19bd1a3efd5d..0dcf50ca10cb 100644
--- a/src/osd/PrimaryLogPG.cc
+++ b/src/osd/PrimaryLogPG.cc
@@ -1628,7 +1628,7 @@ void PrimaryLogPG::calc_trim_to()
       limit != pg_trim_to &&
       pg_log.get_log().approx_size() > target) {
     size_t num_to_trim = std::min(pg_log.get_log().approx_size() - target,
-                             cct->_conf->osd_pg_log_trim_max);
+				  size_t(cct->_conf->osd_pg_log_trim_max));
     if (num_to_trim < cct->_conf->osd_pg_log_trim_min &&
         cct->_conf->osd_pg_log_trim_max >= cct->_conf->osd_pg_log_trim_min) {
       return;

encode/decode don't handle long at all, they deal with u?int{32,64}_t
and uint64_t.  When int64_t happens to be int or long, this works, but
when int64_t is long long, then long ends up uncovered:

diff --git a/src/test/bufferlist.cc b/src/test/bufferlist.cc
index 8a5bc65d31da..adcdcf3b7b68 100644
--- a/src/test/bufferlist.cc
+++ b/src/test/bufferlist.cc
@@ -837,7 +837,7 @@ TEST(BufferListIterator, iterate_with_empties) {
   EXPECT_EQ(bl.length(), 0u);
   EXPECT_EQ(bl.get_num_buffers(), 1u);
 
-  encode(42l, bl);
+  encode(int64_t(42l), bl);
   EXPECT_EQ(bl.get_num_buffers(), 2u);
 
   bl.push_back(ceph::buffer::create(0));
@@ -853,11 +853,11 @@ TEST(BufferListIterator, iterate_with_empties) {
     bl.append(bl_with_empty_ptr);
   }
 
-  encode(24l, bl);
+  encode(int64_t(24l), bl);
   EXPECT_EQ(bl.get_num_buffers(), 5u);
 
   auto i = bl.cbegin();
-  long val;
+  int64_t val;
   decode(val, i);
   EXPECT_EQ(val, 42l);
 
@@ -865,7 +865,7 @@ TEST(BufferListIterator, iterate_with_empties) {
   EXPECT_EQ(val, 24l);
 
   val = 0;
-  i.seek(sizeof(long));
+  i.seek(sizeof(val));
   decode(val, i);
   EXPECT_EQ(val, 24l);
   EXPECT_TRUE(i == bl.end());
@@ -2665,7 +2665,7 @@ TEST(BufferList, InternalCarriage) {
   ceph::bufferlist bl;
   EXPECT_EQ(bl.get_num_buffers(), 0u);
 
-  encode(42l, bl);
+  encode(int64_t(42l), bl);
   EXPECT_EQ(bl.get_num_buffers(), 1u);
 
   {
@@ -2678,7 +2678,7 @@ TEST(BufferList, InternalCarriage) {
     EXPECT_EQ(bl.get_num_buffers(), 2u);
   }
 
-  encode(24l, bl);
+  encode(int64_t(24l), bl);
   EXPECT_EQ(bl.get_num_buffers(), 3u);
 }
 
@@ -2690,7 +2690,7 @@ TEST(BufferList, ContiguousAppender) {
   {
     auto ap = bl.get_contiguous_appender(100);
 
-    denc(42l, ap);
+    denc(int64_t(42l), ap);
     EXPECT_EQ(bl.get_num_buffers(), 1u);
 
     // append bufferlist with single ptr inside. This should
@@ -2707,11 +2707,11 @@ TEST(BufferList, ContiguousAppender) {
       EXPECT_EQ(bl.get_num_buffers(), 3u);
     }
 
-    denc(24l, ap);
+    denc(int64_t(24l), ap);
     EXPECT_EQ(bl.get_num_buffers(), 3u);
-    EXPECT_EQ(bl.length(), sizeof(long) + 3u);
+    EXPECT_EQ(bl.length(), sizeof(int64_t) + 3u);
   }
-  EXPECT_EQ(bl.length(), 2u * sizeof(long) + 3u);
+  EXPECT_EQ(bl.length(), 2u * sizeof(int64_t) + 3u);
 }
 
 TEST(BufferList, TestPtrAppend) {


That was all it took to build ceph-14.2.2-1.fc30 on x86_64 and i686.  I
haven't even had a chance to check that the resulting packages install,
and I won't be able to get to them before I'm back home on Thu, but
these are simple enough that I figured I'd post them before I got my
attention on something else ;-)

I hope this helps,
kefu chai Aug. 26, 2019, 6:08 a.m. UTC | #8
On Sun, Aug 25, 2019 at 4:43 AM Alexandre Oliva <oliva@gnu.org> wrote:
>
> On Aug 20, 2019, Alexandre Oliva <oliva@gnu.org> wrote:
>
> >> if you are able to pinpoint an FTBFS issue or bug while using Ceph on
> >> 32 bit platforms. i can take a look at it.
>
> > Oh, thanks, I didn't mean to burden anyone with that, it's just
> > something that I care about and would be happy to undertake myself.  I
> > was just surprised that there weren't i686 ceph packages in Fedora 30,
> > and found comments in the spec file suggesting it didn't work, but I
> > didn't look into why yet.
>
> Here's the patch I used to disable spdk in my rebuilds of the Fedora 30
> packages.
>
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index 3b95cc231d2c..ba4fc8506651 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -267,7 +267,7 @@ if(WITH_BLUESTORE)
>    endif()
>  endif()
>
> -if(CMAKE_SYSTEM_PROCESSOR MATCHES "i386|i686|amd64|x86_64|AMD64|aarch64")
> +if(CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64")
>    option(WITH_SPDK "Enable SPDK" ON)
>  else()
>    option(WITH_SPDK "Enable SPDK" OFF)

SPDK is disabled by default now, see https://github.com/ceph/ceph/pull/29728

>
> With the following additional tweaks, I could even build 32-bit x86
> packages.  The problems seem to be all related with int types.
>
> For example, this one fixes an assumption that size_t and unsigned are
> not the same type:
>
> diff --git a/src/include/buffer.h b/src/include/buffer.h
> index b8c78210eae0..8e010a150224 100644
> --- a/src/include/buffer.h
> +++ b/src/include/buffer.h
> @@ -737,7 +737,7 @@ inline namespace v14_2_0 {
>
>        void advance(int o) = delete;
>        void advance(unsigned o);
> -      void advance(size_t o) { advance(static_cast<unsigned>(o)); }
> +      void advance(unsigned long o) { advance(static_cast<unsigned>(o)); }

this function has been removed in a9f8b1a6092a923cae74221d0f1120d106d3cb8f

>        void seek(unsigned o);
>        char operator*() const;
>        iterator_impl& operator++();
>
> This one was the simplest fix I could find to address problems in other
> overloads that did not expect unsigned, only int64_t and uint64_t.
> Though I'm not sure this change gets to external representations, I
> figured it made sense to settle on a well-defined width instead of
> letting it vary across platforms, and to go with the width used in the
> prevalent, working platforms:
>
> diff --git a/src/common/config_values.h b/src/common/config_values.h
> index ab52060e4629..17eb11d0329d 100644
> --- a/src/common/config_values.h
> +++ b/src/common/config_values.h
> @@ -50,7 +50,7 @@ public:
>  #define OPTION_OPT_U32(name) uint64_t name;
>  #define OPTION_OPT_U64(name) uint64_t name;
>  #define OPTION_OPT_UUID(name) uuid_d name;
> -#define OPTION_OPT_SIZE(name) size_t name;
> +#define OPTION_OPT_SIZE(name) uint64_t name;
>  #define OPTION(name, ty)       \
>    public:                      \
>      OPTION_##ty(name)
>
> This is probably the only fallout from the above:

the very same patch was applied in 58b0d514a67ce4f5c115f0f5451d1ac939b3702b

>
> diff --git a/src/osd/PrimaryLogPG.cc b/src/osd/PrimaryLogPG.cc
> index 19bd1a3efd5d..0dcf50ca10cb 100644
> --- a/src/osd/PrimaryLogPG.cc
> +++ b/src/osd/PrimaryLogPG.cc
> @@ -1628,7 +1628,7 @@ void PrimaryLogPG::calc_trim_to()
>        limit != pg_trim_to &&
>        pg_log.get_log().approx_size() > target) {
>      size_t num_to_trim = std::min(pg_log.get_log().approx_size() - target,
> -                             cct->_conf->osd_pg_log_trim_max);
> +                                 size_t(cct->_conf->osd_pg_log_trim_max));
>      if (num_to_trim < cct->_conf->osd_pg_log_trim_min &&
>          cct->_conf->osd_pg_log_trim_max >= cct->_conf->osd_pg_log_trim_min) {
>        return;

i think it's okay now. as OPTION_OPT_U32 is now defined as a uint64_t,
and approx_size() returns uint64_t as well.

>
> encode/decode don't handle long at all, they deal with u?int{32,64}_t
> and uint64_t.  When int64_t happens to be int or long, this works, but
> when int64_t is long long, then long ends up uncovered:
>
> diff --git a/src/test/bufferlist.cc b/src/test/bufferlist.cc
> index 8a5bc65d31da..adcdcf3b7b68 100644
> --- a/src/test/bufferlist.cc
> +++ b/src/test/bufferlist.cc
> @@ -837,7 +837,7 @@ TEST(BufferListIterator, iterate_with_empties) {
>    EXPECT_EQ(bl.length(), 0u);
>    EXPECT_EQ(bl.get_num_buffers(), 1u);
>
> -  encode(42l, bl);
> +  encode(int64_t(42l), bl);
>    EXPECT_EQ(bl.get_num_buffers(), 2u);
>
>    bl.push_back(ceph::buffer::create(0));
> @@ -853,11 +853,11 @@ TEST(BufferListIterator, iterate_with_empties) {
>      bl.append(bl_with_empty_ptr);
>    }
>
> -  encode(24l, bl);
> +  encode(int64_t(24l), bl);
>    EXPECT_EQ(bl.get_num_buffers(), 5u);
>
>    auto i = bl.cbegin();
> -  long val;
> +  int64_t val;
>    decode(val, i);
>    EXPECT_EQ(val, 42l);
>
> @@ -865,7 +865,7 @@ TEST(BufferListIterator, iterate_with_empties) {
>    EXPECT_EQ(val, 24l);
>
>    val = 0;
> -  i.seek(sizeof(long));
> +  i.seek(sizeof(val));
>    decode(val, i);
>    EXPECT_EQ(val, 24l);
>    EXPECT_TRUE(i == bl.end());
> @@ -2665,7 +2665,7 @@ TEST(BufferList, InternalCarriage) {
>    ceph::bufferlist bl;
>    EXPECT_EQ(bl.get_num_buffers(), 0u);
>
> -  encode(42l, bl);
> +  encode(int64_t(42l), bl);
>    EXPECT_EQ(bl.get_num_buffers(), 1u);
>
>    {
> @@ -2678,7 +2678,7 @@ TEST(BufferList, InternalCarriage) {
>      EXPECT_EQ(bl.get_num_buffers(), 2u);
>    }
>
> -  encode(24l, bl);
> +  encode(int64_t(24l), bl);
>    EXPECT_EQ(bl.get_num_buffers(), 3u);
>  }
>
> @@ -2690,7 +2690,7 @@ TEST(BufferList, ContiguousAppender) {
>    {
>      auto ap = bl.get_contiguous_appender(100);
>
> -    denc(42l, ap);
> +    denc(int64_t(42l), ap);
>      EXPECT_EQ(bl.get_num_buffers(), 1u);
>
>      // append bufferlist with single ptr inside. This should
> @@ -2707,11 +2707,11 @@ TEST(BufferList, ContiguousAppender) {
>        EXPECT_EQ(bl.get_num_buffers(), 3u);
>      }
>
> -    denc(24l, ap);
> +    denc(int64_t(24l), ap);
>      EXPECT_EQ(bl.get_num_buffers(), 3u);
> -    EXPECT_EQ(bl.length(), sizeof(long) + 3u);
> +    EXPECT_EQ(bl.length(), sizeof(int64_t) + 3u);
>    }
> -  EXPECT_EQ(bl.length(), 2u * sizeof(long) + 3u);
> +  EXPECT_EQ(bl.length(), 2u * sizeof(int64_t) + 3u);
>  }
>
>  TEST(BufferList, TestPtrAppend) {
>

thanks. i posted your patch as https://github.com/ceph/ceph/pull/29881 .

>
> That was all it took to build ceph-14.2.2-1.fc30 on x86_64 and i686.  I
> haven't even had a chance to check that the resulting packages install,
> and I won't be able to get to them before I'm back home on Thu, but
> these are simple enough that I figured I'd post them before I got my
> attention on something else ;-)
>
> I hope this helps,

it does!

>
> --
> Alexandre Oliva, freedom fighter  he/him   https://FSFLA.org/blogs/lxo
> Be the change, be Free!                 FSF Latin America board member
> GNU Toolchain Engineer                        Free Software Evangelist
> Hay que enGNUrecerse, pero sin perder la terGNUra jamás - Che GNUevara

Patch
diff mbox series

diff --git a/lib/librte_eal/common/include/arch/x86/rte_memcpy.h b/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
index 7b758094d..ce714bf02 100644
--- a/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
+++ b/lib/librte_eal/common/include/arch/x86/rte_memcpy.h
@@ -40,6 +40,16 @@  extern "C" {
 static __rte_always_inline void *
 rte_memcpy(void *dst, const void *src, size_t n);
 
+#ifndef RTE_MACHINE_CPUFLAG_SSE4_1
+
+static __rte_always_inline void *
+rte_memcpy(void *dst, const void *src, size_t n)
+{
+  return memcpy(dst, src, n);
+}
+
+#else /* RTE_MACHINE_CPUFLAG_SSE4_1 */
+
 #ifdef RTE_MACHINE_CPUFLAG_AVX512F
 
 #define ALIGNMENT_MASK 0x3F
@@ -869,6 +879,8 @@  rte_memcpy(void *dst, const void *src, size_t n)
 		return rte_memcpy_generic(dst, src, n);
 }
 
+#endif /* RTE_MACHINE_CPUFLAG_SSE4_1 */
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/mk/machine/default/rte.vars.mk b/mk/machine/default/rte.vars.mk
index df08d3b03..6bf695849 100644
--- a/mk/machine/default/rte.vars.mk
+++ b/mk/machine/default/rte.vars.mk
@@ -27,4 +27,4 @@ 
 # CPU_LDFLAGS =
 # CPU_ASFLAGS =
 
-MACHINE_CFLAGS += -march=corei7
+# MACHINE_CFLAGS += -march=corei7