* Sasha Levin <levinsasha928@xxxxxxxxx> wrote: > On Sun, 2011-05-29 at 20:47 +0200, Ingo Molnar wrote: > > * Sasha Levin <levinsasha928@xxxxxxxxx> wrote: > > > > > +++ b/tools/kvm/include/kvm/brlock.h > > > @@ -0,0 +1,12 @@ > > > +#ifndef KVM__BRLOCK_H > > > +#define KVM__BRLOCK_H > > > + > > > +#include "kvm/kvm.h" > > > +#include "kvm/barrier.h" > > > + > > > +#define br_read_lock() mb() > > > +#define br_read_unlock() mb() > > > > These only need to be compiler barrier()s AFAICS, because the 'pause' > > op will signal back to the requestor thread - which whole operation > > is a barrier to begin with. > > I'm wondering why we need a barrier here at all. In this brlock > implementation the readers are waiting on a mutex in their main > loop - right before a call to KVM_RUN. They can't get anywhere near > a br_read_lock() once br_write_lock() has completed. Yes - and i alluded to that in one of my previous mails - but i think we should do a barrier() just to make sure people use them ;-) We don't want huge sections of code assuming readonly data structures. We should probably also add a debug variant that switches this all to rwlocks: that way the correctness of the critical sections can be tested. 5 years down the line we do not want to end up with another 'BKL' kind of situation. > > > +#define br_write_lock() kvm__pause() > > > +#define br_write_unlock() kvm__continue() > > > +#endif > > > > Btw., it might make sense to add a comment to this header file, > > explaining what a 'big reader lock' is :-) > > I'll put the commit message into the header, should be enough? Yeah, that should be more than enough! Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
