Re: [PATCH v2 6/8] kvm tools: Add rwlock wrapper

Sasha Levin
2011-05-30T10:42:27Z

On Mon, 2011-05-30 at 12:30 +0200, Ingo Molnar wrote:
> * Sasha Levin <levinsasha928@gmail.com> wrote:
>
> > On Mon, 2011-05-30 at 12:13 +0200, Ingo Molnar wrote:
> > > * Sasha Levin <levinsasha928@gmail.com> wrote:
> > >
> > > > On Mon, 2011-05-30 at 11:56 +0200, Ingo Molnar wrote:
> > > > > * Sasha Levin <levinsasha928@gmail.com> wrote:
> > > > >
> > > > > > I'm just saying that we're limited to as many VCPU threads as we
> > > > > > can create. br_read_lock() won't do anything on a non-VCPU thread,
> > > > > > which makes it impossible to test it on non-VCPUs.
> > > > >
> > > > > btw., i wondered about that limit - don't we want to fix it?
> > > > >
> > > > > I mean, there's no fundamental reason why brlocks should do 'nothing'
> > > > > in worker threads. In fact it's a subtle breakage waiting AFAICS.
> > > >
> > > > Can they do anything useful without locking? I think we should work
> > > > on integrating an RCU and changing brlocks to use that instead of
> > > > focusing too much on the current implementation.
> > >
> > > What do you mean 'without locking'? If a worker thread uses a
> > > br_read_lock() then that will be 'locking'. It should map to a real
> > > read_lock() in the rwlock debug case, etc.
> > >
> > I meant without locking anything within br_read_lock(), because we
> > wanted to keep the read patch lock-free.
>
> oh, so it's not recursive.
>
> Sane enough - might be worth adding:
>
> br_is_read_locked(&lock)
>
> and a debug check for that into br_read_lock():
>
> BUG_ON(br_is_read_locked(&lock));
>
> > > > This will also fix that limit you don't like :)
> > >
> > > I'd prefer brlocks to more complex solutions in cases where the write
> > > path is very infrequent!
> > >
> > > So we don't want to keep brlocks intentionally crippled.
> >
> > Do you see brlock as a global lock that will pause the entire guest
> > (not just VCPUs - anything except the calling thread)?
>
> Yeah, that's how such brlocks work - life has to stop when there's
> write modifications going on.
>
> There should be a mutex around br_write_lock() itself, to make sure
> two br_write_lock() attempts cannot deadlock each other, but other
> than that it should be pretty straightforward and robust.
>
Yes, It'll need to wait for a thread manager to be added so it could be
un-crippled.

> And note that such a pause/suspend thing might be helpful to do a
> *real* host driven suspend feature in the future: stop all vcpus, all
> worker threads, save state to disk and exit?
>
> Thanks,
>
> Ingo

--

Sasha.