Re: [PATCHv9 1/4] gnttab: per-active entry locking

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

On 21/05/15 08:46, Jan Beulich wrote:
>>>> On 20.05.15 at 17:54, <david.vrabel@citrix.com> wrote:
>> @@ -702,6 +729,7 @@ __gnttab_map_grant_ref(
>>  
>>      cache_flags = (shah->flags & (GTF_PAT | GTF_PWT | GTF_PCD) );
>>  
>> +    active_entry_release(act);
>>      spin_unlock(&rgt->lock);
> 
> Just for my understanding: The lock isn't meant to also cover *shah?
> I.e. it could be dropped ahead of the cache_flags assignment?

I think so, but I think it's preferable (for now) to keep the locked
regions as-is where possible.

There are few other places where it looked like the locked regions can
be reduced.  It won't really make much difference though since the read
lock and active entry locks are not contented.

>> @@ -978,7 +1009,7 @@ __gnttab_unmap_common(
>>          if ( (rc = replace_grant_host_mapping(op->host_addr,
>>                                                op->frame, op->new_addr, 
>>                                                op->flags)) < 0 )
>> -            goto unmap_out;
>> +            goto act_release_out;
>>  
>>          ASSERT(act->pin & (GNTPIN_hstw_mask | GNTPIN_hstr_mask));
>>          op->map->flags &= ~GNTMAP_host_map;
> 
> act doesn't look to be accessed anymore after the if() this is
> contained in - why don't you drop the lock as early as possible?
> Or wait - do we need act->pin to remain stable until after that
> subsequent if() (in which case dropping the lock before the
> final if() in the function would mainly make error handling more
> cumbersome without bying us much)?

See comment above.

David