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

[Malcolm Crossley <malcolm.crossley@citrix.com>]

On 18/11/15 10:54, Jan Beulich wrote:
>>>> On 18.11.15 at 11:36, <ian.campbell@citrix.com> wrote:
>> 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@citrix.com> wrote:
>>>>> On 17/11/15 17:04, Jan Beulich wrote:
>>>>>>>>> On 03.11.15 at 18:58, <malcolm.crossley@citrix.com> 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?
> 

The above description is the very good for for how the per-cpu rwlocks behave.
The code stores a pointer to the per-$resource in the percpu area when a user is
reading the per-$resource, this is why the lock is not safe if you take the lock
for two different per-$resource simultaneously. The grant table code only takes
one grant table lock at any one time so it is a safe user.

I would posit that most code behaves in this manner in an attempt to avoid
deadlocks.

It may also be clearer to change the grant_table rwlock_t to a spinlock which
the writers use.

The interesting question is how generic a pattern is the grant table usage of
only a single per-$resource at a time?

The p2m code has it's own recursion detection code and so is safe from that
issue but does it take a read lock for two per-$resource's simultaneously?


> That's not how I understood it, the rwlock isn't per-pCPU (at least not
> in what this patch does - it remains a per-domain one). The per-pCPU
> object is a pointer to an rwlock, which gets made point to whatever
> domain's rwlock the pCPU wants to own.
> 

This description is correct but it's important to note that the rwlock
is only used by the writers and could be effectively replaced with a spinlock.

Malcolm

> Jan
>