Xen devel - Mar 2011 - [PATCH 3/3] mm: Extend memory hotplug API to allow memory hotplug in virtual machines

This patch contains online_page_chain and apropriate functions
for registering/unregistering online page notifiers. It allows
to do some machine specific tasks during online page stage which
is required to implement memory hotplug in virtual machines.
Additionally, __online_page_increment_counters() and
__online_page_free() function was add to ease generic
hotplug operation.

Signed-off-by: Daniel Kiper <dkiper@net-space.pl>
---
 include/linux/memory_hotplug.h |   11 ++++-
 mm/memory_hotplug.c            |   82 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 88 insertions(+), 5 deletions(-)

diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 8122018..d8cc963 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -12,6 +12,9 @@ struct mem_section;
 
 #ifdef CONFIG_MEMORY_HOTPLUG
 
+#define OP_DO_NOT_INCREMENT_TOTAL_COUNTERS	0
+#define OP_INCREMENT_TOTAL_COUNTERS		1
+
 /*
  * Types for free bootmem stored in page->lru.next. These have to be in
  * some random range in unsigned long space for debugging purposes.
@@ -68,12 +71,16 @@ static inline void zone_seqlock_init(struct zone *zone)
 extern int zone_grow_free_lists(struct zone *zone, unsigned long new_nr_pages);
 extern int zone_grow_waitqueues(struct zone *zone, unsigned long nr_pages);
 extern int add_one_highpage(struct page *page, int pfn, int bad_ppro);
-/* need some defines for these for archs that don''t support it */
-extern void online_page(struct page *page);
 /* VM interface that may be used by firmware interface */
 extern int online_pages(unsigned long, unsigned long);
 extern void __offline_isolated_pages(unsigned long, unsigned long);
 
+extern int register_online_page_notifier(struct notifier_block *nb);
+extern int unregister_online_page_notifier(struct notifier_block *nb);
+
+void __online_page_increment_counters(struct page *page, int inc_total);
+void __online_page_free(struct page *page);
+
 #ifdef CONFIG_MEMORY_HOTREMOVE
 extern bool is_pageblock_removable_nolock(struct page *page);
 #endif /* CONFIG_MEMORY_HOTREMOVE */
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index f0651ae..2f62e26 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -29,11 +29,23 @@
 #include <linux/suspend.h>
 #include <linux/mm_inline.h>
 #include <linux/firmware-map.h>
+#include <linux/notifier.h>
 
 #include <asm/tlbflush.h>
 
 #include "internal.h"
 
+/*
+ * online_page_chain contains chain of notifiers called when page is onlined.
+ * When kernel is booting generic_online_page_notifier() is registered with
+ * priority 0 as default notifier. Custom notifier should be registered with
+ * priority > 0. It could be terminal (it should return NOTIFY_STOP on
success)
+ * or not (it should return NOTIFY_DONE or NOTIFY_OK on success; for full list
+ * of return codes look into include/linux/notifier.h).
+ */
+
+static RAW_NOTIFIER_HEAD(online_page_chain);
+
 DEFINE_MUTEX(mem_hotplug_mutex);
 
 void lock_memory_hotplug(void)
@@ -361,27 +373,91 @@ int __remove_pages(struct zone *zone, unsigned long
phys_start_pfn,
 }
 EXPORT_SYMBOL_GPL(__remove_pages);
 
