* Sasha Levin <[email protected]> wrote:
> On Sun, 2011-05-29 at 20:47 +0200, Ingo Molnar wrote:
> > * Sasha Levin <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
