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: Make spinlock delay more conventional #14248

Merged
merged 1 commit into from Apr 1, 2017

Conversation

Projects
None yet
4 participants
@badone
Contributor

badone commented Mar 30, 2017

The accepted method of implementing a spinlock delay is the "pause"
instruction.

Signed-off-by: Brad Hubbard bhubbard@redhat.com

@branch-predictor

I'd check if this doesn't kill performance, especially in bufferlists which use spinlocks a lot. It shouldn't, but checking it won't hurt either (I'm not sure if issuing PAUSE won't let CPUs go into low-power mode).

static uint32_t bar = 13;
static uint32_t *foo = &bar;
#if (__arm__ || __aarch64__)

This comment has been minimized.

@tchaikov

tchaikov Mar 30, 2017

Contributor

better off using

#if defined(__arm__) || defined(__aarch64)

and might be better just put this inline assembly in simple_spin_lock() instead of introducing another macro.

This comment has been minimized.

@badone

badone Mar 30, 2017

Contributor

Modelled on our existing code in src/test/test_arch.cc should it be changed as well?

This comment has been minimized.

@tchaikov

tchaikov Mar 31, 2017

Contributor

@badone no, defined() is better in the sense that we are testing if they are defined or not. but C/C++ preprocessor evaluate the undefined macro to 0. so we don't need to change the src/test/test_arch.cc, unless we happen to touch them, and want to make them look better.

static uint32_t *foo = &bar;
#if (__arm__ || __aarch64__)
#define cpu_relax() asm volatile("yield": : :"memory")

This comment has been minimized.

@tchaikov

tchaikov Mar 30, 2017

Contributor

why would you want to add a memory barrier here?

This comment has been minimized.

@badone

badone Mar 30, 2017

Contributor

Nearly all the examples I found online include the memory barrier but I'll remove it as per your suggestion.

@branch-predictor

This comment has been minimized.

Member

branch-predictor commented Mar 30, 2017

In my limited testing, I don't see any impact from this change.

@badone

This comment has been minimized.

Contributor

badone commented Mar 31, 2017

Test this please

for (int i = 0; i < 100000; i++) {
*foo = (*foo * 33) + 17;
}
#if defined(__arm__) || defined(__aarch64)

This comment has been minimized.

@tchaikov

This comment has been minimized.

@badone

badone Mar 31, 2017

Contributor

Well that's what I had originally

}
#if defined(__arm__) || defined(__aarch64__)
asm volatile("yield");
#else

This comment has been minimized.

@tchaikov

tchaikov Mar 31, 2017

Contributor

we need to check for at least #if defined(__i386__) here, AFAIK, we do build on ppc64. in that case, it would be

  asm volatile("or 27,27,27");

and fallback to the for loop if we are compiling on none of them or just emit an error/warning at compile time. IMHO, the latter is acceptable in practice.

see http://stackoverflow.com/questions/5425506/equivalent-of-x86-pause-instruction-for-ppc

@badone

This comment has been minimized.

Contributor

badone commented Mar 31, 2017

@tchaikov how about this then?

for (int i = 0; i < 100000; i++) {
*foo = (*foo * 33) + 17;
}
#if defined(__i386_) || defined(__x86_64__)

This comment has been minimized.

@tchaikov

tchaikov Mar 31, 2017

Contributor

__i386__.

This comment has been minimized.

@tchaikov

tchaikov Mar 31, 2017

Contributor

@badone it's not updated yet. you pushed a wrong commit?

#elif defined(__ppc64__)
asm volatile("or 27,27,27");
#else
static_assert(false,"Unknown architecture");

This comment has been minimized.

@tchaikov

tchaikov Mar 31, 2017

Contributor

or just #error "Unknown architecture". which can be handled by the preprocessor earlier. and you might want to add a space before ".

@badone

This comment has been minimized.

Contributor

badone commented Mar 31, 2017

@tchaikov @branch-predictor thanks for the help guys, sorry it was untidy.

for (int i = 0; i < 100000; i++) {
*foo = (*foo * 33) + 17;
}
#if defined(__i386_) || defined(__x86_64__)

This comment has been minimized.

@tchaikov

tchaikov Mar 31, 2017

Contributor

@badone it's not updated yet. you pushed a wrong commit?

common: Make spinlock delay more conventional
The accepted method of implementing a spinlock delay is the "pause"
instruction.

Signed-off-by: Brad Hubbard <bhubbard@redhat.com>
@badone

This comment has been minimized.

Contributor

badone commented Mar 31, 2017

sigh... beat me by about 30 seconds... :(

@badone

This comment has been minimized.

Contributor

badone commented Mar 31, 2017

Test this please

@tchaikov

This comment has been minimized.

@tchaikov tchaikov merged commit 3f92a85 into ceph:master Apr 1, 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

This comment has been minimized.

Contributor

smithfarm commented Apr 4, 2017

@badone Which architectures was this PR tested on? It appears to have broken the ppc64le and s390x builds. . .

@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Apr 4, 2017

The build error is:

[  672s] [ 33%] Building CXX object src/CMakeFiles/common-objs.dir/common/simple_spin.cc.o
[  672s] /home/abuild/rpmbuild/BUILD/ceph-12.0.1+git.1491313394.3372162/src/common/simple_spin.cc:38:2: error: #error "Unknown architecture"
[  672s]  #error "Unknown architecture"
[  672s]   ^
[  672s] src/CMakeFiles/common-objs.dir/build.make:2415: recipe for target 'src/CMakeFiles/common-objs.dir/common/simple_spin.cc.o' failed
[  672s] make[2]: *** [src/CMakeFiles/common-objs.dir/common/simple_spin.cc.o] Error 1
@smithfarm

This comment has been minimized.

Contributor

smithfarm commented Apr 4, 2017

@badone

This comment has been minimized.

Contributor

badone commented Apr 4, 2017

@smithfarm only tested on x86_64 AFAIK. The problem with ppc should be fixed by #14310 I'm afraid I was unaware that anyone was seriously building in s390x so didn't really consider it... my bad, sorry.

YankunLi pushed a commit to YankunLi/ceph that referenced this pull request Apr 11, 2017

OpRequest: release the message throttle when unregistered
We don't want messages in the OpTracker history hanging on to
message throttle.

Fixes: ceph#14248
Backport: hammer, firefly
Signed-off-by: Samuel Just <sjust@redhat.com>
(cherry picked from commit 67be35c)

Conflicts:
    src/msg/Message.h (s/nullptr/0/ because C++98)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment