Re: [PATCH 4/11] ksm: reorganize ksm_check_stable_tree
On Thu, Feb 07, 2013 at 04:07:17PM -0800, Hugh Dickins wrote:
> On Tue, 5 Feb 2013, Mel Gorman wrote:
> > On Fri, Jan 25, 2013 at 05:59:35PM -0800, Hugh Dickins wrote:
> > > Memory hotremove's ksm_check_stable_tree() is pitifully inefficient
> > > (restarting whenever it finds a stale node to remove), but rearrange
> > > so that at least it does not needlessly restart from nid 0 each time.
> > > And add a couple of comments: here is why we keep pfn instead of page.
> > >
> > > Signed-off-by: Hugh Dickins
> > > ---
> > > mm/ksm.c | 38 ++
> > > 1 file changed, 22 insertions(+), 16 deletions(-)
> > >
> > > --- mmotm.orig/mm/ksm.c 2013-01-25 14:36:52.152205940 -0800
> > > +++ mmotm/mm/ksm.c2013-01-25 14:36:53.244205966 -0800
> > > @@ -1830,31 +1830,36 @@ void ksm_migrate_page(struct page *newpa
> > > #endif /* CONFIG_MIGRATION */
> > >
> > > #ifdef CONFIG_MEMORY_HOTREMOVE
> > > -static struct stable_node *ksm_check_stable_tree(unsigned long start_pfn,
> > > - unsigned long end_pfn)
> > > +static void ksm_check_stable_tree(unsigned long start_pfn,
> > > + unsigned long end_pfn)
> > > {
> > > + struct stable_node *stable_node;
> > > struct rb_node *node;
> > > int nid;
> > >
> > > - for (nid = 0; nid < nr_node_ids; nid++)
> > > - for (node = rb_first(&root_stable_tree[nid]); node;
> > > - node = rb_next(node)) {
> > > - struct stable_node *stable_node;
> > > -
> > > + for (nid = 0; nid < nr_node_ids; nid++) {
> > > + node = rb_first(&root_stable_tree[nid]);
> > > + while (node) {
> >
> > This is not your fault, the old code is wrong too. It is assuming that all
> > nodes are populated in numeric orders with no holes. It won't work if just
> > two nodes 0 and 4 are online. It should be using for_each_online_node().
>
> If the old code is wrong, it probably would be my fault! But I believe
> this is okay: these rb_roots we're looking at, they are in memory which
> is not being offlined, and the trees for offline nodes will simply be
> empty, won't they? Something's badly wrong if otherwise.
>
I would expect them to be empty but that was not the problem I had in
mind. Unfortunately I mixed up nr_online_ids and nr_node_ids and read
the loop incorrectly. What you have is fine.
--
Mel Gorman
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/11] ksm: reorganize ksm_check_stable_tree
On Tue, 5 Feb 2013, Mel Gorman wrote:
> On Fri, Jan 25, 2013 at 05:59:35PM -0800, Hugh Dickins wrote:
> > Memory hotremove's ksm_check_stable_tree() is pitifully inefficient
> > (restarting whenever it finds a stale node to remove), but rearrange
> > so that at least it does not needlessly restart from nid 0 each time.
> > And add a couple of comments: here is why we keep pfn instead of page.
> >
> > Signed-off-by: Hugh Dickins
> > ---
> > mm/ksm.c | 38 ++
> > 1 file changed, 22 insertions(+), 16 deletions(-)
> >
> > --- mmotm.orig/mm/ksm.c 2013-01-25 14:36:52.152205940 -0800
> > +++ mmotm/mm/ksm.c 2013-01-25 14:36:53.244205966 -0800
> > @@ -1830,31 +1830,36 @@ void ksm_migrate_page(struct page *newpa
> > #endif /* CONFIG_MIGRATION */
> >
> > #ifdef CONFIG_MEMORY_HOTREMOVE
> > -static struct stable_node *ksm_check_stable_tree(unsigned long start_pfn,
> > -unsigned long end_pfn)
> > +static void ksm_check_stable_tree(unsigned long start_pfn,
> > + unsigned long end_pfn)
> > {
> > + struct stable_node *stable_node;
> > struct rb_node *node;
> > int nid;
> >
> > - for (nid = 0; nid < nr_node_ids; nid++)
> > - for (node = rb_first(&root_stable_tree[nid]); node;
> > - node = rb_next(node)) {
> > - struct stable_node *stable_node;
> > -
> > + for (nid = 0; nid < nr_node_ids; nid++) {
> > + node = rb_first(&root_stable_tree[nid]);
> > + while (node) {
>
> This is not your fault, the old code is wrong too. It is assuming that all
> nodes are populated in numeric orders with no holes. It won't work if just
> two nodes 0 and 4 are online. It should be using for_each_online_node().
If the old code is wrong, it probably would be my fault! But I believe
this is okay: these rb_roots we're looking at, they are in memory which
is not being offlined, and the trees for offline nodes will simply be
empty, won't they? Something's badly wrong if otherwise.
I certainly prefer to avoid for_each_online_node() etc: maybe I'm
confusing with for_each_online_something_else(), but experience tells
that you can get into nasty hotplug mutex ordering issues with those
things - not worth the pain if you can easily and safely avoid them.
Hugh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/11] ksm: reorganize ksm_check_stable_tree
On Fri, Jan 25, 2013 at 05:59:35PM -0800, Hugh Dickins wrote:
> Memory hotremove's ksm_check_stable_tree() is pitifully inefficient
> (restarting whenever it finds a stale node to remove), but rearrange
> so that at least it does not needlessly restart from nid 0 each time.
> And add a couple of comments: here is why we keep pfn instead of page.
>
> Signed-off-by: Hugh Dickins
> ---
> mm/ksm.c | 38 ++
> 1 file changed, 22 insertions(+), 16 deletions(-)
>
> --- mmotm.orig/mm/ksm.c 2013-01-25 14:36:52.152205940 -0800
> +++ mmotm/mm/ksm.c2013-01-25 14:36:53.244205966 -0800
> @@ -1830,31 +1830,36 @@ void ksm_migrate_page(struct page *newpa
> #endif /* CONFIG_MIGRATION */
>
> #ifdef CONFIG_MEMORY_HOTREMOVE
> -static struct stable_node *ksm_check_stable_tree(unsigned long start_pfn,
> - unsigned long end_pfn)
> +static void ksm_check_stable_tree(unsigned long start_pfn,
> + unsigned long end_pfn)
> {
> + struct stable_node *stable_node;
> struct rb_node *node;
> int nid;
>
> - for (nid = 0; nid < nr_node_ids; nid++)
> - for (node = rb_first(&root_stable_tree[nid]); node;
> - node = rb_next(node)) {
> - struct stable_node *stable_node;
> -
> + for (nid = 0; nid < nr_node_ids; nid++) {
> + node = rb_first(&root_stable_tree[nid]);
> + while (node) {
This is not your fault, the old code is wrong too. It is assuming that all
nodes are populated in numeric orders with no holes. It won't work if just
two nodes 0 and 4 are online. It should be using for_each_online_node().
--
Mel Gorman
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 4/11] ksm: reorganize ksm_check_stable_tree
Memory hotremove's ksm_check_stable_tree() is pitifully inefficient
(restarting whenever it finds a stale node to remove), but rearrange
so that at least it does not needlessly restart from nid 0 each time.
And add a couple of comments: here is why we keep pfn instead of page.
Signed-off-by: Hugh Dickins
---
mm/ksm.c | 38 ++
1 file changed, 22 insertions(+), 16 deletions(-)
--- mmotm.orig/mm/ksm.c 2013-01-25 14:36:52.152205940 -0800
+++ mmotm/mm/ksm.c 2013-01-25 14:36:53.244205966 -0800
@@ -1830,31 +1830,36 @@ void ksm_migrate_page(struct page *newpa
#endif /* CONFIG_MIGRATION */
#ifdef CONFIG_MEMORY_HOTREMOVE
-static struct stable_node *ksm_check_stable_tree(unsigned long start_pfn,
-unsigned long end_pfn)
+static void ksm_check_stable_tree(unsigned long start_pfn,
+ unsigned long end_pfn)
{
+ struct stable_node *stable_node;
struct rb_node *node;
int nid;
- for (nid = 0; nid < nr_node_ids; nid++)
- for (node = rb_first(&root_stable_tree[nid]); node;
- node = rb_next(node)) {
- struct stable_node *stable_node;
-
+ for (nid = 0; nid < nr_node_ids; nid++) {
+ node = rb_first(&root_stable_tree[nid]);
+ while (node) {
stable_node = rb_entry(node, struct stable_node, node);
if (stable_node->kpfn >= start_pfn &&
- stable_node->kpfn < end_pfn)
- return stable_node;
+ stable_node->kpfn < end_pfn) {
+ /*
+* Don't get_ksm_page, page has already gone:
+* which is why we keep kpfn instead of page*
+*/
+ remove_node_from_stable_tree(stable_node);
+ node = rb_first(&root_stable_tree[nid]);
+ } else
+ node = rb_next(node);
+ cond_resched();
}
-
- return NULL;
+ }
}
static int ksm_memory_callback(struct notifier_block *self,
unsigned long action, void *arg)
{
struct memory_notify *mn = arg;
- struct stable_node *stable_node;
switch (action) {
case MEM_GOING_OFFLINE:
@@ -1874,11 +1879,12 @@ static int ksm_memory_callback(struct no
/*
* Most of the work is done by page migration; but there might
* be a few stable_nodes left over, still pointing to struct
-* pages which have been offlined: prune those from the tree.
+* pages which have been offlined: prune those from the tree,
+* otherwise get_ksm_page() might later try to access a
+* non-existent struct page.
*/
- while ((stable_node = ksm_check_stable_tree(mn->start_pfn,
- mn->start_pfn + mn->nr_pages)) != NULL)
- remove_node_from_stable_tree(stable_node);
+ ksm_check_stable_tree(mn->start_pfn,
+ mn->start_pfn + mn->nr_pages);
/* fallthrough */
case MEM_CANCEL_OFFLINE:
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
