Re: [Xen-devel] [PATCH 2/2] grant_table: convert grant table rwlock to percpu rwlock

[Ian Campbell <ian.campbell@xxxxxxxxxx>]

On Tue, 2015-11-17 at 17:53 +0000, Andrew Cooper wrote:
> On 17/11/15 17:39, Jan Beulich wrote:
> > > > > On 17.11.15 at 18:30, <andrew.cooper3@xxxxxxxxxx> wrote:
> > > On 17/11/15 17:04, Jan Beulich wrote:
> > > > > > > On 03.11.15 at 18:58, <malcolm.crossley@xxxxxxxxxx> wrote:
> > > > > --- a/xen/common/grant_table.c
> > > > > +++ b/xen/common/grant_table.c
> > > > > @@ -178,6 +178,10 @@ struct active_grant_entry {
> > > > > Â#define _active_entry(t, e) \
> > > > > ÂÂÂÂÂ((t)->active[(e)/ACGNT_PER_PAGE][(e)%ACGNT_PER_PAGE])
> > > > > Â
> > > > > +bool_t grant_rwlock_barrier;
> > > > > +
> > > > > +DEFINE_PER_CPU(rwlock_t *, grant_rwlock);
> > > > Shouldn't these be per grant table? And wouldn't doing so eliminate
> > > > the main limitation of the per-CPU rwlocks?
> > > The grant rwlock is per grant table.
> > That's understood, but I don't see why the above items aren't, too.
> 
> Ah - because there is never any circumstance where two grant tables are
> locked on the same pcpu.

So per-cpu rwlocks are really a per-pcpu read lock with a fallthrough to a
per-$resource (here == granttable) rwlock when any writers are present for
any instance $resource, not just the one where the write lock is desired,
for the duration of any write lock?

It took me a little while to realize this[0] and I think it certain bears
some discussion in code comments around the restrictions and requirements
of using such a lock for a resource. e.g. the very tempting looking per-
$resource rwlock now cannot be used directly.

With that in mind it would be good if as much of this could be encapsulated
as possible.

If this can't easily be done as part of the percpu_rwlock infra[1] then can
we at least have per-$resource helper functions which eliminates the need
to open code "(&get_per_cpu_var(grant_rwlock), &grant_rwlock_barrier, &lgt-
>lock);" everywhere which wants to take a lock.

Or at the very least can we rename d->grant_table->lock to something
uninviting ("rw_inner_lock", "fallback_lock") and, more importantly stick a
whacking great "do not use under any circumstances, use $x instead" next to
it in the struct declaration.

Does this locking scheme have real-world prior-art in other projects? Looks
like Linux at one point might have been going to gain something called per-
cpu rwlocks but a) I'm not sure they were the same and b) they didn't seem
to go anywhere for some reason.

Ian.

[0] FWIW my initial expectation based on the naming was either a per-cpu-
per-resource lock or a global independent rwlocks for each cpu.

[1] e.g. a data structure encapsulating the per-cpu, barrier and offsetof
the underlying lock within a given struct initialised with:

    bool_t grant_rwlock_barrier;
    DEFINE_PER_CPU(rwlock_t *, grant_percpu_rwlock);
    DEFINE_PER_CPU_RWLOCK(grant_rwlock, grant_percpu_rwlock, 
grant_rwlock_barrier, offsetof(struct ...,Ârwlock);

Then lockers just us per_cpu_read_lock(&grant_rwlock) etc.

In fact the first two lines could be part of this macro if people were
willing to tolerate a bit of cpp macro pasting.

Main downside is lack of type safety due to the offset of. Might be
checkable in the macro though.

Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel