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

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

On 25/11/15 12:35, Jan Beulich wrote:
>>>> On 20.11.15 at 17:03, <malcolm.crossley@citrix.com> wrote:
>> @@ -208,8 +210,6 @@ active_entry_acquire(struct grant_table *t, grant_ref_t e)
>>  {
>>      struct active_grant_entry *act;
>>  
>> -    ASSERT(rw_is_locked(&t->lock));
> 
> Even if not covering all cases, I don't think this should be dropped,
> just like you don't drop rw_is_write_locked() asserts. Would properly
> replacing this be rather expensive?

I could add a percpu_rw_is_locked function but it will be racy because I can't
set the local lock barrier variable ( set/clearing that would race with real
users of the write lock ) Therefore I would be left polling the percpu readers
which is not reliable without the local barrier variable. Should I still add
the function if it won't reliably detect the locked condition?

> 
>> @@ -3180,7 +3178,7 @@ grant_table_create(
>>          goto no_mem_0;
>>  
>>      /* Simple stuff. */
>> -    rwlock_init(&t->lock);
>> +    percpu_rwlock_resource_init(&t->lock);
> 
> Considering George's general comment (on patch 1), would it perhaps
> make sense to store (in debug builds) the per-CPU variable's offset
> in the lock structure, having percpu_{read,write}_lock() verify it? Or
> at the very least have gt_{read,write}_lock() wrappers, thus
> avoiding the need to explicitly pass grant_rwlock at each use site? I
> certainly agree with George that we should make it as hard as
> possible for someone to get using these constructs wrong.

I can add storing the "per-CPU variable" owner in the local lock structure for
debug builds and have the percpu_{read,write} verify it at runtime.

The downsides is that the size of structures will vary between debug and non-debug
builds and that perhaps users may be confused what the per-CPU variables are used for.

I'll add the wrappers to for the grant table but will this make it harder to determine
how the locking works?

Thanks for the comments

Malcolm
> 
> Jan
>