New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

common: support s390 and unknown architectures in spin-wait loop #14337

Merged
merged 1 commit into from Apr 6, 2017

Conversation

Projects
None yet
3 participants
@smithfarm
Contributor

smithfarm commented Apr 5, 2017

Fixes: http://tracker.ceph.com/issues/19492
Signed-off-by: Nathan Cutler ncutler@suse.com

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Apr 5, 2017

@badone @tchaikov The jury is still out on whether s390 has such an assembler instruction. This is one way the issue could be dealt with - are we sure we want to break builds for architectures not recognized in this #if block, though? Would it make sense to put the old code into the #else ?

@tchaikov

This comment has been minimized.

Contributor

tchaikov commented Apr 5, 2017

are we sure we want to break builds for architectures not recognized in this #if block, though?

i am not sure now. maybe we should #else them.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Apr 5, 2017

@kefu So, like this?

@@ -18,6 +18,11 @@
#include <stdint.h>
#include <pthread.h>
#if !defined(__i386__) && !defined(__x86_64__) && !defined(__arm__) && !defined(__aarch64__) && !defined(__powerpc__) && !defined(__ppc__)

This comment has been minimized.

@tchaikov

tchaikov Apr 5, 2017

Contributor

i feel guilty at seeing this. but it works!

@tchaikov tchaikov added the needs-qa label Apr 5, 2017

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Apr 5, 2017

One colleague suggested the detail namespace idiom, so I added a commit for that. Another colleague wrote:

I don't know s390, but looks like in kernel we have just a simple compiler barrier for s390 implementation of cpu_relax() (which is done by "asm pause" on x86).

The compiler barrier is:

#define barrier() __asm__ __volatile__("": : :"memory")

So I added a commit for that as well. Please re-review.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Apr 5, 2017

Pushed wip-19492 to ceph-ci.

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Apr 5, 2017

Builds fine on shaman, also builds successfully on all four architectures (x86_64, aarch64, ppc64le, and s390x) in OBS.

@badone

This comment has been minimized.

Contributor

badone commented Apr 5, 2017

I'd be interested to see the assembly produced for that on s390(x) (even though I know little about the arch). On x86_64 at least it appears to produce no instructions, https://godbolt.org/g/i6vrMP If that is what we are using in the kernel (I looked originally and this is what I found also) I guess it's the best we can do. I puzzle over what sort of DAS s390(x) ceph installations are using, but I guess that's a discussion for another forum?

void simple_spin_lock(simple_spinlock_t *lock)
{
while(1) {

This comment has been minimized.

@tchaikov

tchaikov Apr 6, 2017

Contributor

could take this chance to add a space after while.

This comment has been minimized.

@smithfarm

smithfarm Apr 6, 2017

Contributor

done

@@ -18,25 +18,42 @@
#include <stdint.h>
#include <pthread.h>
void simple_spin_lock(simple_spinlock_t *lock)
namespace detail {

This comment has been minimized.

@tchaikov

tchaikov Apr 6, 2017

Contributor

i don't think detail namespace would help here. the "detail" namespace is usually used in headers. but an anonymous namespace does. and would be better if we can rename delay to something like cpu_relax or just pause.

This comment has been minimized.

@smithfarm

smithfarm Apr 6, 2017

Contributor

ok, done

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Apr 6, 2017

@badone The purpose of the s390 build is to enable mainframe clients to access external Ceph clusters over the network.

common: support s390 and unknown architectures in spin-wait loop
This fixes the s390 build which was broken by 3448176

It also reinstates the previous architecture-independent for-loop
implementation as a catch-all for "unknown" (not-explicitly-supported)
architectures.

Fixes: http://tracker.ceph.com/issues/19492
Signed-off-by: Nathan Cutler <ncutler@suse.com>
@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Apr 6, 2017

@badone Regarding "": : :"memory" I found this: https://en.wikipedia.org/wiki/Memory_ordering#Compile-time_memory_barrier_implementation

I also read http://x86.renejeschke.de/html/file_module_x86_id_232.html which raises the question in my mind whether we need a real delay here, or just a memory barrier, or both?

@badone

badone approved these changes Apr 6, 2017

LGTM

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Apr 6, 2017

As agreed on IRC, pushed latest version to ceph-ci and will run it through rados/thrash for validation.

@smithfarm smithfarm changed the title from common: support s390(x) in spin-wait loop to common: support s390 and unknown architectures in spin-wait loop Apr 6, 2017

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Apr 6, 2017

teuthology-suite -k distro --priority 101 --suite rados/thrash --subset $(expr $RANDOM % 2000)/2000 --email ncutler@suse.com --ceph wip-19492 --machine-type smithi

pass http://pulpito.ceph.com:80/smithfarm-2017-04-06_12:17:51-rados:thrash-wip-19492-distro-basic-smithi/

@smithfarm smithfarm merged commit 3c89ca8 into ceph:master Apr 6, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@smithfarm smithfarm deleted the smithfarm:wip-19492 branch Apr 6, 2017

chardan added a commit to chardan/ceph that referenced this pull request Apr 6, 2017

common: Implements simple_spin_t in terms of std::atomic_flag.
Removes the requirement of machine-specific code in favor of the standard
library. See also PR ceph#14337.

Signed-off-by: Jesse Williamson <jwilliamson@suse.de>

chardan added a commit to chardan/ceph that referenced this pull request Apr 7, 2017

common: Implements simple_spin_t in terms of std::atomic_flag
Removes the requirement of machine-specific code in favor of the standard
library. See also PR ceph#14337.

Signed-off-by: Jesse Williamson <jwilliamson@suse.de>

chardan added a commit to chardan/ceph that referenced this pull request Apr 7, 2017

common: Implements simple_spin_t in terms of std::atomic_flag
Removes the requirement of machine-specific code in favor of the standard
library. See also PR ceph#14337.

Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment