[PATCH 1/3] x86/pvh: fix Dom0 memory accounting

[PATCH 1/3] x86/pvh: fix Dom0 memory accounting

[Roger Pau Monne]

Hello,

The following three patches try to fix memory accounting when building
a PVH Dom0. Patch 1 and 2 are the more relevant ones, since they fix
the actual issues. Patch 3 is a cleanup of the code path in order to
make it simpler.

I consider those bug fixes for PVH Dom0, but since it's still not
possible to create a PVH Dom0 I'm not sure there's much point in
merging those.

Thanks, Roger.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 1/3] x86/dom0: use dom0_paging_pages to account for the memory used by IOMMU pt

[Roger Pau Monne]

Instead of open coding a calculation for it.

Signed-off-by: Roger PAu Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/dom0_build.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index f616b99ddc..c997f5e6f5 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -254,14 +254,9 @@ unsigned long __init dom0_compute_nr_pages(
     if ( is_pv_32bit_domain(d) )
         avail -= d->max_vcpus - 1;
 
-    /* Reserve memory for iommu_dom0_init() (rough estimate). */
+    /* Reserve memory for iommu_dom0_init(). */
     if ( iommu_enabled )
-    {
-        unsigned int s;
-
-        for ( s = 9; s < BITS_PER_LONG; s += 9 )
-            avail -= max_pdx >> s;
-    }
+        avail -= dom0_paging_pages(d, max_pdx);
 
     need_paging = is_hvm_domain(d) &&
         (!iommu_hap_pt_share || !paging_mode_hap(d));
-- 
2.13.5 (Apple Git-94)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 2/3] x86/pvh: use max_pdx to calculate the paging memory usage

[Roger Pau Monne]

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(-)

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) &&
-- 
2.13.5 (Apple Git-94)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH 3/3] x86/dom0: simplify dom0_compute_nr_pages

[Roger Pau Monne]

Remove the loop in dom0_compute_nr_pages.

This is a non-functional change for PV domains, that would always have
need_paging set to false, and thus only did a single loop iteration.

For a PVH Dom0 the loop is not needed anymore, since the amount of
memory needed for paging no longer depends on the amount of memory
assigned to Dom0, but rather on the value of max_pdx. Hence it can be
removed from the amount of available memory before doing the
accounting of memory assigned to Dom0.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/dom0_build.c | 56 +++++++++++++++++++++--------------------------
 1 file changed, 25 insertions(+), 31 deletions(-)

diff --git a/xen/arch/x86/dom0_build.c b/xen/arch/x86/dom0_build.c
index e2be70c33f..7404091943 100644
--- a/xen/arch/x86/dom0_build.c
+++ b/xen/arch/x86/dom0_build.c
@@ -242,7 +242,6 @@ 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 )
         avail += avail_domheap_pages_region(node, 0, 0) +
