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

[Julien Grall <julien@xen.org>]

Hi Jan,

On 29/04/2021 14:40, Jan Beulich wrote:
> 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."

Looks good to me, thanks!

> 
>>> @@ -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).

I am assuming the concern here would be concurrent access. In which 
case, neither of the two versions would be actually be safe.

Anyway, I can see the theory so I am OK with it. However, this is more a 
clean-up than something strictly necessary for this patch. I can live 
with the code beeing modified here, but this at least ought to be 
explained in the commit message.

>>> @@ -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. */

Sounds good to me!

Cheers,

-- 
Julien Grall