[PATCH] SMP support for drain local pages.

[PATCH] SMP support for drain local pages.

[Nigel Cunningham]

	From: Nigel Cunningham <ncunningham@linuxmail.org>

Hi.

This patch adds SMP support for drain_local_pages, so that suspend
implementations can drain pcp structures on all CPUs and thus accurately
determine which pages are free.

Please apply.

diff -ruN 2.6.6-current-bk/mm/page_alloc.c smp-drain-local-pages/mm/page_alloc.c
--- 2.6.6-current-bk/mm/page_alloc.c    2004-05-26 19:47:15.000000000 +1000
+++ smp-drain-local-pages/mm/page_alloc.c       2004-05-26 19:56:19.000000000 +1000
@@ -459,6 +459,24 @@
         __drain_pages(smp_processor_id());
         local_irq_restore(flags);
  }
+
+#ifdef CONFIG_SMP
+static void __smp_drain_local_pages(void * data)
+{
+       drain_local_pages();
+}
+
+void smp_drain_local_pages(void)
+{
+       smp_call_function(__smp_drain_local_pages, NULL, 0, 1);
+       drain_local_pages();
+}
+#else
+void smp_drain_local_pages(void)
+{
+       drain_local_pages();
+}
+#endif
  #endif /* CONFIG_PM */

  static void zone_statistics(struct zonelist *zonelist, struct zone *z)

Re: [PATCH] SMP support for drain local pages.

[Dave Jones]

On Wed, May 26, 2004 at 08:39:51PM +1000, Nigel Cunningham wrote:

 > +#ifdef CONFIG_SMP
 > +static void __smp_drain_local_pages(void * data)
 > +{
 > +       drain_local_pages();
 > +}
 > +
 > +void smp_drain_local_pages(void)
 > +{
 > +       smp_call_function(__smp_drain_local_pages, NULL, 0, 1);
 > +       drain_local_pages();
 > +}
 > +#else
 > +void smp_drain_local_pages(void)
 > +{
 > +       drain_local_pages();
 > +}
 > +#endif
 >  #endif /* CONFIG_PM */

if you use on_each_cpu() you can do away with the ifdef.
It also takes care of preemption issues.

		Dave

Re: [PATCH] SMP support for drain local pages v2.

[Nigel Cunningham]

Thanks Dave for educating the ignorant :>.

Is this better?

--- 2.6.6-current-bk/mm/page_alloc.c    2004-05-26 19:47:15.000000000 +1000
+++ smp-drain-local-pages/mm/page_alloc.c       2004-05-27 08:50:36.000000000 +1000
@@ -459,6 +459,20 @@
         __drain_pages(smp_processor_id());
         local_irq_restore(flags);
  }
+
+/*
+ * Spill per-cpu pages on all CPUs back into the buddy allocator.
+ * The first function is just to avoid a compiler warning.
+ */
+static void __smp_drain_local_pages(void * data)
+{
+       drain_local_pages();
+}
+
+void smp_drain_local_pages(void)
+{
+       on_each_cpu(__smp_drain_local_pages, NULL, 0, 1);
+}
  #endif /* CONFIG_PM */

  static void zone_statistics(struct zonelist *zonelist, struct zone *z)

Re: [PATCH] SMP support for drain local pages v2.

[Andrew Morton]

Nigel Cunningham <ncunningham@linuxmail.org> wrote:
>
> +/*
> + * Spill per-cpu pages on all CPUs back into the buddy allocator.
> + * The first function is just to avoid a compiler warning.
> + */
> +static void __smp_drain_local_pages(void * data)
> +{
> +       drain_local_pages();
> +}
> +
> +void smp_drain_local_pages(void)
> +{
> +       on_each_cpu(__smp_drain_local_pages, NULL, 0, 1);
> +}

I think we only need a single entry point.  Make it a new "drain_percpu_pages()"
or such to break unconverted callers, switch callers of drain_local_pages()
over to the new function.   It needs no SMP ifdefs in it - on_each_cpu() will
do the right thing on UP.

But until something which needs this change is merged into the tree I'd say
that this patch should live with the patch which requires it.

Re: [PATCH] SMP support for drain local pages v2.

[Nigel Cunningham]

Hi.

Andrew Morton wrote:
> I think we only need a single entry point.  Make it a new "drain_percpu_pages()"
> or such to break unconverted callers, switch callers of drain_local_pages()
> over to the new function.   It needs no SMP ifdefs in it - on_each_cpu() will
> do the right thing on UP.
> 
> But until something which needs this change is merged into the tree I'd say
> that this patch should live with the patch which requires it.

CPU hotplugging uses drain_local_pages, and shouldn't drain the pcp lists for all cpus. That's why I 
left the original version alone.

