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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.01.2021 11:17, Julien Grall wrote:
> Hi Andrew and Jan,
> 
> On 04/01/2021 15:41, Andrew Cooper wrote:
>> On 04/01/2021 15:22, Jan Beulich wrote:
>>> On 04.01.2021 16:04, Andrew Cooper wrote:
>>>> 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.
>>>> I see this as a backwards step.  It turns a build-time -ENOMEM into a
>>>> runtime -ENOMEM, and if you recall from one of the XSAs, the Windows PV
>>>> drivers will BUG() if set_version fails.  (Yes - this is dumb behaviour,
>>>> but it is in the field now.)
> 
> During yesterday call, Paul pointed that this is a behavior from old 
> Windows driver. New Windows PV driver will not use Grant Table v2.
> 
> However, AFAICT, there is already runtime -ENOMEM failure in set_version 
> when trying to populate the status frame (see the call to 
> gnttab_populate_status_frames()).
> 
>>> Well, if this was the only source of -ENOMEM (i.e. none was there
>>> previously), I'd surely understand the concern. But there have been
>>> memory allocations before on this path.
>>
>> ... you're literally writing a patch saying "avoid large allocation at
>> domain create time, and make it at runtime instead" and then trying to
>> argue that it isn't a concern because there are other memory allocations.
>>
>> It is very definitely a backwards step irrespective of the size of the
>> allocation, even if the current behaviour isn't necessarily perfect.
>>
>>>   In any event, this will
>>> need settling between you and Julien, as it was him to request the
>>> change.
>>
>> Well - that's because gnttab v2 is disabled in general on ARM.
> 
> I didn't have Arm in mind when I originally requested Jan the change.
> 
> Instead, the request was based on the fact that most of the guests don't 
> use of grant-table v2. To me this feels a waste of memory and if it can 
> be avoid then it is best.
> 
>>
>> Conditionally avoiding the allocation because of opt_gnttab_max_version
>> being 1 would be ok, because it doesn't introduce new runtime failures
>> for guests.
> 
> Based on my answer above, I would not consider it as a new runtime 
> failure -ENOMEM is already possible when switching from v1 to v2.
> 
> In fact, the allocation is going to be much smaller than the other 
> allocations (we may be allocating multiple pages). So if we are 
> concerned about this, then we should already be concerned about the 
> existings one.

I agree with all arguments further up, but I'd like to correct this
aspect as well as ...

> Anyway, the array itself is likely going to be small (I haven't computed 
> the size) so I would be OK to not defer the allocation.

... this implication: Strictly speaking there's no correlation
between the allocation sizes. The one getting moved here has its
size depend on the maximum number of grants a domain is allowed
to use. The pre-existing allocation is dependent on the number of
grants the domain has or had already in use. Therefore if the
maximum is much larger than the in-use count, the new allocation
may turn out to be larger than the existing one. Yet I agree this
may not be very likely.

Jan