Re: [PATCH 3/4] kvm tools: Add a brlock

[Ingo Molnar <mingo@elte.hu>]

* Sasha Levin <levinsasha928@gmail.com> wrote:

> On Sun, 2011-05-29 at 20:47 +0200, Ingo Molnar wrote:
> > * Sasha Levin <levinsasha928@gmail.com> 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