follow-up fixups for atomic_t spinlocks #17611
Conversation
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
Signed-off-by: Jesse Williamson <jwilliamson@suse.de>
chardan
changed the title
|
|
||
| return (0); | ||
| } | ||
|
|
||
| int XioConnection::CState::state_fail(Message* m, uint32_t flags) | ||
| { | ||
| if (! (flags & OP_FLAG_LOCKED)) | ||
| pthread_spin_lock(&xcon->sp); | ||
| xcon->sp.lock(); | ||
|
|
There was a problem hiding this comment.
If we're going to pass a 'locked' or 'unlocked' flag around, then it's better to just pass a reference to unique_lock
This might be more involved than you want to do right now, though.
On the other hand it's one of those "We're touching it doing cleanups anyway, we may as well get all the obvious, relevant ones done" sorta' things.
And since a flag stating whether something is locked and a lock are basically unique_lock except wandering around separately and not exception safe and not as idiomatic, I'd kind of like to replace use of the LOCKED flag with unique_lock.
There was a problem hiding this comment.
Yeah, TBH I'm a bit sick today and thus taking the lazy route exactly as you surmised. ;-)
I also think the current implementation is probably more complicated than it needs to be. I'm ok taking time to Do The Right Thing(TM).
|
|
||
| std::uniform_int_distribution<> dis(0, 1); | ||
| r = dis(random_engine); | ||
| r = ceph::util::generate_random_number(0, 1); | ||
| } |
There was a problem hiding this comment.
The power of libraries!
There was a problem hiding this comment.
Brought a tear to my eye, it did!
There was a problem hiding this comment.
Mind to rename to gen_rand_number? ，to make consistent with rgw naming style (like https://github.com/ceph/ceph/blob/master/src/rgw/rgw_common.cc#L830)
| @@ -160,7 +159,7 @@ void XioConnection::send_keepalive_or_ack(bool ack, const utime_t *tp) | |||
| { | |||
| /* If con is not in READY state, we need to queue the request */ | |||
| if (cstate.session_state.read() != XioConnection::UP) { | |||
| pthread_spin_lock(&sp); | |||
| std::lock_guad<ceph::util::spinlock> lg(sp); | |||
There was a problem hiding this comment.
Yes, and that makes it worrisome that it compiled on both my local system and here... hmm...
There was a problem hiding this comment.
with-xio is set to OFF by default, so cmake -DWITH_XIO=ON maybe?
|
Also, you might want to use guardedly_lock/uniquely_lock, since they're merged in now. (You don't HAVE to, but it'd save you having to type the template argument.) |
|
You just have to install Accelio and enable XIO. There's a README. |
|
@adamemerson For general elucidation, is there a way to turn on such flags for these github builds? |
|
I don't think so. If there is, I don't know about it. |
...tidy up a few things following the larger atomic_t commit. Thanks to @theanalyst for pointing out the missing spot!
@adamemerson Also patches some of the lock/RNG sites mentioned as a consequence of include/random.h being available.
Signed-off-by: Jesse Williamson jwilliamson@suse.de