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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.11.15 at 14:43, <malcolm.crossley@xxxxxxxxxx> wrote:
> On 25/11/15 12:35, Jan Beulich wrote:
>>>>> On 20.11.15 at 17:03, <malcolm.crossley@xxxxxxxxxx> 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?

Iiuc that would then risk the ASSERT() producing false positives,
which clearly is a no-go. In which case I'd still like you to leave in
some sort of comment (perhaps just the current code converted
to a comment) to document the requirement.

>>> @@ -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'm not sure what confusion might arise here.

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

Why would it? At the call sites we don't really care about the
mechanism. But perhaps I'm not seeing what you're thinking of...

Jan


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