On Tue, 13 Mar 2018, Jesse Williamson wrote: > On Tue, 13 Mar 2018, Adam C. Emerson wrote: > > > My thought is we probably don't really /need/ these checks. If we > > /want/ them, especially in places like Objecter where there's One Lock > > per Class, we should do them statically. This is fairly easy to do in > > the type system if a bit fiddly. The big downside is we'd be replacing > > unique_lock shared_lock with magical template Foo. > > So, if you look through my rough (and incomplete) sketch here, you'll see that > this is exactly what I was doing-- basically to use the type system to > identify the roles of each Mutex. > > https://github.com/chardan/ceph/tree/jfw-wip-monstrous-mutex > > ...and the next step would be to consolidate everything and IF we didn't want > lockdep most everything can become a std::mutex<>, with only a couple > exceptions that would be std::recursive_mutex<>. But, we have a couple of > special behaviors. > > If we /do/ want to keep lockdep around, then the start of the work there > highlights the specific roles that each Mutex and Cond observes. There is in > no case any actual shift in runtime behavior that I saw in going through 219 > source files, I'm very confident that we can succeed in this. I think we definitely *do* want to keep lockdep, but make it a compile-time option. Make vstart dev checkouts default to building with the debug mutex, but production builds with std::mutex. > I will also point out that (now that I've had a look at it again to jog my > memory) there's no way to actually guarantee these properties, looks like I > wrote myself a note: > > // JFW: these are race conditions waiting to happen... should be done away > with! > bool is_locked() const { return nlock > 0; } This one definitely has to die. > bool is_locked_by_me() const { return is_locked() && this_thread::get_id() > == locked_by; } This one is safe in that there is never a false negative, so you can assert(is_locked_by_me()). You cannot assert(!is_locked_by_me()), though. It sounds like you and Adam are pretty set on a more correct check that involves passing through a unique_lock to do the check (vs my suggestion of a macro that is a no-op in the non-debug build case). I have no objection to the former as long as it doesn't overly pollute the code... I worry it has a runtime cost for production builds, though! sage -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