I'm submitting it now in preparation for merging, but Pavel's work on SMP support for swsusp should 
be using this too. (It's not, but it should be).

Nigel
-- 
Nigel & Michelle Cunningham
C/- Westminster Presbyterian Church Belconnen
61 Templeton Street, Cook, ACT 2614.
+61 (2) 6251 7727(wk); +61 (2) 6254 0216 (home)

Evolution (n): A hypothetical process whereby infinitely improbable events occur
with alarming frequency, order arises from chaos, and no one is given credit.

Re: [PATCH] SMP support for drain local pages v2.

[Andrew Morton]

Nigel Cunningham <ncunningham@linuxmail.org> wrote:
>
> Hi.
> 
> Andrew Morton wrote:
> > I think we only need a single entry point.  Make it a new "drain_percpu_pages()"
> > or such to break unconverted callers, switch callers of drain_local_pages()
> > over to the new function.   It needs no SMP ifdefs in it - on_each_cpu() will
> > do the right thing on UP.
> > 
> > But until something which needs this change is merged into the tree I'd say
> > that this patch should live with the patch which requires it.
> 
> CPU hotplugging uses drain_local_pages,

Early versions did, but not now.

Re: [PATCH] SMP support for drain local pages v2.

[Nigel Cunningham]

Andrew Morton wrote:
> Nigel Cunningham <ncunningham@linuxmail.org> wrote:
>>CPU hotplugging uses drain_local_pages,
> Early versions did, but not now.

Oh. Okay. :>

Nigel
-- 
Nigel & Michelle Cunningham
C/- Westminster Presbyterian Church Belconnen
61 Templeton Street, Cook, ACT 2614.
+61 (2) 6251 7727(wk); +61 (2) 6254 0216 (home)

Evolution (n): A hypothetical process whereby infinitely improbable events occur
with alarming frequency, order arises from chaos, and no one is given credit.

Re: [PATCH] SMP support for drain local pages v2.

[Pavel Machek]

Hi!

> >But until something which needs this change is merged into the tree 
> >I'd say
> >that this patch should live with the patch which requires it.
> 
> CPU hotplugging uses drain_local_pages, and shouldn't drain the pcp 
> lists for all cpus. That's why I left the original version alone.
> 
> I'm submitting it now in preparation for merging, but Pavel's work on 
> SMP support for swsusp should be using this too. (It's not, but it 
> should be).

drain_local_pages was there so that accounting is not
screwed by local pages. That should be non-issue for stopped cpus.

OTOH you are right that my swsusp with smp does not work
on my smp machine, and I do not yet know why.
-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: [PATCH] SMP support for drain local pages v2.

[Nigel Cunningham]

Hi.

Andrew, Dave: do you want to be cc'd still?

Pavel Machek wrote:
> drain_local_pages was there so that accounting is not
> screwed by local pages. That should be non-issue for stopped cpus.

You're probably right that it's not absolutely necessary. That said, I have three reasons for 
wanting to do it: each cpu has say 200 pages in its pcp structs (this varies with zone size). If we 
don't drain them, we're saving 200 extra pages (because they're not marked as free). That's fine for 
a dual CPU system, but it's not very scalable. (I admit I wouldn't want to suspend to disk a 4GB 
machine now, but in the future?).

More than that, though, my real motivation is to be able to account for every page in the system and 
ensure that suspend is doing the right thing with all of them. If I drain the PCP page lists, I can 
be more certain that allocations do come from the right place.

Thirdly, after suspending hot pages aren't really hot anymore. We may as well start with clean lists.

> 
> OTOH you are right that my swsusp with smp does not work
> on my smp machine, and I do not yet know why.

I found that I needed to:
- save context for other CPUs.
- ensure they're completely idle during copyback even running the idle thread messed things up (use 
a smp function)
- ensure suspend is always running on CPU0 (at the start of suspending and resuming)

You might also consider how you're restoring highmem pages and how that interacts with SMP esp in 
flushing tlbs.

Nigel
-- 
Nigel & Michelle Cunningham
C/- Westminster Presbyterian Church Belconnen
61 Templeton Street, Cook, ACT 2614.
+61 (2) 6251 7727(wk); +61 (2) 6254 0216 (home)

Evolution (n): A hypothetical process whereby infinitely improbable events occur
with alarming frequency, order arises from chaos, and no one is given credit.

Re: [PATCH] SMP support for drain local pages.

[Pavel Machek]

Hi!

> This patch adds SMP support for drain_local_pages, so that suspend
> implementations can drain pcp structures on all CPUs and thus 
> accurately
> determine which pages are free.

Why do you need it, btw?

1st it might be easier to just on_each_cpu(drain_local_pages)
in suspend2

2nd all but one cpus are stopped, right? That mrans that their local pages can't
mess up suspnd's accounting => no need to drain them.
				Pavel

2.6.6-current-bk/mm/page_alloc.c 
> smp-drain-local-pages/mm/page_alloc.c
> --- 2.6.6-current-bk/mm/page_alloc.c    2004-05-26 19:47:15.000000000 
> +1000
> +++ smp-drain-local-pages/mm/page_alloc.c       2004-05-26 
> 19:56:19.000000000 +1000
> @@ -459,6 +459,24 @@
>         __drain_pages(smp_processor_id());
>         local_irq_restore(flags);
>  }
> +
> +#ifdef CONFIG_SMP
> +static void __smp_drain_local_pages(void * data)
> +{
> +       drain_local_pages();
> +}
> +
> +void smp_drain_local_pages(void)
> +{
> +       smp_call_function(__smp_drain_local_pages, NULL, 0, 1);
> +       drain_local_pages();
> +}
> +#else
> +void smp_drain_local_pages(void)
> +{
> +       drain_local_pages();
> +}
> +#endif
>  #endif /* CONFIG_PM */
> 
>  static void zone_statistics(struct zonelist *zonelist, struct zone 
>  *z)
> 
> 
> 
> -
> To unsubscribe from this list: send the line "unsubscribe 
> linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms