| Message ID | 20170929112535.82299-3-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 29.09.17 at 13:25, <roger.pau@citrix.com> wrote: > nr_pages doesn't take into account holes or MMIO regions, and > underestimates the amount of memory needed for paging. Be on the safe > side and use max_pdx instead. > > Note that both cases are just approximations, but using max_pdx yields > a number of free pages after Dom0 build always greater than the > minimum reserve (either 1/16 of memory or 128MB, whatever is > smaller). > > Without this patch on a 16GB box the amount of free memory after > building Dom0 without specifying any dom0_mem parameter would be > 122MB, with this patch applied the amount of free memory after Dom0 > build is 144MB, which is greater than the reserved 128MB. For the case of there not being a "dom0_mem=" this may indeed be acceptable (albeit I notice the gap is larger than before, just this time in the right direction). For the supposedly much more common case of there being "dom0_mem=" (and with a positive value), however, not using nr_pages ... > @@ -288,7 +289,7 @@ unsigned long __init dom0_compute_nr_pages( > break; > > /* Reserve memory for shadow or HAP. */ > - avail -= dom0_paging_pages(d, nr_pages); > + avail -= paging_pgs; ... here is likely going to result in a huge overestimation. Also please don't forget to Cc maintainers. Jan
>>> On 13.10.17 at 10:49, <JBeulich@suse.com> wrote: >>>> On 29.09.17 at 13:25, <roger.pau@citrix.com> wrote: >> nr_pages doesn't take into account holes or MMIO regions, and >> underestimates the amount of memory needed for paging. Be on the safe >> side and use max_pdx instead. >> >> Note that both cases are just approximations, but using max_pdx yields >> a number of free pages after Dom0 build always greater than the >> minimum reserve (either 1/16 of memory or 128MB, whatever is >> smaller). >> >> Without this patch on a 16GB box the amount of free memory after >> building Dom0 without specifying any dom0_mem parameter would be >> 122MB, with this patch applied the amount of free memory after Dom0 >> build is 144MB, which is greater than the reserved 128MB. > > For the case of there not being a "dom0_mem=" this may indeed > be acceptable (albeit I notice the gap is larger than before, just > this time in the right direction). For the supposedly much more > common case of there being "dom0_mem=" (and with a positive > value), however, not using nr_pages ... > >> @@ -288,7 +289,7 @@ unsigned long __init dom0_compute_nr_pages( >> break; >> >> /* Reserve memory for shadow or HAP. */ >> - avail -= dom0_paging_pages(d, nr_pages); >> + avail -= paging_pgs; > > ... here is likely going to result in a huge overestimation. Which I realize may or may not be a problem - the question is whether and if so how far the clamping done by nr_pages = min(nr_pages, avail); above here would result in a meaningfully different amount of memory Dom0 may get for certain command line option / total amount of memory combinations. I.e. quite a bit more than a single data point would need to be provided to prove this isn't going to be perceived as a regression by anyone. Jan
On Fri, Oct 13, 2017 at 08:59:29AM +0000, Jan Beulich wrote: > >>> On 13.10.17 at 10:49, <JBeulich@suse.com> wrote: > >>>> On 29.09.17 at 13:25, <roger.pau@citrix.com> wrote: > >> nr_pages doesn't take into account holes or MMIO regions, and > >> underestimates the amount of memory needed for paging. Be on the safe > >> side and use max_pdx instead. > >> > >> Note that both cases are just approximations, but using max_pdx yields > >> a number of free pages after Dom0 build always greater than the > >> minimum reserve (either 1/16 of memory or 128MB, whatever is > >> smaller). > >> > >> Without this patch on a 16GB box the amount of free memory after > >> building Dom0 without specifying any dom0_mem parameter would be > >> 122MB, with this patch applied the amount of free memory after Dom0 > >> build is 144MB, which is greater than the reserved 128MB. > > > > For the case of there not being a "dom0_mem=" this may indeed > > be acceptable (albeit I notice the gap is larger than before, just > > this time in the right direction). For the supposedly much more > > common case of there being "dom0_mem=" (and with a positive > > value), however, not using nr_pages ... > >> @@ -288,7 +289,7 @@ unsigned long __init dom0_compute_nr_pages( > >> break; > >> > >> /* Reserve memory for shadow or HAP. */ > >> - avail -= dom0_paging_pages(d, nr_pages); > >> + avail -= paging_pgs; > > > > ... here is likely going to result in a huge overestimation. > > Which I realize may or may not be a problem - the question is > whether and if so how far the clamping done by > > nr_pages = min(nr_pages, avail); > > above here would result in a meaningfully different amount of > memory Dom0 may get for certain command line option / total > amount of memory combinations. I.e. quite a bit more than a > single data point would need to be provided to prove this isn't > going to be perceived as a regression by anyone. What about using something like max_pdx - total_pages + nr_pages, that seems like a good compromise that should take into account the MMIO holes without overestimating as much as just using max_pdx. Thanks, Roger.
>>> On 13.10.17 at 12:44, <roger.pau@citrix.com> wrote: > On Fri, Oct 13, 2017 at 08:59:29AM +0000, Jan Beulich wrote: >> >>> On 13.10.17 at 10:49, <JBeulich@suse.com> wrote: >> >>>> On 29.09.17 at 13:25, <roger.pau@citrix.com> wrote: >> >> nr_pages doesn't take into account holes or MMIO regions, and >> >> underestimates the amount of memory needed for paging. Be on the safe >> >> side and use max_pdx instead. >> >> >> >> Note that both cases are just approximations, but using max_pdx yields >> >> a number of free pages after Dom0 build always greater than the >> >> minimum reserve (either 1/16 of memory or 128MB, whatever is >> >> smaller). >> >> >> >> Without this patch on a 16GB box the amount of free memory after >> >> building Dom0 without specifying any dom0_mem parameter would be >> >> 122MB, with this patch applied the amount of free memory after Dom0 >> >> build is 144MB, which is greater than the reserved 128MB. >> > >> > For the case of there not being a "dom0_mem=" this may indeed >> > be acceptable (albeit I notice the gap is larger than before, just >> > this time in the right direction). For the supposedly much more >> > common case of there being "dom0_mem=" (and with a positive >> > value), however, not using nr_pages ... >> >> @@ -288,7 +289,7 @@ unsigned long __init dom0_compute_nr_pages( >> >> break; >> >> >> >> /* Reserve memory for shadow or HAP. */ >> >> - avail -= dom0_paging_pages(d, nr_pages); >> >> + avail -= paging_pgs; >> > >> > ... here is likely going to result in a huge overestimation. >> >> Which I realize may or may not be a problem - the question is >> whether and if so how far the clamping done by >> >> nr_pages = min(nr_pages, avail); >> >> above here would result in a meaningfully different amount of >> memory Dom0 may get for certain command line option / total >> amount of memory combinations. I.e. quite a bit more than a >> single data point would need to be provided to prove this isn't >> going to be perceived as a regression by anyone. > > What about using something like max_pdx - total_pages + nr_pages, that > seems like a good compromise that should take into account the MMIO > holes without overestimating as much as just using max_pdx. I'm not sure - I see no clear correlation between max_pdx, total_pages, and possible large MMIO space needs. In fact, max_pdx doesn't express MMIO space needs at all, you'd need to use max_page in that case. Jan
diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c index c997f5e6f5..e2be70c33f 100644 --- a/xen/arch/x86/dom0_build.c +++ b/xen/arch/x86/dom0_build.c @@ -241,6 +241,7 @@ unsigned long __init dom0_compute_nr_pages( { nodeid_t node; unsigned long avail = 0, nr_pages, min_pages, max_pages; + unsigned long paging_pgs = dom0_paging_pages(d, max_pdx); bool need_paging; for_each_node_mask ( node, dom0_nodes ) @@ -256,7 +257,7 @@ unsigned long __init dom0_compute_nr_pages( /* Reserve memory for iommu_dom0_init(). */ if ( iommu_enabled ) - avail -= dom0_paging_pages(d, max_pdx); + avail -= paging_pgs; need_paging = is_hvm_domain(d) && (!iommu_hap_pt_share || !paging_mode_hap(d)); @@ -288,7 +289,7 @@ unsigned long __init dom0_compute_nr_pages( break; /* Reserve memory for shadow or HAP. */ - avail -= dom0_paging_pages(d, nr_pages); + avail -= paging_pgs; } if ( is_pv_domain(d) &&
nr_pages doesn't take into account holes or MMIO regions, and underestimates the amount of memory needed for paging. Be on the safe side and use max_pdx instead. Note that both cases are just approximations, but using max_pdx yields a number of free pages after Dom0 build always greater than the minimum reserve (either 1/16 of memory or 128MB, whatever is smaller). Without this patch on a 16GB box the amount of free memory after building Dom0 without specifying any dom0_mem parameter would be 122MB, with this patch applied the amount of free memory after Dom0 build is 144MB, which is greater than the reserved 128MB. In order to avoid having to calculate the same value twice, add a local variable to store it. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- --- xen/arch/x86/dom0_build.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)