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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.01.2021 16: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.)
>> 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.

I agree when taking this one possible perspective. There's the other
one as well: For domains never switching to the v2 API, allocating
the table at build time is purely a waste of memory. Note also how
the description says "This array can be large" - it doesn't have to
be, and hence the other involved allocations may be at higher risk
of yielding -ENOMEM.

>>  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'm afraid I don't understand the "because" here: For Arm it's
irrelevant whether v1 or v2 of this patch gets used - the table
would never be allocated anymore in either case.

> 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. 
> 
> The correctness of this change does depend on opt_gnttab_max_version
> being invariant for the lifetime of a domain.  If it were to become a
> runtime parameter, it would need caching per domain, (which is frankly
> how it should have been implemented all along, along with a parameter in
> XEN_DOMCTL_createdomain).

I think it is well understood that conversion of boot time
(command line) options to runtime ones needs careful inspection
of the consumers of the controlled variable(s). The option isn't
a runtime one right now, which is all that counts. (I agree this
would better be a per-domain property set when creating one.)

Jan