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

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

On 18/11/15 11:50, Ian Campbell wrote:
> On Wed, 2015-11-18 at 11:23 +0000, Malcolm Crossley wrote:
>> 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.
> 
> So essentially the "per-pcpu read lock" as I called it is really in essence
> a sort of "byte lock" via the NULL vs non-NULL state of the per-cpu pointer
> to the underlying rwlock.

It's not quite a byte lock because it stores a full pointer to the per-$resource
that it's using. It could be changed to be a byte lock but then you will need a
percpu area per-$resource.

> 
>>> 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.
> 
> The rwlock is taken (briefly) by readers if *writer_activating is, isn't
> it?

Yes I got this wrong. Sorry about causing confusion.

Malcolm

> 
> Ian.
>