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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.04.2021 15:15, Julien Grall wrote:
> On 15/04/2021 10:41, 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().
> 
> Given the controversy of the change, I would suggest to summarize why 
> this approach is considered to be ok in the commit message.

I've added "While the delaying of the respective memory allocation adds
possible reasons for failure of the respective enclosing operations,
there are other memory allocations there already, so callers can't
expect these operations to always succeed anyway."

>> @@ -1767,18 +1778,23 @@ status_alloc_failed:
>>           free_xenheap_page(gt->status[i]);
>>           gt->status[i] = NULL;
>>       }
> 
> NIT: can you add a newline here and...
> 
>> +    if ( !nr_status_frames(gt) )
>> +    {
>> +        xfree(gt->status);
>> +        gt->status = ZERO_BLOCK_PTR;
>> +    }
> 
> ... here for readability.

Can do.

>> @@ -1833,12 +1849,11 @@ 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;
>> +    for ( i = 0; i < n; i++ )
>> +        free_xenheap_page(gt->status[i]);
>> +    xfree(gt->status);
>> +    gt->status = ZERO_BLOCK_PTR;
> The new position of the for loop seems unrelated to the purpose of the 
> patch. May I ask why this was done?

Since I was touching this anyway, I thought I could also bring it
into "canonical" order: Up-ing of an array's size should always
first populate the higher entries, then bump the upper bound.
Shrinking of an array's size should always first shrink the upper
bound, then un-populate the higher entries. This may not strictly
be needed here, but I think code we have would better not set bad
precedents (which may otherwise propagate elsewhere).

>> @@ -4047,11 +4062,12 @@ int gnttab_acquire_resource(
>>           if ( gt->gt_version != 2 )
>>               break;
>>   
>> +        rc = gnttab_get_status_frame_mfn(d, final_frame, &tmp);
> 
> NIT: It wasn't obvious to me why gnttab_get_status_frame_mfn() is moved 
> before gt->status. May I suggest to add a in-code comment abouve the 
> ordering?

I've added

        /* This may change gt->status, so has to happen before setting vaddrs. 
*/ 

Jan