Xen devel - Nov 2013 - [PATCH] xen/balloon: Set balloon's initial state to number of existing RAM pages

Currently balloon''s initial value is set to max_pfn which includes
non-RAM ranges such as MMIO hole. As result, initial memory target
(specified by guest''s configuration file) will appear smaller than
what balloon driver perceives to be the current number of available
pages. Thus it will balloon down "extra" pages, decreasing amount of
available memory for no good reason.

Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
---
 drivers/xen/balloon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
index b232908..1b62304 100644
--- a/drivers/xen/balloon.c
+++ b/drivers/xen/balloon.c
@@ -641,7 +641,7 @@ static int __init balloon_init(void)
 
 	balloon_stats.current_pages = xen_pv_domain()
 		? min(xen_start_info->nr_pages - xen_released_pages, max_pfn)
-		: max_pfn;
+		: get_num_physpages();
 	balloon_stats.target_pages  = balloon_stats.current_pages;
 	balloon_stats.balloon_low   = 0;
 	balloon_stats.balloon_high  = 0;
-- 
1.8.1.4

On Wed, Nov 06, 2013 at 03:37:40PM -0500, Boris Ostrovsky
wrote:
> Currently balloon''s initial value is set to max_pfn which includes
> non-RAM ranges such as MMIO hole. As result, initial memory target
> (specified by guest''s configuration file) will appear smaller than
> what balloon driver perceives to be the current number of available
> pages. Thus it will balloon down "extra" pages, decreasing amount
of
> available memory for no good reason.
Duh!

Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
>  drivers/xen/balloon.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index b232908..1b62304 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -641,7 +641,7 @@ static int __init balloon_init(void)
>  
>  	balloon_stats.current_pages = xen_pv_domain()
>  		? min(xen_start_info->nr_pages - xen_released_pages, max_pfn)
> -		: max_pfn;
> +		: get_num_physpages();
>  	balloon_stats.target_pages  = balloon_stats.current_pages;
>  	balloon_stats.balloon_low   = 0;
>  	balloon_stats.balloon_high  = 0;
> -- 
> 1.8.1.4
>

On 11/07/2013 04:37 AM, Boris Ostrovsky wrote:
> Currently balloon''s initial value is set to max_pfn which includes
> non-RAM ranges such as MMIO hole. As result, initial memory target
> (specified by guest''s configuration file) will appear smaller than
> what balloon driver perceives to be the current number of available
> pages. Thus it will balloon down "extra" pages, decreasing amount
of
> available memory for no good reason.
> 
This fix the strange behavior I mentioned yesterday, every time after
guest started balloon driver will be triggered unreasonably.
> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> ---
>  drivers/xen/balloon.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> index b232908..1b62304 100644
> --- a/drivers/xen/balloon.c
> +++ b/drivers/xen/balloon.c
> @@ -641,7 +641,7 @@ static int __init balloon_init(void)
>  
>  	balloon_stats.current_pages = xen_pv_domain()
>  		? min(xen_start_info->nr_pages - xen_released_pages, max_pfn)
> -		: max_pfn;
> +		: get_num_physpages();
By the way, should the other places using max_pfn also be changed with
get_num_physpages()?
>  	balloon_stats.target_pages  = balloon_stats.current_pages;
>  	balloon_stats.balloon_low   = 0;
>  	balloon_stats.balloon_high  = 0;
> 
-- 
Regards,
-Bob

Bob Liu <bob.liu@oracle.com> wrote:
>
>On 11/07/2013 04:37 AM, Boris Ostrovsky wrote:
>> Currently balloon''s initial value is set to max_pfn which
includes
>> non-RAM ranges such as MMIO hole. As result, initial memory target
>> (specified by guest''s configuration file) will appear smaller
than
>> what balloon driver perceives to be the current number of available
>> pages. Thus it will balloon down "extra" pages, decreasing
amount of
>> available memory for no good reason.
>> 
>
>This fix the strange behavior I mentioned yesterday, every time after
>guest started balloon driver will be triggered unreasonably.
>
>> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
>> ---
>>  drivers/xen/balloon.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
>> index b232908..1b62304 100644
>> --- a/drivers/xen/balloon.c
>> +++ b/drivers/xen/balloon.c
>> @@ -641,7 +641,7 @@ static int __init balloon_init(void)
>>  
>>  	balloon_stats.current_pages = xen_pv_domain()
>>  		? min(xen_start_info->nr_pages - xen_released_pages, max_pfn)
>> -		: max_pfn;
>> +		: get_num_physpages();
>
>By the way, should the other places using max_pfn also be changed with
>get_num_physpages()?
No. In PV that calculation is correct as it gets the amount of RAM pages - which
is a exactly what you need.
>
>>  	balloon_stats.target_pages  = balloon_stats.current_pages;
>>  	balloon_stats.balloon_low   = 0;
>>  	balloon_stats.balloon_high  = 0;
>>

