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