-void online_page(struct page *page)
+int register_online_page_notifier(struct notifier_block *nb)
+{
+	int rc;
+
+	lock_memory_hotplug();
+	rc = raw_notifier_chain_register(&online_page_chain, nb);
+	unlock_memory_hotplug();
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(register_online_page_notifier);
+
+int unregister_online_page_notifier(struct notifier_block *nb)
+{
+	int rc;
+
+	lock_memory_hotplug();
+	rc = raw_notifier_chain_unregister(&online_page_chain, nb);
+	unlock_memory_hotplug();
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(unregister_online_page_notifier);
+
+void __online_page_increment_counters(struct page *page, int inc_total)
 {
 	unsigned long pfn = page_to_pfn(page);
 
-	totalram_pages++;
+	if (inc_total == OP_INCREMENT_TOTAL_COUNTERS)
+		totalram_pages++;
+
 	if (pfn >= num_physpages)
 		num_physpages = pfn + 1;
 
 #ifdef CONFIG_HIGHMEM
-	if (PageHighMem(page))
+	if (inc_total == OP_INCREMENT_TOTAL_COUNTERS && PageHighMem(page))
 		totalhigh_pages++;
 #endif
 
 #ifdef CONFIG_FLATMEM
 	max_mapnr = max(pfn, max_mapnr);
 #endif
+}
+EXPORT_SYMBOL_GPL(__online_page_increment_counters);
 
+void __online_page_free(struct page *page)
+{
 	ClearPageReserved(page);
 	init_page_count(page);
 	__free_page(page);
 }
+EXPORT_SYMBOL_GPL(__online_page_free);
+
+static int generic_online_page_notifier(struct notifier_block *nb, unsigned
long val, void *v)
+{
+	struct page *page = v;
+
+	__online_page_increment_counters(page, OP_INCREMENT_TOTAL_COUNTERS);
+	__online_page_free(page);
+
+	return NOTIFY_OK;
+}
+
+/*
+ * 0 priority makes this the fallthrough default. All
+ * architectures wanting to override this should set
+ * a higher priority and return NOTIFY_STOP to keep
+ * this from running.
+ */
+
+static struct notifier_block generic_online_page_nb = {
+	.notifier_call = generic_online_page_notifier,
+	.priority = 0
+};
+
+static int __init init_online_page_chain(void)
+{
+	return register_online_page_notifier(&generic_online_page_nb);
+}
+pure_initcall(init_online_page_chain);
+
+static void online_page(struct page *page)
+{
+	raw_notifier_call_chain(&online_page_chain, 0, page);
+}
 
 static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,
 			void *arg)
-- 
1.5.6.5

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

On Mon, 2011-03-28 at 11:25 +0200, Daniel Kiper wrote:
> This patch contains online_page_chain and apropriate functions
> for registering/unregistering online page notifiers. It allows
> to do some machine specific tasks during online page stage which
> is required to implement memory hotplug in virtual machines.
> Additionally, __online_page_increment_counters() and
> __online_page_free() function was add to ease generic
> hotplug operation.
I really like that you added some symbolic constants there.  It makes it
potentially a lot more readable.

My worry is that the next person who comes along is going to _really_
scratch their head asking why they would use:
OP_DO_NOT_INCREMENT_TOTAL_COUNTERS or: OP_INCREMENT_TOTAL_COUNTERS.
There aren''t any code comments about it, and the patch description
doesn''t really help.

In the end, we''re only talking about a couple of lines of code for each
case (reordering the function a bit too):

        void online_page(struct page *page)
        {
        // 1. pfn-based bits upping the max physical address markers:
                unsigned long pfn = page_to_pfn(page);
                if (pfn >= num_physpages)
                        num_physpages = pfn + 1;
        #ifdef CONFIG_FLATMEM
                max_mapnr = max(page_to_pfn(page), max_mapnr);
        #endif
        
        // 2. number of pages counters:
                totalram_pages++;
        #ifdef CONFIG_HIGHMEM
                if (PageHighMem(page))
                        totalhigh_pages++;
        #endif
        
        // 3. preparing ''struct page'' and freeing:
                ClearPageReserved(page);
                init_page_count(page);
                __free_page(page);
        }
        
Your stuff already extracted the free stuff very nicely.  I think now we
just need to separate out the totalram_pages/totalhigh_pages bits from
the num_physpages/max_mapnr ones.

If done right, this should also help the totalram_pages/totalhigh_pages
go away balloon_retrieve(), and make Xen less likely to break in the
future.  It also makes it immediately obvious why Xen skips incrementing
those counters: it does it later.

I also note that Xen has a copy of a part of online_page() in its
increase_reservation(): 

                /* Relinquish the page back to the allocator. */
                ClearPageReserved(page);
                init_page_count(page);
                __free_page(page);

That means that Xen is basically carrying an open-coded copy of
online_page() all by itself today.  

-- Dave


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

On Mon, 28 Mar 2011 11:25:07 +0200
Daniel Kiper <dkiper@net-space.pl> wrote:
> This patch contains online_page_chain and apropriate functions
> for registering/unregistering online page notifiers. It allows
> to do some machine specific tasks during online page stage which
> is required to implement memory hotplug in virtual machines.
> Additionally, __online_page_increment_counters() and
> __online_page_free() function was add to ease generic
> hotplug operation.
> 
>  
>  void lock_memory_hotplug(void)
> @@ -361,27 +373,91 @@ int __remove_pages(struct zone *zone, unsigned long
phys_start_pfn,
>  }
>  EXPORT_SYMBOL_GPL(__remove_pages);
>  
> -void online_page(struct page *page)
> +int register_online_page_notifier(struct notifier_block *nb)
> +{
> +	int rc;
> +
> +	lock_memory_hotplug();
> +	rc = raw_notifier_chain_register(&online_page_chain, nb);
> +	unlock_memory_hotplug();
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(register_online_page_notifier);
> +
> +int unregister_online_page_notifier(struct notifier_block *nb)
> +{
> +	int rc;
> +
> +	lock_memory_hotplug();
> +	rc = raw_notifier_chain_unregister(&online_page_chain, nb);
> +	unlock_memory_hotplug();
> +
> +	return rc;
> +}
> +EXPORT_SYMBOL_GPL(unregister_online_page_notifier);
> +
> +void __online_page_increment_counters(struct page *page, int inc_total)
>  {
>  	unsigned long pfn = page_to_pfn(page);
>  
> -	totalram_pages++;
> +	if (inc_total == OP_INCREMENT_TOTAL_COUNTERS)
> +		totalram_pages++;
> +
>  	if (pfn >= num_physpages)
>  		num_physpages = pfn + 1;
>  
>  #ifdef CONFIG_HIGHMEM
> -	if (PageHighMem(page))
> +	if (inc_total == OP_INCREMENT_TOTAL_COUNTERS &&
PageHighMem(page))
>  		totalhigh_pages++;
>  #endif
>  
>  #ifdef CONFIG_FLATMEM
>  	max_mapnr = max(pfn, max_mapnr);
>  #endif
> +}
> +EXPORT_SYMBOL_GPL(__online_page_increment_counters);
>  
> +void __online_page_free(struct page *page)
> +{
>  	ClearPageReserved(page);
>  	init_page_count(page);
>  	__free_page(page);
>  }
> +EXPORT_SYMBOL_GPL(__online_page_free);
> +
> +static int generic_online_page_notifier(struct notifier_block *nb,
unsigned long val, void *v)
> +{
> +	struct page *page = v;
> +
> +	__online_page_increment_counters(page, OP_INCREMENT_TOTAL_COUNTERS);
> +	__online_page_free(page);
> +
> +	return NOTIFY_OK;
> +}
> +
> +/*
> + * 0 priority makes this the fallthrough default. All
> + * architectures wanting to override this should set
> + * a higher priority and return NOTIFY_STOP to keep
> + * this from running.
> + */
> +
> +static struct notifier_block generic_online_page_nb = {
> +	.notifier_call = generic_online_page_notifier,
> +	.priority = 0
> +};
> +
> +static int __init init_online_page_chain(void)
> +{
> +	return register_online_page_notifier(&generic_online_page_nb);
> +}
> +pure_initcall(init_online_page_chain);
> +
> +static void online_page(struct page *page)
> +{
> +	raw_notifier_call_chain(&online_page_chain, 0, page);
> +}
>  
This is a bit strange.  Normally we''ll use a notifier chain to tell
listeners "hey, X just happened".  But this code is different - it
instead uses a notifier chain to tell handlers "hey, do X".  Where in
this case, X is "free a page".

And this (ab)use of notifiers is not a good fit!  Because we have the
obvious problem that if there are three registered noftifiers, we don''t
want to be freeing the page three times.  Hence the tricks with
notifier callout return values.

If there are multiple independent notifier handlers, how do we manage
their priorities?  And what are the effects of the ordering of the
registration calls?

And when one callback overrides an existing one, is there any point in
leaving the original one installed at all?

I dunno, it''s all a bit confusing and strange.  Perhaps it would help
if you were to explain exactly what behaviour you want here, and we can
look to see if there is a more idiomatic way of doing it.



Also...  I don''t think we need (the undocumented)
OP_DO_NOT_INCREMENT_TOTAL_COUNTERS and OP_INCREMENT_TOTAL_COUNTERS. 
Just do

void __online_page_increment_counters(struct page *page,
					bool inc_total_counters);

and pass it "true" or false".

And then document it, please.  The code as you have it contains no
explanation of the inc_total_counters argument and hence no guidance to
others regarding how to use it.


I merged your patch 1/3.

I skipped your patch 2/3, as the new macros appear to have no callers
in this patchset.

I suggest that once we''re happy with them, your patches 2 and 3 be
merged up via whichever tree merges the Xen balloon driver changes. 
That might be my tree, I forget :) Was anyone else thinking of grabbing
them?


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

> I merged your patch 1/3.
> 
> I skipped your patch 2/3, as the new macros appear to have no callers
> in this patchset.
> 
> I suggest that once we''re happy with them, your patches 2 and 3 be
> merged up via whichever tree merges the Xen balloon driver changes. 
> That might be my tree, I forget :) Was anyone else thinking of grabbing
> them?
That would be. I can carry that pathces if this is easier for you.
Would need the Ack-by at some point from mm maintainers when the patches are
to everybody''s satisfaction.


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