On Wed, Nov 06, 2013 at 08:25:04PM -0500, Konrad Rzeszutek Wilk
wrote:
> Bob Liu <bob.liu@oracle.com> wrote:
> >
> >On 11/07/2013 04:37 AM, Boris Ostrovsky wrote:
> >> Currently balloon''s initial value is set to max_pfn which
includes
> >> non-RAM ranges such as MMIO hole. As result, initial memory target
> >> (specified by guest''s configuration file) will appear
smaller than
> >> what balloon driver perceives to be the current number of
available
> >> pages. Thus it will balloon down "extra" pages,
decreasing amount of
> >> available memory for no good reason.
> >>
> >
> >This fix the strange behavior I mentioned yesterday, every time after
> >guest started balloon driver will be triggered unreasonably.
> >
> >> Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> >> ---
> >>  drivers/xen/balloon.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> >> index b232908..1b62304 100644
> >> --- a/drivers/xen/balloon.c
> >> +++ b/drivers/xen/balloon.c
> >> @@ -641,7 +641,7 @@ static int __init balloon_init(void)
> >>
> >>  	balloon_stats.current_pages = xen_pv_domain()
> >>  		? min(xen_start_info->nr_pages - xen_released_pages,
max_pfn)
> >> -		: max_pfn;
> >> +		: get_num_physpages();
> >
> >By the way, should the other places using max_pfn also be changed with
> >get_num_physpages()?
>
> No. In PV that calculation is correct as it gets the amount of RAM pages -
which is a exactly what you need.
What about PCI passthrough case?

Daniel

On Thu, Nov 07, 2013 at 03:38:32PM +0100, Daniel Kiper
wrote:
> On Wed, Nov 06, 2013 at 08:25:04PM -0500, Konrad Rzeszutek Wilk wrote:
> > Bob Liu <bob.liu@oracle.com> wrote:
> > >
> > >On 11/07/2013 04:37 AM, Boris Ostrovsky wrote:
> > >> Currently balloon''s initial value is set to max_pfn
which includes
> > >> non-RAM ranges such as MMIO hole. As result, initial memory
target
> > >> (specified by guest''s configuration file) will
appear smaller than
> > >> what balloon driver perceives to be the current number of
available
> > >> pages. Thus it will balloon down "extra" pages,
decreasing amount of
> > >> available memory for no good reason.
> > >>
> > >
> > >This fix the strange behavior I mentioned yesterday, every time
after
> > >guest started balloon driver will be triggered unreasonably.
> > >
> > >> Signed-off-by: Boris Ostrovsky
<boris.ostrovsky@oracle.com>
> > >> ---
> > >>  drivers/xen/balloon.c | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
> > >> index b232908..1b62304 100644
> > >> --- a/drivers/xen/balloon.c
> > >> +++ b/drivers/xen/balloon.c
> > >> @@ -641,7 +641,7 @@ static int __init balloon_init(void)
> > >>
> > >>  	balloon_stats.current_pages = xen_pv_domain()
> > >>  		? min(xen_start_info->nr_pages - xen_released_pages,
max_pfn)
> > >> -		: max_pfn;
> > >> +		: get_num_physpages();
> > >
> > >By the way, should the other places using max_pfn also be changed
with
> > >get_num_physpages()?
> >
> > No. In PV that calculation is correct as it gets the amount of RAM
pages - which is a exactly what you need.
> 
> What about PCI passthrough case?
That is still valid. The nr_pages will have the amount of
RAM pages. If the user is using e820_host=1 option
the E820 in an PV guest looks like the host one - and
the Linux kernel ends up ballooning in/out to make the
P2M look like the E820. If not using the e820_host=1
you end up with a big giant E820_RAM - at which point
the ''max_pfn'' is the end of the E820_RAM and
nr_pages - xen_relesed_pages is smaller, or equal.

The end result is that the nr_pages is the amount of
RAM pages that are right now available.
The max_pfn can be different - either bigger or the same.

I guess what you are thinking of is that we have:

	min(nr_pages - xen_released_pages, max_pfn).

And we could just get rid of the ''min'' and just use
nr_pages - xen_released_page. And since the
''nr_pages - xen_released_page'' should be equal to
''get_num_physpages()'' so - why not do that.

But that sounds to me like a new patch - lets not
try to fix too many things at once.
> 
> Daniel
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel