Re: fix for hidden corei7 requirement in binary packages
+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
-- Cheers, Brad
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
-- Regards Kefu Chai
+ 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
-- Regards Kefu Chai
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 ;-) -- 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
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
-- Regards Kefu Chai
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, 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
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, -- 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
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
-- Regards Kefu Chai
participants (3)
-
Alexandre Oliva
-
Brad Hubbard
-
kefu chai