[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2] gnttab: defer allocation of status frame tracking array
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |