Re: [PATCHv6 5/5] gnttab: use per-VCPU maptrack free lists

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

On 23/04/15 17:11, Jan Beulich wrote:
>>>> On 22.04.15 at 18:00, <david.vrabel@citrix.com> wrote:
>>  static inline int
>>  get_maptrack_handle(
>>      struct grant_table *lgt)
>>  {
>> +    struct vcpu          *v = current;
>>      int                   i;
>>      grant_handle_t        handle;
>>      struct grant_mapping *new_mt;
>>      unsigned int          new_mt_limit, nr_frames;
>>  
>> -    spin_lock(&lgt->maptrack_lock);
>> -
>> -    while ( unlikely((handle = __get_maptrack_handle(lgt)) == -1) )
>> -    {
>> -        nr_frames = nr_maptrack_frames(lgt);
>> -        if ( nr_frames >= max_maptrack_frames )
>> -            break;
>> -
>> -        new_mt = alloc_xenheap_page();
>> -        if ( !new_mt )
>> -            break;
>> +    handle = __get_maptrack_handle(lgt, v);
>> +    if ( likely(handle != -1) )
>> +        return handle;
>>  
>> -        clear_page(new_mt);
>> +    nr_frames = nr_vcpu_maptrack_frames(v);
>> +    if ( nr_frames >= max_maptrack_frames )
>> +        return -1;
>>  
>> -        new_mt_limit = lgt->maptrack_limit + MAPTRACK_PER_PAGE;
>> +    new_mt = alloc_xenheap_page();
>> +    if ( !new_mt )
>> +        return -1;
>>  
>> -        for ( i = 1; i < MAPTRACK_PER_PAGE; i++ )
>> -            new_mt[i - 1].ref = lgt->maptrack_limit + i;
>> -        new_mt[i - 1].ref = lgt->maptrack_head;
>> -        lgt->maptrack_head = lgt->maptrack_limit;
>> +    clear_page(new_mt);
>>  
>> -        lgt->maptrack[nr_frames] = new_mt;
>> -        smp_wmb();
>> -        lgt->maptrack_limit      = new_mt_limit;
>> +    new_mt_limit = v->maptrack_limit + MAPTRACK_PER_PAGE;
>>  
>> -        gdprintk(XENLOG_INFO, "Increased maptrack size to %u frames\n",
>> -                 nr_frames + 1);
>> +    for ( i = 1; i < MAPTRACK_PER_PAGE; i++ ){
>> +        new_mt[i - 1].ref = (lgt->maptrack_pages * MAPTRACK_PER_PAGE) + i;
>> +        new_mt[i - 1].vcpu = v->vcpu_id;
>> +    }
>> +    /* Set last entry vcpu and ref */
>> +    new_mt[i - 1].ref = v->maptrack_head;
>> +    new_mt[i - 1].vcpu = v->vcpu_id;
>> +    v->maptrack_head = lgt->maptrack_pages * MAPTRACK_PER_PAGE;
>> +    if (v->maptrack_tail == MAPTRACK_TAIL)
>> +    {
>> +        v->maptrack_tail = (lgt->maptrack_pages * MAPTRACK_PER_PAGE)
>> +            + MAPTRACK_PER_PAGE - 1;
>> +        new_mt[i - 1].ref = MAPTRACK_TAIL;
>>      }
>>  
>> +    spin_lock(&lgt->maptrack_lock);
>> +    lgt->maptrack[lgt->maptrack_pages++] = new_mt;
>>      spin_unlock(&lgt->maptrack_lock);
> 
> The uses of ->maptrack_pages ahead of taking the lock can race
> with updates inside the lock. And with locking elsewhere dropped
> by the previous patch it looks like you can't update ->maptrack[]
> like you do (you'd need a barrier between the pointer store and
> the increment, and with that I think the lock would become
> superfluous if it was needed only for this update).
> 
> Also note the coding style issues in the changes above^(and also
> in code further down).

This was a last minute optimisation, this isn't on the hot patch so
we'll expand the spin_lock to cover all users of maptrack_pages.

Sorry about the coding style problems.

> 
>> -    return handle;
>> +    v->maptrack_limit = new_mt_limit;
>> +
>> +    return __get_maptrack_handle(lgt, v);
> 
> With the lock dropped, nothing guarantees this to succeed, which it
> ought to unless the table size reached its allowed maximum.
> 
>> @@ -1422,6 +1448,17 @@ gnttab_setup_table(
>>      }
>>      gt->maptrack_pages = 0;
>>  
>> +    /* Tracking of mapped foreign frames table */
>> +    if ( (gt->maptrack = xzalloc_array(struct grant_mapping *,
>> +                                       max_maptrack_frames * d->max_vcpus)) == NULL )
>> +        goto out2;
> 
> See the comments on the similar misplaced hunk in the previous
> patch.
> 
>> --- a/xen/include/xen/grant_table.h
>> +++ b/xen/include/xen/grant_table.h
>> @@ -60,6 +60,8 @@ struct grant_mapping {
>>      u32      ref;           /* grant ref */
>>      u16      flags;         /* 0-4: GNTMAP_* ; 5-15: unused */
>>      domid_t  domid;         /* granting domain */
>> +    u32      vcpu;          /* vcpu which created the grant mapping */
>> +    u16      pad[2];
>>  };
> 
> What is this pad[] good for?

The pad is to keep the struct power of 2 sized because this allows the
compiler to optimise these macro's to right and left shifts:

#define MAPTRACK_PER_PAGE (PAGE_SIZE / sizeof(struct grant_mapping))
#define maptrack_entry(t, e) \
    ((t)->maptrack[(e)/MAPTRACK_PER_PAGE][(e)%MAPTRACK_PER_PAGE])


Malcolm
> 
> Jan
>