Re: [PATCH v2] gnttab: defer allocation of status frame tracking array

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.12.2020 10:57, Julien Grall wrote:
> Hi Jan,
> 
> On 23/12/2020 15:13, Jan Beulich wrote:
>> This array can be large when many grant frames are permitted; avoid
>> allocating it when it's not going to be used anyway, by doing this only
>> in gnttab_populate_status_frames().
>>
>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> ---
>> v2: Defer allocation to when a domain actually switches to the v2 grant
>>      API.
>>
>> --- a/xen/common/grant_table.c
>> +++ b/xen/common/grant_table.c
>> @@ -1725,6 +1728,17 @@ gnttab_populate_status_frames(struct dom
>>       /* Make sure, prior version checks are architectural visible */
>>       block_speculation();
>>   
>> +    if ( gt->status == ZERO_BLOCK_PTR )
>> +    {
>> +        gt->status = xzalloc_array(grant_status_t *,
>> +                                   
>> grant_to_status_frames(gt->max_grant_frames));
>> +        if ( !gt->status )
>> +        {
>> +            gt->status = ZERO_BLOCK_PTR;
>> +            return -ENOMEM;
>> +        }
>> +    }
>> +
>>       for ( i = nr_status_frames(gt); i < req_status_frames; i++ )
>>       {
>>           if ( (gt->status[i] = alloc_xenheap_page()) == NULL )
>> @@ -1745,18 +1759,23 @@ status_alloc_failed:
>>           free_xenheap_page(gt->status[i]);
>>           gt->status[i] = NULL;
>>       }
>> +    if ( !nr_status_frames(gt) )
>> +    {
>> +        xfree(gt->status);
>> +        gt->status = ZERO_BLOCK_PTR;
>> +    }
>>       return -ENOMEM;
>>   }
>>   
>>   static int
>>   gnttab_unpopulate_status_frames(struct domain *d, struct grant_table *gt)
>>   {
>> -    unsigned int i;
>> +    unsigned int i, n = nr_status_frames(gt);
>>   
>>       /* Make sure, prior version checks are architectural visible */
>>       block_speculation();
>>   
>> -    for ( i = 0; i < nr_status_frames(gt); i++ )
>> +    for ( i = 0; i < n; i++ )
>>       {
>>           struct page_info *pg = virt_to_page(gt->status[i]);
>>           gfn_t gfn = gnttab_get_frame_gfn(gt, true, i);
>> @@ -1811,12 +1830,12 @@ gnttab_unpopulate_status_frames(struct d
>>           page_set_owner(pg, NULL);
>>       }
>>   
>> -    for ( i = 0; i < nr_status_frames(gt); i++ )
>> -    {
>> -        free_xenheap_page(gt->status[i]);
>> -        gt->status[i] = NULL;
>> -    }
>>       gt->nr_status_frames = 0;
>> +    smp_wmb(); /* Just in case - all accesses should be under lock. */
> 
> I think gt->status cannot be accessed locklessly. If a entity read 
> gt->nr_status_frames != 0, then there is no promise the array will be 
> accessible when trying to access it as it may have been freed.

Yet the common principle of (pointer,count) pairs to describe arrays
to be updated / accessed in sequences guaranteeing a non-zero count
implies a valid pointer could as well be considered to apply here.
I.e. when freeing, at least in principle clearing count first would
be a sensible thing to do, wouldn't it? Subsequently allocation and
consumers could be updated to follow suit ...

> So I would drop the barrier here.

I certainly can (unless double checking would tell me otherwise),
but I'm not convinced this is a good idea. I'd rather have a barrier
too many in the code than one too little, even if the "too little"
may only be the result of a future change. And this isn't a path
where performance considerations would suggest avoiding barriers
when they're not strictly needed.

Jan