@@ -258,39 +257,34 @@ unsigned long __init dom0_compute_nr_pages(
     /* Reserve memory for iommu_dom0_init(). */
     if ( iommu_enabled )
         avail -= paging_pgs;
-
-    need_paging = is_hvm_domain(d) &&
-        (!iommu_hap_pt_share || !paging_mode_hap(d));
-    for ( ; ; need_paging = false )
-    {
-        nr_pages = dom0_nrpages;
-        min_pages = dom0_min_nrpages;
-        max_pages = dom0_max_nrpages;
-
+    if ( is_hvm_domain(d) && (!iommu_hap_pt_share || !paging_mode_hap(d)) )
         /*
-         * If allocation isn't specified, reserve 1/16th of available memory
-         * for things like DMA buffers. This reservation is clamped to a
-         * maximum of 128MB.
+         * Reserve memory for shadow or HAP if not sharing the page
+         * tables with the IOMMU.
          */
-        if ( nr_pages == 0 )
-            nr_pages = -min(avail / 16, 128UL << (20 - PAGE_SHIFT));
-
-        /* Negative specification means "all memory - specified amount". */
-        if ( (long)nr_pages  < 0 ) nr_pages  += avail;
-        if ( (long)min_pages < 0 ) min_pages += avail;
-        if ( (long)max_pages < 0 ) max_pages += avail;
-
-        /* Clamp according to min/max limits and available memory. */
-        nr_pages = max(nr_pages, min_pages);
-        nr_pages = min(nr_pages, max_pages);
-        nr_pages = min(nr_pages, avail);
-
-        if ( !need_paging )
-            break;
-
-        /* Reserve memory for shadow or HAP. */
         avail -= paging_pgs;
-    }
+
+    nr_pages = dom0_nrpages;
+    min_pages = dom0_min_nrpages;
+    max_pages = dom0_max_nrpages;
+
+    /*
+     * If allocation isn't specified, reserve 1/16th of available memory
+     * for things like DMA buffers. This reservation is clamped to a
+     * maximum of 128MB.
+     */
+    if ( nr_pages == 0 )
+        nr_pages = -min(avail / 16, 128UL << (20 - PAGE_SHIFT));
+
+    /* Negative specification means "all memory - specified amount". */
+    if ( (long)nr_pages  < 0 ) nr_pages  += avail;
+    if ( (long)min_pages < 0 ) min_pages += avail;
+    if ( (long)max_pages < 0 ) max_pages += avail;
+
+    /* Clamp according to min/max limits and available memory. */
+    nr_pages = max(nr_pages, min_pages);
+    nr_pages = min(nr_pages, max_pages);
+    nr_pages = min(nr_pages, avail);
 
     if ( is_pv_domain(d) &&
          (parms->p2m_base == UNSET_ADDR) && (dom0_nrpages <= 0) &&
-- 
2.13.5 (Apple Git-94)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 1/3] x86/dom0: use dom0_paging_pages to account for the memory used by IOMMU pt

[Jan Beulich]

>>> On 29.09.17 at 13:25, <roger.pau@citrix.com> wrote:
> Instead of open coding a calculation for it.

But how well do the two calculation results match up? In particular
I doubt the IOMMU page table reservation needs is vCPU count
dependent.

> --- a/xen/arch/x86/dom0_build.c
> +++ b/xen/arch/x86/dom0_build.c
> @@ -254,14 +254,9 @@ unsigned long __init dom0_compute_nr_pages(
>      if ( is_pv_32bit_domain(d) )
>          avail -= d->max_vcpus - 1;
>  
> -    /* Reserve memory for iommu_dom0_init() (rough estimate). */
> +    /* Reserve memory for iommu_dom0_init(). */

Is the new result any less rough an estimate compared to what
we've been using so far? Quite the opposite, I would guess.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/3] x86/pvh: use max_pdx to calculate the paging memory usage

[Jan Beulich]

>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/3] x86/pvh: use max_pdx to calculate the paging memory usage

[Jan Beulich]

>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 3/3] x86/dom0: simplify dom0_compute_nr_pages

[Jan Beulich]

>>> On 29.09.17 at 13:25, <roger.pau@citrix.com> wrote:
> Remove the loop in dom0_compute_nr_pages.
> 
> This is a non-functional change for PV domains, that would always have
> need_paging set to false, and thus only did a single loop iteration.
> 
> For a PVH Dom0 the loop is not needed anymore, since the amount of
> memory needed for paging no longer depends on the amount of memory
> assigned to Dom0, but rather on the value of max_pdx. Hence it can be
> removed from the amount of available memory before doing the
> accounting of memory assigned to Dom0.

Which, as said in reply to patch 2, may be slightly or meaningfully
wrong. If both earlier patches were proven to be fine, the one here
can have my ack.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/3] x86/pvh: use max_pdx to calculate the paging memory usage

[Roger Pau Monné]

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH 2/3] x86/pvh: use max_pdx to calculate the paging memory usage

[Jan Beulich]

>>> 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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel