Xen devel - Dec 2009 - [PATCH] linux/balloon: prefer using pages from balloon in alloc_empty_pages_and_pagevec()

This is both faster and less demanding on kernel resources.

Likely also something that could be done in the pv-ops tree (though it
would need some adjustment to deal with the balloon_order!=0 case).

Signed-off-by: Jan Beulich <jbeulich@novell.com>

--- head-2009-12-07.orig/drivers/xen/balloon/balloon.c	2009-12-15
09:37:50.000000000 +0100
+++ head-2009-12-07/drivers/xen/balloon/balloon.c	2009-12-15 09:47:04.000000000
+0100
@@ -607,6 +607,17 @@ struct page **alloc_empty_pages_and_page
 		return NULL;
 
 	for (i = 0; i < nr_pages; i++) {
+		balloon_lock(flags);
+		page = balloon_first_page();
+		if (page && !PageHighMem(page)) {
+			UNLIST_PAGE(page);
+			bs.balloon_low--;
+			balloon_unlock(flags);
+			pagevec[i] = page;
+			continue;
+		}
+		balloon_unlock(flags);
+
 		page = pagevec[i] = alloc_page(GFP_KERNEL|__GFP_COLD);
 		if (page == NULL)
 			goto err;




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> "Jan Beulich" <JBeulich@novell.com> 16.12.09 16:13
>>>
>Likely also something that could be done in the pv-ops tree (though it
>would need some adjustment to deal with the balloon_order!=0 case).
... which seems to be broken at present anyway: The main loops in
{alloc,free}_empty_pages_and_pagevec() both did not get their
upper bound updated correctly (should be npages, but was left to
be nr_pages), for alloc_empty_pages_and_pagevec() the wrong value
is also being used to allocate pagevec[] (though I really think that this
shouldn''t allocate more than a nr_pages vector to avoid a rather large
but unnecessary kmalloc() when nr_pages is small), and in its error
path only a single page gets freed (rather than the whole
balloon_order chunk).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Wednesday 16 December 2009, Jan Beulich wrote:
> ... which seems to be broken at present anyway: The main loops in
> {alloc,free}_empty_pages_and_pagevec() both did not get their
> upper bound updated correctly (should be npages, but was left to
> be nr_pages), for alloc_empty_pages_and_pagevec() the wrong value
> is also being used to allocate pagevec[] (though I really think that this
> shouldn''t allocate more than a nr_pages vector to avoid a rather
large
> but unnecessary kmalloc() when nr_pages is small), and in its error
> path only a single page gets freed (rather than the whole
> balloon_order chunk).
You are correct.  The endpoint of the main loops should be npages, as should 
the variable used in the kmalloc().

I could only allocate the pagevec to be nr_pages and just fill it in with the 
partial order 9 page rather than rounding it up.  I''ll ponder that.

I don''t see anything wrong with the error path.  It correctly walks
back down
the pagevec and frees all the pages it''s allocated so far.

Dave McCracken
Oracle Corp.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> Dave McCracken <dcm@mccr.org> 17.12.09 21:33 >>>
>I don''t see anything wrong with the error path.  It correctly walks
back down
>the pagevec and frees all the pages it''s allocated so far.
Oh, you''re probably looking at the code past the err: label, but I was
referring to the handling of a failed apply_to_page_range().

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Friday 18 December 2009, Jan Beulich wrote:
> >>> Dave McCracken <dcm@mccr.org> 17.12.09 21:33
>>>
> >
> >I don''t see anything wrong with the error path.  It correctly
walks back
> > down  the pagevec and frees all the pages it''s allocated so
far.
> 
> Oh, you''re probably looking at the code past the err: label, but I
was
> referring to the handling of a failed apply_to_page_range().
Ok, yeah.  It frees up the page that''s currently being worked on, then
jumps
to err: to clean up all the other pages in the pagevec and free the pagevec.  
Is there something wrong about it that I''m missing?

Dave McCracken
Oracle Corp

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

>>> Dave McCracken <dcm@mccr.org> 18.12.09 15:32 >>>
>On Friday 18 December 2009, Jan Beulich wrote:
>> >>> Dave McCracken <dcm@mccr.org> 17.12.09 21:33
>>>
>> >
>> >I don''t see anything wrong with the error path.  It
correctly walks back
>> > down  the pagevec and frees all the pages it''s allocated
so far.
>> 
>> Oh, you''re probably looking at the code past the err: label,
but I was
>> referring to the handling of a failed apply_to_page_range().
>
>Ok, yeah.  It frees up the page that''s currently being worked on,
then jumps
>to err: to clean up all the other pages in the pagevec and free the pagevec.
>Is there something wrong about it that I''m missing?
It should __free_pages(..., order) rather than __free_page(...).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Friday 18 December 2009, Jan Beulich wrote:
> >Ok, yeah.  It frees up the page that''s currently being worked
on, then
> > jumps  to err: to clean up all the other pages in the pagevec and free
> > the pagevec. Is there something wrong about it that I''m
missing?
> 
> It should __free_pages(..., order) rather than __free_page(...).
Gah, you''re right.  Of course.  Sorry.  I''ll add that fix to
my patch.

Dave McCracken

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel