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

[Julien Grall <julien@xxxxxxx>]

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.
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.
However, I would like to seek clarification on whether your end goal is 
to remove any -ENOMEM failure from set_version.
Cheers,

--
Julien Grall