On Mon, Mar 28, 2011 at 09:25:24AM -0700, Dave Hansen
wrote:
> On Mon, 2011-03-28 at 11:25 +0200, Daniel Kiper wrote:
> > This patch contains online_page_chain and apropriate functions
> > for registering/unregistering online page notifiers. It allows
> > to do some machine specific tasks during online page stage which
> > is required to implement memory hotplug in virtual machines.
> > Additionally, __online_page_increment_counters() and
> > __online_page_free() function was add to ease generic
> > hotplug operation.
>
> I really like that you added some symbolic constants there.  It makes it
> potentially a lot more readable.
>
> My worry is that the next person who comes along is going to _really_
> scratch their head asking why they would use:
> OP_DO_NOT_INCREMENT_TOTAL_COUNTERS or: OP_INCREMENT_TOTAL_COUNTERS.
> There aren''t any code comments about it, and the patch description
> doesn''t really help.
>
> In the end, we''re only talking about a couple of lines of code for
each
> case (reordering the function a bit too):
>
>         void online_page(struct page *page)
>         {
>         // 1. pfn-based bits upping the max physical address markers:
>                 unsigned long pfn = page_to_pfn(page);
>                 if (pfn >= num_physpages)
>                         num_physpages = pfn + 1;
>         #ifdef CONFIG_FLATMEM
>                 max_mapnr = max(page_to_pfn(page), max_mapnr);
>         #endif
>
>         // 2. number of pages counters:
>                 totalram_pages++;
>         #ifdef CONFIG_HIGHMEM
>                 if (PageHighMem(page))
>                         totalhigh_pages++;
>         #endif
>
>         // 3. preparing ''struct page'' and freeing:
>                 ClearPageReserved(page);
>                 init_page_count(page);
>                 __free_page(page);
>         }
>
> Your stuff already extracted the free stuff very nicely.  I think now we
> just need to separate out the totalram_pages/totalhigh_pages bits from
> the num_physpages/max_mapnr ones.
What do you think about __online_page_increment_counters()
(totalram_pages and totalhigh_pages) and
__online_page_set_limits() (num_physpages and max_mapnr) ???

