This patch contains online_page_callback and apropriate functions for
setting/restoring online page callbacks. 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_set_limits(),
__online_page_increment_counters() and __online_page_free() function
was added to ease generic hotplug operation.
Signed-off-by: Daniel Kiper <[email protected]>
Reviewed-by: Konrad Rzeszutek Wilk <[email protected]>
---
include/linux/memory_hotplug.h | 11 +++++-
mm/memory_hotplug.c | 68 ++++++++++++++++++++++++++++++++++++++--
2 files changed, 74 insertions(+), 5 deletions(-)
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 8122018..0b8e2a7 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -68,12 +68,19 @@ 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);
+typedef void (*online_page_callback_t)(struct page *page);
+
+extern int set_online_page_callback(online_page_callback_t callback);
+extern int restore_online_page_callback(online_page_callback_t callback);
+
+extern void __online_page_set_limits(struct page *page);
+extern void __online_page_increment_counters(struct page *page);
+extern 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 a807ccb..9d47c39 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -34,6 +34,17 @@
#include "internal.h"
+/*
+ * online_page_callback contains pointer to current page onlining function.
+ * Initially it is generic_online_page(). If it is required it could be
+ * changed by calling set_online_page_callback() for callback registration
+ * and restore_online_page_callback() for generic callback restore.
+ */
+
+static void generic_online_page(struct page *page);
+
+static online_page_callback_t online_page_callback = generic_online_page;
+
DEFINE_MUTEX(mem_hotplug_mutex);
void lock_memory_hotplug(void)
@@ -361,23 +372,74 @@ int __remove_pages(struct zone *zone, unsigned long phys_start_pfn,
}
EXPORT_SYMBOL_GPL(__remove_pages);
-void online_page(struct page *page)
+int set_online_page_callback(online_page_callback_t callback)
+{
+ int rc = -EINVAL;
+
+ lock_memory_hotplug();
+
+ if (online_page_callback == generic_online_page) {
+ online_page_callback = callback;
+ rc = 0;
+ }
+
+ unlock_memory_hotplug();
+
+ return rc;
+}
+EXPORT_SYMBOL_GPL(set_online_page_callback);
+
+int restore_online_page_callback(online_page_callback_t callback)
+{
+ int rc = -EINVAL;
+
+ lock_memory_hotplug();
+
+ if (online_page_callback == callback) {
+ online_page_callback = generic_online_page;
+ rc = 0;
+ }
+
+ unlock_memory_hotplug();
+
+ return rc;
+}
+EXPORT_SYMBOL_GPL(restore_online_page_callback);
+
+void __online_page_set_limits(struct page *page)
{
unsigned long pfn = page_to_pfn(page);
- totalram_pages++;
if (pfn >= num_physpages)
num_physpages = pfn + 1;
+}
+EXPORT_SYMBOL_GPL(__online_page_set_limits);
+
+void __online_page_increment_counters(struct page *page)
+{
+ totalram_pages++;
#ifdef CONFIG_HIGHMEM
if (PageHighMem(page))
totalhigh_pages++;
#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 void generic_online_page(struct page *page)
+{
+ __online_page_set_limits(page);
+ __online_page_increment_counters(page);
+ __online_page_free(page);
+}
static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,
void *arg)
@@ -388,7 +450,7 @@ static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,
if (PageReserved(pfn_to_page(start_pfn)))
for (i = 0; i < nr_pages; i++) {
page = pfn_to_page(start_pfn + i);
- online_page(page);
+ online_page_callback(page);
onlined_pages++;
}
*(unsigned long *)arg = onlined_pages;
--
1.5.6.5
On Tue, 17 May 2011, Daniel Kiper wrote:
> This patch contains online_page_callback and apropriate functions for
> setting/restoring online page callbacks. 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_set_limits(),
> __online_page_increment_counters() and __online_page_free() function
> was added to ease generic hotplug operation.
>
There are several issues with this.
First, this is completely racy and only allows one global callback to be
in use at a time without looping, which is probably why you had to pass an
argument to restore_online_page_callback(). Your implementation also
requires that a callback must be synchronized with itself for the
comparison to generic_online_page to make any sense. Nobody knows which
callback is effective at any given moment and has no guarantees that when
they've set the callback that it will be the one called, otherwise.
Second, there's no explanation offered about why you have to split
online_page() into three separate functions. In addition, you've exported
all of them so presumably modules will need to be doing this when loading
or unloading and that further complicates the race mentioned above.
Third, there are no followup patches that use this interface or show how
you plan on using it (other than eluding that it will be used for virtual
machines in the changelog) so we're left guessing as to why we need it
implemented in this fashion and restricts the amount of help I can offer
because I don't know the problem you're facing.
On Wed, May 18, 2011 at 08:36:02PM -0700, David Rientjes wrote:
> On Tue, 17 May 2011, Daniel Kiper wrote:
>
> > This patch contains online_page_callback and apropriate functions for
> > setting/restoring online page callbacks. 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_set_limits(),
> > __online_page_increment_counters() and __online_page_free() function
> > was added to ease generic hotplug operation.
>
> There are several issues with this.
>
> First, this is completely racy and only allows one global callback to be
> in use at a time without looping, which is probably why you had to pass an
One callback is allowed by design. Currently I do not see
any real usage for more than one callback.
> argument to restore_online_page_callback(). Your implementation also
This is protection against accidental callback restore
by module which does not registered callback.
> requires that a callback must be synchronized with itself for the
> comparison to generic_online_page to make any sense. Nobody knows which
This is protection against accidental earlier registered callback
overwrite by module which does not registered callback.
> callback is effective at any given moment and has no guarantees that when
> they've set the callback that it will be the one called, otherwise.
It is assured by design described above that if module registered callback
then it will be called during online page phase (If it is not earlier
unregistered by module knowing address to that callback).
> Second, there's no explanation offered about why you have to split
> online_page() into three separate functions. In addition, you've exported
> all of them so presumably modules will need to be doing this when loading
> or unloading and that further complicates the race mentioned above.
My work on memory hotplug for Xen showed that most of the code from original
online_page() is called in my implementation of Xen online_page(). In that
situation Dave Hansen and I agreed that it is worth to split original
online_page() into let say "atomic" operations and export them to
other modules to reuse existing code and avoid stupid bugs.
> Third, there are no followup patches that use this interface or show how
> you plan on using it (other than eluding that it will be used for virtual
> machines in the changelog) so we're left guessing as to why we need it
> implemented in this fashion and restricts the amount of help I can offer
> because I don't know the problem you're facing.
Patch which depends on that patch is here: https://lkml.org/lkml/2011/5/17/413.
However, I agree that comment is not clear.
In general I see that comment for this
patch should be clarified/extended.
Daniel
On Thu, 19 May 2011 22:45:09 +0200
Daniel Kiper <[email protected]> wrote:
> On Wed, May 18, 2011 at 08:36:02PM -0700, David Rientjes wrote:
> > On Tue, 17 May 2011, Daniel Kiper wrote:
> >
> > > This patch contains online_page_callback and apropriate functions for
> > > setting/restoring online page callbacks. 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_set_limits(),
> > > __online_page_increment_counters() and __online_page_free() function
> > > was added to ease generic hotplug operation.
> >
> > There are several issues with this.
> >
> > First, this is completely racy and only allows one global callback to be
> > in use at a time without looping, which is probably why you had to pass an
>
> One callback is allowed by design. Currently I do not see
> any real usage for more than one callback.
I'd suggest that you try using the notifier.h tools here and remove the
restriction. Sure, we may never use the capability but I expect the
code will look nice and simple and once it's done, it's done.
On Thu, May 19, 2011 at 04:01:43PM -0700, Andrew Morton wrote:
> On Thu, 19 May 2011 22:45:09 +0200
> Daniel Kiper <[email protected]> wrote:
>
> > On Wed, May 18, 2011 at 08:36:02PM -0700, David Rientjes wrote:
> > > On Tue, 17 May 2011, Daniel Kiper wrote:
> > >
> > > > This patch contains online_page_callback and apropriate functions for
> > > > setting/restoring online page callbacks. 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_set_limits(),
> > > > __online_page_increment_counters() and __online_page_free() function
> > > > was added to ease generic hotplug operation.
> > >
> > > There are several issues with this.
> > >
> > > First, this is completely racy and only allows one global callback to be
> > > in use at a time without looping, which is probably why you had to pass an
> >
> > One callback is allowed by design. Currently I do not see
> > any real usage for more than one callback.
>
> I'd suggest that you try using the notifier.h tools here and remove the
> restriction. Sure, we may never use the capability but I expect the
> code will look nice and simple and once it's done, it's done.
Hmmm... I am a bit confused. Here https://lkml.org/lkml/2011/3/28/510 you
was against (ab)using notifiers. Here https://lkml.org/lkml/2011/3/29/313
you proposed currently implemented solution. Maybe I missed something...
What should I do now ??? I agree that the code should look nice and simple
and once it's done, it's done.
Daniel
On Fri, 20 May 2011 01:25:20 +0200 Daniel Kiper <[email protected]> wrote:
> On Thu, May 19, 2011 at 04:01:43PM -0700, Andrew Morton wrote:
> > On Thu, 19 May 2011 22:45:09 +0200
> > Daniel Kiper <[email protected]> wrote:
> >
> > > On Wed, May 18, 2011 at 08:36:02PM -0700, David Rientjes wrote:
> > > > On Tue, 17 May 2011, Daniel Kiper wrote:
> > > >
> > > > > This patch contains online_page_callback and apropriate functions for
> > > > > setting/restoring online page callbacks. 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_set_limits(),
> > > > > __online_page_increment_counters() and __online_page_free() function
> > > > > was added to ease generic hotplug operation.
> > > >
> > > > There are several issues with this.
> > > >
> > > > First, this is completely racy and only allows one global callback to be
> > > > in use at a time without looping, which is probably why you had to pass an
> > >
> > > One callback is allowed by design. Currently I do not see
> > > any real usage for more than one callback.
> >
> > I'd suggest that you try using the notifier.h tools here and remove the
> > restriction. Sure, we may never use the capability but I expect the
> > code will look nice and simple and once it's done, it's done.
>
> Hmmm... I am a bit confused. Here https://lkml.org/lkml/2011/3/28/510 you
> was against (ab)using notifiers. Here https://lkml.org/lkml/2011/3/29/313
> you proposed currently implemented solution. Maybe I missed something...
> What should I do now ??? I agree that the code should look nice and simple
> and once it's done, it's done.
Oh, OK, the callback's role is to free a page, so there's no sens in
there ever being more than a single registered callback.