On Mon, Sep 24, 2018 at 8:04 AM Sage Weil <sweil@xxxxxxxxxx> wrote: > > ceph::mutex is a new type that's intended to replace use of the legacy > Mutex (pthreads mutex wrapper) and also direct use of std::mutex. > > - For debug builds, ceph::mutex is an alias for ceph::mutex_debug, which > is a Mutex-like implementation from Adam that wraps pthreads and wires > into lockdep--but is also conformant with the Lockable concept. > Similarly, ceph::condition_variable is ceph::condition_variable_debug, > which works with mutex_debug and adds various asserts (taken from Mutex > and Cond) to catch some common patterns of mutex/cond misuse. > > This 'debug' mode is enabled for the notcmalloc shaman builds. > > - For release builds, ceph::mutex etc alias directly to std::mutex so > there is no overhead. > > - do_cmake.sh is updated to -DCMAKE_BUILD_TYPE=Debug by default. This > means you get the debug ceph::mutex version (that does lockdep). It also > means optimizations are turned down so you can gdb through core files > with less pain from symbols optimized away. :) > > - The only code that has been converted to ceph::mutex so far is Finisher. > You can see what that transition looks like here: > > https://github.com/ceph/ceph/pull/24133/commits/8535965ba4298537c489897ccb191a0f458d60d7 > > - lock("Foo::lock") -> lock(ceph::make_mutex("Foo::lock")) in ctor > - Lock() -> lock(), Unlock() -> unlock(), etc. > - Mutex::Locker l(lock) -> unique_lock l(lock) (or lock_guard) > - cond.Signal() -> cond.notify_all() > > Everything is now in place to make these changes across the rest of > the code. I suggest doing this subsystem by subsystem to minimize merge > conflicts and so that each component can independently sequence it through > their testing, resolve conflicts, etc. Hmm, when I saw this PR and https://github.com/ceph/ceph/pull/24107 ("make Mutex Lockable") go by I sort of assumed we'd switch out the guts of Mutex to this. Is there a reason not to do that, other than we want to wean people off the old implementations/aliases? (I guess maybe it means we lose our classic lockdep, but with anything switched over that's not worth much anyway...) -Greg > > sage