Daniel

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

On Mon, Mar 28, 2011 at 03:37:35PM -0700, Andrew Morton
wrote:
> On Mon, 28 Mar 2011 11:25:07 +0200
> Daniel Kiper <dkiper@net-space.pl> wrote:
> > +/*
> > + * 0 priority makes this the fallthrough default. All
> > + * architectures wanting to override this should set
> > + * a higher priority and return NOTIFY_STOP to keep
> > + * this from running.
> > + */
> > +
> > +static struct notifier_block generic_online_page_nb = {
> > +	.notifier_call = generic_online_page_notifier,
> > +	.priority = 0
> > +};
> > +
> > +static int __init init_online_page_chain(void)
> > +{
> > +	return register_online_page_notifier(&generic_online_page_nb);
> > +}
> > +pure_initcall(init_online_page_chain);
> > +
> > +static void online_page(struct page *page)
> > +{
> > +	raw_notifier_call_chain(&online_page_chain, 0, page);
> > +}
>
> This is a bit strange.  Normally we''ll use a notifier chain to
tell
> listeners "hey, X just happened".  But this code is different -
it
> instead uses a notifier chain to tell handlers "hey, do X". 
Where in
> this case, X is "free a page".
>
> And this (ab)use of notifiers is not a good fit!  Because we have the
> obvious problem that if there are three registered noftifiers, we
don''t
> want to be freeing the page three times.  Hence the tricks with
> notifier callout return values.
>
> If there are multiple independent notifier handlers, how do we manage
> their priorities?  And what are the effects of the ordering of the
> registration calls?
>
> And when one callback overrides an existing one, is there any point in
> leaving the original one installed at all?
>
> I dunno, it''s all a bit confusing and strange.  Perhaps it would
help
> if you were to explain exactly what behaviour you want here, and we can
> look to see if there is a more idiomatic way of doing it.
OK. I am looking for simple generic mechanism which allow runtime
registration/unregistration of generic or module specific (in that
case Xen) page onlining function. Dave Hansen sugested compile time
solution (https://lkml.org/lkml/2011/2/8/235), however, it does not
fit well in my new project on which I am working on (I am going post
details at the end of April).
> Also...  I don''t think we need (the undocumented)
> OP_DO_NOT_INCREMENT_TOTAL_COUNTERS and OP_INCREMENT_TOTAL_COUNTERS.
> Just do
>
> void __online_page_increment_counters(struct page *page,
> 					bool inc_total_counters);
>
> and pass it "true" or false".
What do you think about __online_page_increment_counters()
(totalram_pages and totalhigh_pages) and
__online_page_set_limits() (num_physpages and max_mapnr) ???

Daniel

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

On Tue, 29 Mar 2011 20:59:13 +0200
Daniel Kiper <dkiper@net-space.pl> wrote:
> > This is a bit strange.  Normally we''ll use a notifier chain
to tell
> > listeners "hey, X just happened".  But this code is
different - it
> > instead uses a notifier chain to tell handlers "hey, do X". 
Where in
> > this case, X is "free a page".
> >
> > And this (ab)use of notifiers is not a good fit!  Because we have the
> > obvious problem that if there are three registered noftifiers, we
don''t
> > want to be freeing the page three times.  Hence the tricks with
> > notifier callout return values.
> >
> > If there are multiple independent notifier handlers, how do we manage
> > their priorities?  And what are the effects of the ordering of the
> > registration calls?
> >
> > And when one callback overrides an existing one, is there any point in
> > leaving the original one installed at all?
> >
> > I dunno, it''s all a bit confusing and strange.  Perhaps it
would help
> > if you were to explain exactly what behaviour you want here, and we
can
> > look to see if there is a more idiomatic way of doing it.
> 
> OK. I am looking for simple generic mechanism which allow runtime
> registration/unregistration of generic or module specific (in that
> case Xen) page onlining function. Dave Hansen sugested compile time
> solution (https://lkml.org/lkml/2011/2/8/235), however, it does not
> fit well in my new project on which I am working on (I am going post
> details at the end of April).
Well, without a complete description of what you''re trying to do and
without any indication of what "does not fit well" means, I''m
at a bit
of a loss to suggest anything.

If we are assured that only one callback will ever be registered at a
time then a simple

typdef void (*callback_t)(struct page *);

static callback_t g_callback;

int register_callback(callback_t callback)
{
	int ret = -EINVAL;

	lock(some_lock);
	if (g_callback == NULL) {
		g_callback = callback;
		ret = 0;
	}
	unlock(some_lock)
	return ret;
}

would suffice.  That''s rather nasty because calls to (*g_callback)
require some_lock.  Use RCU.
> > Also...  I don''t think we need (the undocumented)
> > OP_DO_NOT_INCREMENT_TOTAL_COUNTERS and OP_INCREMENT_TOTAL_COUNTERS.
> > Just do
> >
> > void __online_page_increment_counters(struct page *page,
> > 					bool inc_total_counters);
> >
> > and pass it "true" or false".
> 
> What do you think about __online_page_increment_counters()
> (totalram_pages and totalhigh_pages) and
> __online_page_set_limits() (num_physpages and max_mapnr) ???
I don''t understand the proposal.

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

On Tue, 2011-03-29 at 12:15 -0700, Andrew Morton wrote:
> On Tue, 29 Mar 2011 20:59:13 +0200
> Daniel Kiper <dkiper@net-space.pl> wrote:
> > OK. I am looking for simple generic mechanism which allow runtime
> > registration/unregistration of generic or module specific (in that
> > case Xen) page onlining function. Dave Hansen sugested compile time
> > solution (https://lkml.org/lkml/2011/2/8/235), however, it does not
> > fit well in my new project on which I am working on (I am going post
> > details at the end of April).
> 
> Well, without a complete description of what you''re trying to do
and
> without any indication of what "does not fit well" means,
I''m at a bit
> of a loss to suggest anything.
We need (the arch-independent) online_page() to act differently when
we''re hotplugging a Xen ballooned page versus a normal memory hotplug
operation.  We''ve basically run out of pages to take out of the balloon
and we need some more with which to fill it up (thus the hotplug).  But,
pages _in_ the balloon are not currently in use.  We want to hot-add
pages to the system, but keep them unused.

online_page(page)
{
	// add page to counters and max_pfn 
	...
	
	if (xen_doing_hotplug(page))
		put_page_in_balloon(page);
	else
		free_page(page);
}

Daniel also seems to want to avoid incrementing the counters and then
immediately decrementing them in the Xen code.  I''m not sure it
matters.

-- Dave


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

On Tue, Mar 29, 2011 at 12:15:41PM -0700, Andrew Morton
wrote:
> On Tue, 29 Mar 2011 20:59:13 +0200
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > > This is a bit strange.  Normally we''ll use a notifier
chain to tell
> > > listeners "hey, X just happened".  But this code is
different - it
> > > instead uses a notifier chain to tell handlers "hey, do
X".  Where in
> > > this case, X is "free a page".
> > >
> > > And this (ab)use of notifiers is not a good fit!  Because we have
the
> > > obvious problem that if there are three registered noftifiers, we
don''t
> > > want to be freeing the page three times.  Hence the tricks with
> > > notifier callout return values.
> > >
> > > If there are multiple independent notifier handlers, how do we
manage
> > > their priorities?  And what are the effects of the ordering of
the
> > > registration calls?
> > >
> > > And when one callback overrides an existing one, is there any
point in
> > > leaving the original one installed at all?
> > >
> > > I dunno, it''s all a bit confusing and strange.  Perhaps
it would help
> > > if you were to explain exactly what behaviour you want here, and
we can
> > > look to see if there is a more idiomatic way of doing it.
> >
> > OK. I am looking for simple generic mechanism which allow runtime
> > registration/unregistration of generic or module specific (in that
> > case Xen) page onlining function. Dave Hansen sugested compile time
> > solution (https://lkml.org/lkml/2011/2/8/235), however, it does not
> > fit well in my new project on which I am working on (I am going post
> > details at the end of April).
>
> Well, without a complete description of what you''re trying to do
and
> without any indication of what "does not fit well" means,
I''m at a bit
> of a loss to suggest anything.
The most important thing for me is runtime registration/unregistration.
It will be good if it is possible to register more than one callback
at a time (e.g. for counting), however, it is not required now. It
appears that your proposal fits quite well my requirements. I will check
that. Thank you.
> If we are assured that only one callback will ever be registered at a
> time then a simple
>
> typdef void (*callback_t)(struct page *);
>
> static callback_t g_callback;
>
> int register_callback(callback_t callback)
> {
> 	int ret = -EINVAL;
>
> 	lock(some_lock);
> 	if (g_callback == NULL) {
> 		g_callback = callback;
> 		ret = 0;
> 	}
> 	unlock(some_lock)
> 	return ret;
> }
>
> would suffice.  That''s rather nasty because calls to (*g_callback)
> require some_lock.  Use RCU.
I think that in this case lock_memory_hotplug()/unlock_memory_hotplug()
is much better because it is used for locking during memory hotplug
operation. That means they protect against callback changes during
memory hotplug. It appears sufficient here.
> > > Also...  I don''t think we need (the undocumented)
> > > OP_DO_NOT_INCREMENT_TOTAL_COUNTERS and
OP_INCREMENT_TOTAL_COUNTERS.
> > > Just do
> > >
> > > void __online_page_increment_counters(struct page *page,
> > > 					bool inc_total_counters);
> > >
> > > and pass it "true" or false".
> >
> > What do you think about __online_page_increment_counters()
> > (totalram_pages and totalhigh_pages) and
> > __online_page_set_limits() (num_physpages and max_mapnr) ???
>
> I don''t understand the proposal.
void __online_page_increment_counters(struct page *page)
{
  totalram_pages++;

#ifdef CONFIG_HIGHMEM
  if (PageHighMem(page))
    totalhigh_pages++;
#endif
}

void __online_page_set_limits(struct page *page)
{
  unsigned long pfn = page_to_pfn(page);

  if (pfn >= num_physpages)
    num_physpages = pfn + 1;

#ifdef CONFIG_FLATMEM
  max_mapnr = max(pfn, max_mapnr);
#endif
}

Daniel

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

On Tue, 2011-03-29 at 20:32 +0200, Daniel Kiper wrote:
> > Your stuff already extracted the free stuff very nicely.  I think now
we
> > just need to separate out the totalram_pages/totalhigh_pages bits from
> > the num_physpages/max_mapnr ones.
> 
> What do you think about __online_page_increment_counters()
> (totalram_pages and totalhigh_pages) and
> __online_page_set_limits() (num_physpages and max_mapnr) ??? 
I think there''s a point when "online_page" in there becomes
unnecessary,
but those sound OK to me.

-- Dave


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