Re: [PATCHv11 3/4] gnttab: make the grant table lock a read-write lock

[David Vrabel <david.vrabel@citrix.com>]

On 05/06/15 15:34, Jan Beulich wrote:
>>>> On 02.06.15 at 18:26, <david.vrabel@citrix.com> wrote:
>> @@ -970,9 +988,10 @@ __gnttab_unmap_common(
>>      TRACE_1D(TRC_MEM_PAGE_GRANT_UNMAP, dom);
>>  
>>      rgt = rd->grant_table;
>> -    double_gt_lock(lgt, rgt);
>>  
>> -    op->flags = op->map->flags;
>> +    read_lock(&rgt->lock);
>> +
>> +    op->flags = read_atomic(&op->map->flags);
>>      if ( unlikely(!op->flags) || unlikely(op->map->domid != dom) )
>>      {
>>          gdprintk(XENLOG_WARNING, "Unstable handle %u\n", op->handle);
>> @@ -1019,31 +1038,34 @@ __gnttab_unmap_common(
>>              act->pin -= GNTPIN_hstw_inc;
>>      }
>>  
>> -    if ( gnttab_need_iommu_mapping(ld) )
>> + act_release_out:
>> +    active_entry_release(act);
>> + unmap_out:
>> +    read_unlock(&rgt->lock);
> 
> Trying to answer the question on what protects the maptrack
> entries: With the flags check done first and, after initial setup, the
> ref field never changing, all modifications of flags as well as the
> decision whether to drop the maptrack handle appear to be guarded
> by ref's active entry lock. I think this is implicit enough to require
> being properly spelled out somewhere.
> 
> Together with struct grant_mapping not being used elsewhere (I
> just now created a patch to make this more explicit by moving its
> declaration to the C file) I think this addresses that particular
> concern. If you agree, feel free to add

Yes, this was exactly my reasoning.  I shall improve the docs/comments
to make this clearer in v12.

> Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks!

David