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

Sasha Levin
2011-05-30T10:23:14Z

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.

> > 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)?

> Currently we should at minimum need to BUG_ON() if br_read_lock() or
> br_write_lock() is called in a worker thread context, right?
>
> > > We should have enumeration for all threads that kvm starts, and
> > > that we can use for a generic pause/resume facility. Can you see
> > > anything that prevents that model?
> >
> > Theres a difference between how you would pause a VCPU thread as
> > opposed to a non-VCPU thread, other than that - no. We could use a
> > thread manager.
>
> Exactly, which would be useful for other purposes as well :-)
>
> This only involves the write path so it is not an issue.
>
> Thanks,
>
> Ingo

--

Sasha.