On Mon, 24 Sep 2018, Gregory Farnum wrote: > 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...) I originally thought we'd transition like that way too, but the current form of Mutex isn't quite right (e.g., recursive and non-recursive mutex is the same type) and cosmetically it's a mess. Adam already implemented the ceph::mutex_debug, which is a Mutex clone that conforms to the C++ style (except for the ctor arguments)... I didn't notice it already did exactly what I wanted until after #24017. In the end that earlier PR that made Mutex conform to the Lockable concept isn't actually a useful stepping stone. In any case, I think migrating directly to ceph::mutex will get us very close to standard c++ style (only real delta is the namespace and factory function). s
