common: consolidate spinlocks #15816

chardan · 2017-06-21T19:35:01Z

Completes migration away from SpinLock.h to wrappers around std::atomic_flag.

simple_spin_lock() and ceph_spin_lock() are now consolidated as ceph::spin_lock(), any remaining instances of spinlock_t move to std::atomic_flag.

Refer-to:
#14337

See-also minibenchmark here (short version: the standard library is fast):
https://github.com/chardan/ceph-spinlock-minibench

adamemerson · 2017-06-28T18:25:01Z

src/include/spinlock.h

+  void lock() const {
+    ceph::spin_lock(spinlock);
+  }
+  /// release spinlock


Why is this const? (Not that it's wrong, it just seems odd.)

It just tells the compiler and reader that this member function causes no side-effects on the object itself. In addition to expressing intent, it can enable some extra optimizations.

But the only thing this class does is wrap the actual spin_lock, which is for some reason declared mutable. What optimizations could it possibly perform that are worth that confusion? Or are we grabbing non-mutable locks in chat functions that require this chicanery?

@gregsfortytwo The answer to that very good question is "that's how Spinlock.h did it" and I wanted to avoid breaking any code that was declaring const locks (which make no sense to me, either!). I think this whole thing can go away by using std::unique_lock<>, which I'm very happy to consider as either an extension of this PR or follow-up PR.

Oh. This must be because in the mists of time, declaring locks mutable was annoying. I wonder if I did that. o_0
Follow-up PR sounds good to me!

adamemerson · 2017-06-28T18:26:03Z

src/include/spinlock.h

+  // Scoped control of a Spinlock:
+  class Locker {
+    const Spinlock& spinlock;
+  public:


Rather than adding our own Locker, couldn't we use unique_lock with the spinlock class?

I believe that we definitely could! After the current changeset, let's either consider that as a next-step of this PR, or as the "next PR".

adamemerson · 2017-06-28T18:27:17Z

src/include/spinlock.h

+
+class Spinlock {
+  mutable std::atomic_flag spinlock { false };
+


Shouldn't this be ATOMIC_FLAG_INIT? I didn't think any other way was guaranteed to initialize it properly.

...aaaand, you would be right! I thought I'd found a way around that with aggregate initialization, which does indeed /work/-- but, it turns out that it's a non-standard library extension (N3690, 29.7). I'll correct it and also look through recent atomic code to see if anything using atomic flag is initialized in this way. Thanks for catching this!

adamemerson · 2017-06-28T18:29:45Z

src/common/ceph_context.cc

@@ -285,11 +294,11 @@ class CephContextObs : public md_config_obs_t {
                          const std::set <std::string> &changed) override {
    if (changed.count(
 	  "enable_experimental_unrecoverable_data_corrupting_features")) {
-      ceph_spin_lock(&cct->_feature_lock);
+      ceph::spin_lock(&cct->_feature_lock);
      get_str_set(


I'd be tempted to replace all of these calls to direct lock/unlock with scoped lockers.

That thought's crossed my mind, too-- seemed out of scope (hah-hah!) for this PR. That said, I'm happy to do it (and things should be easy to search for now). One of the big benefits is that scoped locks are safe in the face of any thrown exceptions, and if we happen to have any case where we've missed those might "instantly" fix a bug or two.

I don't want to PILE WORK ON YOU that could in principle be done later, but if you're willing to do it I'd love to see us use scoped lockers instead of explicit locks/unlocks..

As with std::lock_guard, I'm absolutely happy to do it-- but it may be a better "next" PR than this one (or, alternatively, it may be perfect for this one). I guess the question is whether the cycles are better spent here, or on trying to fix things for Luminous.

chardan · 2017-06-28T23:22:49Z

If folks are starting to feel good about this, I propose one of these two next steps:

consolidate the PRs into 1 commit;
1a) follow up commit(s) that moves most cases to std::unique_lock<> or std::lock_guard<>
OR
1b) same as 1a, but separate PR

...I'm ok with either of these, obviously 1a will take a bit longer, where as 1b could be ready for testing "now".

adamemerson · 2017-06-29T01:55:11Z

I would prefer 1a, simply on the theory that if we're going to have to touch every lock/unlock call anyway we may as well unique_lockify them now. However since you're doing the work I'd defer to whichever judgment you mentioned.

Though since the weird const/mutable stuff is a historical accident I'd strongly prefer we get rid of that in either case.

chardan · 2017-06-29T02:28:42Z

@adamemerson I'll ponder it a bit-- 1a makes sense because now that the locking is indeed "consolidated", it's easier to see how the standard library can be applied. Same with the idea of a lock that's const. Let me look and see what it would take from this point.

chardan · 2017-06-29T04:35:35Z

src/common/buffer.cc

@@ -44,8 +44,8 @@ using namespace ceph;

 #ifdef BUFFER_DEBUG
 static std::atomic_flag buffer_debug_lock = ATOMIC_FLAG_INIT;
-# define bdout { simple_spin_lock(&buffer_debug_lock); std::cout
-# define bendl std::endl; simple_spin_unlock(&buffer_debug_lock); }
+# define bdout { ceph::spin_lock(&buffer_debug_lock); std::cout


This ends up being the one place I keep the bare spin_lock() calls... who knows what that macro does? (Would it be safe to use the scope-guard version..?)

It should be since every bdout has to be paired with a bendl to close the scope, analogous to dout/dendl.

Well, actually, if it /doesn't/ match I guess we'll get a compiler error, so will-do!

chardan · 2017-06-29T04:53:18Z

Have to put the pen down for a bit-- but, unfortunately std::atomic_flag does not model a /simple lockable/ type, so direct use of std::lock_guard<> and std::unique_lock<> won't work for it. But, it looks like there are several ways around this. I've started in, though I'm beginning to have second thoughts about trying to get it into this PR-- I'll pick up with a fresher mind!

adamemerson · 2017-06-29T08:55:49Z

Is there any reason to have/use a bare std::atomic_flag in Ceph as opposed to just using the ceph::spn_lock class?

I'd think from the perspective of both clarity (YES, I know this thing IS a Spinlock...not that std::atomic flag is good for much else, but still.)

And from the perspective of substitutability (i.e. if I decide I want a spinlock somewhere I have a std::mutex or vice versa I can just change the member type and not every lock/unlock call, at least assuming people using the lock factories.)

chardan · 2017-06-29T16:08:25Z

@adamemerson I'll mull it over a bit more, but I don't think there's any reason to use the bare spinlock (we /may/ want to provide a method to expose it, but I see no need for that right now), and there are at least two additional advantages-- you can't forget to initialize the std::atomic_flag, and you can get a /simple lockable/ model. I think what I'll probably wind up doing is implementing a new spinlock class (or two, for different semantics) that will work with the standard library. I'll see if I can make some headway, but I have vacation coming up and that's starting to nudge me toward wanting to make it a separate PR again. There are also some other cleanups, like bare pthreads locks, that should happen.

adamemerson · 2017-06-29T16:11:42Z

@chardan That sounds good. And, happy vacating.

gregsfortytwo · 2017-06-29T17:02:16Z

Was just chatting in irc and I trust you guys to get this right so definitely not doing a detailed review — but we definitely don't want to condition "switching to the standard library" on "making everything use unique_lock/Locker variants". There are (perhaps unfortunately) places where that's not feasible with our locking constraints and trying to identify and switch all of them before we migrate our new stuff is just going to drag everybody down into misery.

adamemerson · 2017-06-29T17:04:49Z

@gregsfortytwo Oh, sure. Where it's not appropriate I agree completely. I know of at least ONE place where it doesn't make sense off the top of my head.

It's just one of those things that I /prefer/ to do whenever it's reasonable and possible without vast restructuring since it eliminates a bunch of potential bugs in error pathways and whatnot.

chardan · 2017-07-13T05:10:29Z

Sound good! Looks like the next step is code review?

Most cases did indeed work out to require only RAII guards! A powerful idiom.

adamemerson

This is a fine and good patch.

Launch Grand Thaumaturgic Miracles

adamemerson · 2017-07-21T17:21:48Z

src/include/spinlock.h

+ * Foundation.  See file COPYING.
+ *
+ * @author Sage Weil <sage@inktank.com>
+ */


Credit yourself in the file you wrote :)

Not to mention the copyright date! Thanks for mentioning this.

adamemerson · 2017-07-21T17:24:15Z

src/msg/Messenger.cc

-    std::lock_guard<Spinlock> lock(random_lock);
+    static std::mutex random_lock;
+    std::lock_guard<std::mutex> lock(random_lock);
+


I assume this goes away when your random PR gets pulled in.

Hmm... yep, looks like that whole block of code can likely vanish! Thanks for the reminder that I should check for cases where we use <random> as well as calls to rand().

chardan · 2017-07-21T21:13:36Z

@liewegas ping... does this look all right to you?

liewegas · 2017-07-21T21:23:04Z

Yep! Let's merge this post-luminous, though. I won't want to be hunting down some weird regression due to a typo here.

chardan · 2017-07-21T21:23:26Z

(Note: I'm adding a versioning namespace, up shortly.)

chardan · 2017-07-21T21:23:49Z

@liewegas Ok, thank you!

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

ceph::spinlock models /BasicLockable/, and works with standard library types like std::lock_guard<>. Signed-off-by: Jesse Williamson <jwilliamson@suse.de>

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

adamemerson · 2017-08-16T14:37:45Z

Since we've forked luminous, is there anything this is waiting on before it can be merged?

chardan · 2017-08-21T19:25:13Z

@liewegas (You have to imagine Rod Serling saying this) "Submitted for your approval."

liewegas · 2017-08-31T20:47:15Z

http://pulpito.ceph.com/sage-2017-08-31_16:29:40-rados-wip-sage-testing2-2017-08-31-0908-distro-basic-smithi/
had one suspicious failure i think was another pr, but want to test this one again anyway!

chardan · 2017-08-31T23:16:08Z

Sounds good-- let me know how I can help!

liewegas · 2017-09-01T21:27:54Z

http://pulpito.ceph.com/sage-2017-09-01_13:34:04-rados-wip-sage-testing2-2017-08-31-1648-distro-basic-smithi/

liewegas · 2017-09-01T21:28:18Z

\o/

chardan · 2017-09-01T21:41:45Z

Yay!

chardan force-pushed the jfw-wip-consolidate-spinlocks branch from 61d31f5 to 85d1b94 Compare June 21, 2017 20:32

liewegas added cleanup common labels Jun 21, 2017

chardan force-pushed the jfw-wip-consolidate-spinlocks branch 3 times, most recently from 87cdee1 to 786653d Compare June 22, 2017 21:00

liewegas added the needs-qa label Jun 27, 2017

adamemerson reviewed Jun 28, 2017

View reviewed changes

chardan force-pushed the jfw-wip-consolidate-spinlocks branch 3 times, most recently from eb258e9 to 35e1d3d Compare June 28, 2017 22:54

chardan force-pushed the jfw-wip-consolidate-spinlocks branch from 35e1d3d to 0141c7d Compare June 28, 2017 23:24

chardan force-pushed the jfw-wip-consolidate-spinlocks branch 3 times, most recently from ac236df to 04d7f08 Compare June 29, 2017 04:34

chardan commented Jun 29, 2017

View reviewed changes

chardan force-pushed the jfw-wip-consolidate-spinlocks branch 5 times, most recently from 674fa52 to c7754e9 Compare June 29, 2017 18:22

chardan force-pushed the jfw-wip-consolidate-spinlocks branch from f9578ee to 18aef06 Compare July 20, 2017 14:51

adamemerson approved these changes Jul 21, 2017

View reviewed changes

chardan force-pushed the jfw-wip-consolidate-spinlocks branch from 18aef06 to b5b454b Compare July 21, 2017 19:30

Jesse Williamson added 11 commits August 11, 2017 14:03

common: adds spinlock.h, migrates code; removes Spinlock.h

b1abde9

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

common: add spin_lock_guard

d3495ec

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

common: apply ceph::spin_lock_guard

00dedde

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

common: adds ceph::spinlock, removes ceph::Spinlock

b2aa2f3

ceph::spinlock models /BasicLockable/, and works with standard library types like std::lock_guard<>. Signed-off-by: Jesse Williamson <jwilliamson@suse.de>

common: migrate ceph::Spinlock to ceph::spinlock

1048e2c

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

msg: apply ceph::spin_lock_guard

6789ca6

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

msg: pthread_spinlock() migration to ceph::spinlock

76d5bea

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

os: migrate ceph::Spinlock to ceph::spinlock

e287178

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

osd: migrate to ceph::spinlock

f93655e

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

compressor: migrate ceph::Spinlock to ceph::spinlock

a529605

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

common (spinlock): add versioning namespace

7415375

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

chardan force-pushed the jfw-wip-consolidate-spinlocks branch from 3922b1d to 7415375 Compare August 11, 2017 21:03

liewegas added needs-qa wip-sage2-testing labels Aug 30, 2017

liewegas merged commit 3682651 into ceph:master Sep 1, 2017


		class Spinlock {
		mutable std::atomic_flag spinlock { false };

common: consolidate spinlocks #15816

common: consolidate spinlocks #15816

Conversation

chardan commented Jun 21, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregsfortytwo Jun 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chardan commented Jun 28, 2017

adamemerson commented Jun 29, 2017

chardan commented Jun 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chardan commented Jun 29, 2017

adamemerson commented Jun 29, 2017

chardan commented Jun 29, 2017

adamemerson commented Jun 29, 2017

gregsfortytwo commented Jun 29, 2017

adamemerson commented Jun 29, 2017

chardan commented Jul 13, 2017 • edited

adamemerson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chardan commented Jul 21, 2017

liewegas commented Jul 21, 2017

chardan commented Jul 21, 2017 • edited

chardan commented Jul 21, 2017

adamemerson commented Aug 16, 2017

chardan commented Aug 21, 2017

liewegas commented Aug 31, 2017

chardan commented Aug 31, 2017

liewegas commented Sep 1, 2017

liewegas commented Sep 1, 2017

chardan commented Sep 1, 2017

chardan commented Jun 21, 2017 •

edited

gregsfortytwo Jun 28, 2017 •

edited

chardan commented Jul 13, 2017 •

edited

chardan commented Jul 21, 2017 •

edited