Hi!
This fixes 2 memory leaks in swsusp: during relocating pagedir, eaten
pages were not properly freed in error path and even regular freeing
path was freeing one page too little. Please apply,
Pavel
--- linux-cvs/kernel/power/swsusp.c 2004-05-22 19:39:01.000000000 +0200
+++ linux/kernel/power/swsusp.c 2004-06-06 00:30:09.000000000 +0200
@@ -455,7 +455,7 @@
@@ -503,6 +503,9 @@
if (!pbe)
continue;
pbe->orig_address = (long) page_address(page);
+ /* Copy page is dangerous: it likes to mess with
+ preempt count on specific cpus. Wrong preempt count is then copied,
+ oops. */
copy_page((void *)pbe->address, (void *)pbe->orig_address);
pbe++;
}
@@ -923,8 +952,9 @@
suspend_pagedir_t *new_pagedir, *old_pagedir = pagedir_nosave;
void **eaten_memory = NULL;
void **c = eaten_memory, *m, *f;
+ int ret = 0;
- printk("Relocating pagedir");
+ printk("Relocating pagedir ");
if(!does_collide_order(old_pagedir, (unsigned long)old_pagedir, pagedir_order)) {
printk("not necessary\n");
@@ -941,22 +971,23 @@
c = eaten_memory;
}
- if (!m)
- return -ENOMEM;
-
- pagedir_nosave = new_pagedir = m;
- copy_pagedir(new_pagedir, old_pagedir);
+ if (!m) {
+ printk("out of memory\n");
+ ret = -ENOMEM;
+ } else {
+ pagedir_nosave = new_pagedir = m;
+ copy_pagedir(new_pagedir, old_pagedir);
+ }
c = eaten_memory;
- while(c) {
+ while (c) {
printk(":");
- f = *c;
+ f = c;
c = *c;
- if (f)
- free_pages((unsigned long)f, pagedir_order);
+ free_pages((unsigned long)f, pagedir_order);
}
printk("|\n");
- return 0;
+ return ret;
}
/*
--
934a471f20d6580d5aad759bf0d97ddc
Pavel Machek <[email protected]> wrote:
>
> This fixes 2 memory leaks in swsusp: during relocating pagedir, eaten
> pages were not properly freed in error path and even regular freeing
> path was freeing one page too little. Please apply,
Thanks for the patch.
Here is the same fix for pmdisk.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
--
===== kernel/power/pmdisk.c 1.84 vs edited =====
--- 1.84/kernel/power/pmdisk.c 2004-04-13 03:55:18 +10:00
+++ edited/kernel/power/pmdisk.c 2004-06-10 20:47:15 +10:00
@@ -795,6 +795,7 @@
suspend_pagedir_t *new_pagedir, *old_pagedir = pm_pagedir_nosave;
void **eaten_memory = NULL;
void **c = eaten_memory, *m, *f;
+ int err;
pr_debug("pmdisk: Relocating pagedir\n");
@@ -803,32 +804,31 @@
return 0;
}
+ err = -ENOMEM;
while ((m = (void *) __get_free_pages(GFP_ATOMIC, pagedir_order))) {
memset(m, 0, PAGE_SIZE);
- if (!does_collide_order(old_pagedir, (unsigned long)m, pagedir_order))
+ if (!does_collide_order(old_pagedir, (unsigned long)m,
+ pagedir_order)) {
+ pm_pagedir_nosave = new_pagedir = m;
+ copy_pagedir(new_pagedir, old_pagedir);
+ err = 0;
break;
+ }
eaten_memory = m;
printk( "." );
*eaten_memory = c;
c = eaten_memory;
}
- if (!m)
- return -ENOMEM;
-
- pm_pagedir_nosave = new_pagedir = m;
- copy_pagedir(new_pagedir, old_pagedir);
-
c = eaten_memory;
while(c) {
printk(":");
- f = *c;
+ f = c;
c = *c;
- if (f)
- free_pages((unsigned long)f, pagedir_order);
+ free_pages((unsigned long)f, pagedir_order);
}
printk("|\n");
- return 0;
+ return err;
}
On Thu, Jun 10, 2004 at 08:50:12PM +1000, Herbert Xu wrote:
>
> @@ -803,32 +804,31 @@
> return 0;
> }
>
> + err = -ENOMEM;
> while ((m = (void *) __get_free_pages(GFP_ATOMIC, pagedir_order))) {
> memset(m, 0, PAGE_SIZE);
BTW, what does this memset do?
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Hi!
> > @@ -803,32 +804,31 @@
> > return 0;
> > }
> >
> > + err = -ENOMEM;
> > while ((m = (void *) __get_free_pages(GFP_ATOMIC, pagedir_order))) {
> > memset(m, 0, PAGE_SIZE);
>
> BTW, what does this memset do?
Someone (me?) was trying to be carefull, and was not carefull enough.
AFAICS it should be memset(..., PAGE_SIZE << pagedir_order)
--
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms
Hi!
> > @@ -803,32 +804,31 @@
> > return 0;
> > }
> >
> > + err = -ENOMEM;
> > while ((m = (void *) __get_free_pages(GFP_ATOMIC, pagedir_order))) {
> > memset(m, 0, PAGE_SIZE);
>
> BTW, what does this memset do?
Here's incremental fix, it compiles but not tested.
BTW I have problems getting mail to you:
This is the Postfix program at host atrey.karlin.mff.cuni.cz.
I'm sorry to have to inform you that the message returned
below could not be delivered to one or more destinations.
For further assistance, please send mail to <postmaster>
If you do so, please include this problem report. You can
delete your own text from the message returned below.
The Postfix program
<[email protected]>: host arnor.apana.org.au[203.14.152.115]said:
550 mail from 195.113.31.123 rejected: administrative prohibition
Pavel
--- tmp/linux/kernel/power/swsusp.c 2004-06-10 23:16:05.000000000 +0200
+++ linux/kernel/power/swsusp.c 2004-06-10 23:09:07.000000000 +0200
@@ -962,7 +962,7 @@
}
while ((m = (void *) __get_free_pages(GFP_ATOMIC, pagedir_order))) {
- memset(m, 0, PAGE_SIZE);
+ memset(m, 0, PAGE_SIZE << pagedir_order);
if (!does_collide_order(old_pagedir, (unsigned long)m, pagedir_order))
break;
eaten_memory = m;
--
People were complaining that M$ turns users into beta-testers...
On Thu, Jun 10, 2004 at 11:24:48PM +0200, Pavel Machek wrote:
> > > @@ -803,32 +804,31 @@
> > > return 0;
> > > }
> > >
> > > + err = -ENOMEM;
> > > while ((m = (void *) __get_free_pages(GFP_ATOMIC, pagedir_order))) {
> > > memset(m, 0, PAGE_SIZE);
> >
> > BTW, what does this memset do?
>
> Here's incremental fix, it compiles but not tested.
Thanks, but my question remains: why do we need the memset? The
area allocated is either thrown away because it collides or is
overwritten with the pagedir stuff straight away.
> BTW I have problems getting mail to you:
Sorry, that should be fixed now.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Hi!
> > > > @@ -803,32 +804,31 @@
> > > > return 0;
> > > > }
> > > >
> > > > + err = -ENOMEM;
> > > > while ((m = (void *) __get_free_pages(GFP_ATOMIC, pagedir_order))) {
> > > > memset(m, 0, PAGE_SIZE);
> > >
> > > BTW, what does this memset do?
> >
> > Here's incremental fix, it compiles but not tested.
>
> Thanks, but my question remains: why do we need the memset? The
> area allocated is either thrown away because it collides or is
> overwritten with the pagedir stuff straight away.
You are right, it should not be needed.
Pavel
--
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
On Fri, Jun 11, 2004 at 11:48:44AM +0200, Pavel Machek wrote:
>
> > Thanks, but my question remains: why do we need the memset? The
> > area allocated is either thrown away because it collides or is
> > overwritten with the pagedir stuff straight away.
>
> You are right, it should not be needed.
Great. Here's the patch that removes the memset calls from both
pmdisk and swsusp. It depends on the previous patches in this
thread.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Hi!
> > > Thanks, but my question remains: why do we need the memset? The
> > > area allocated is either thrown away because it collides or is
> > > overwritten with the pagedir stuff straight away.
> >
> > You are right, it should not be needed.
>
> Great. Here's the patch that removes the memset calls from both
> pmdisk and swsusp. It depends on the previous patches in this
> thread.
If you want more cleanups, copy_pagedir() should be probably replaced
by simple memset...
Pavel
--
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
On Fri, Jun 11, 2004 at 12:23:27PM +0200, Pavel Machek wrote:
>
> If you want more cleanups, copy_pagedir() should be probably replaced
> by simple memset...
Yes that's a great idea. Here is the patch to do that. Again it
relies on all the previous patches in this thread.
Cheers,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Hi.
Herbert Xu wrote:
[...]
> + pm_pagedir_nosave =
> + memcpy(m, old_pagedir,
> + PAGE_SIZE << pagedir_order);
[...]
> + pagedir_nosave =
> + memcpy(m, old_pagedir, PAGE_SIZE << pagedir_order);
> }
>
> c = eaten_memory;
We were avoiding the use of memcpy because it messes up the preempt count with 3DNow, and
potentially as other unseen side effects. The preempt could possibly simply be reset at resume time,
but the point remains.
Regards,
Nigel
--
Nigel & Michelle Cunningham
C/- Westminster Presbyterian Church Belconnen
61 Templeton Street, Cook, ACT 2614.
+61 (417) 100 574 (mobile)
Nigel Cunningham <[email protected]> wrote:
>
> We were avoiding the use of memcpy because it messes up the preempt count with 3DNow, and
> potentially as other unseen side effects. The preempt could possibly simply be reset at resume time,
> but the point remains.
eh? memcpy just copies memory. Maybe your meant copy_*_user()?
On Sat, Jun 12, 2004 at 01:17:30PM +1000, Nigel Cunningham wrote:
>
> We were avoiding the use of memcpy because it messes up the preempt count
> with 3DNow, and potentially as other unseen side effects. The preempt could
> possibly simply be reset at resume time, but the point remains.
In what way does it mess up the preempt count?
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
At this point it is okay to memcpy - it is copying pagedir, at that point we are outside any critical session. --p
------
Written-on-t68i. Sorry.
On Fri, Jun 11, 2004 at 09:00:59PM -0700, Andrew Morton wrote:
> Nigel Cunningham <[email protected]> wrote:
> >
> > We were avoiding the use of memcpy because it messes up the preempt count with 3DNow, and
> > potentially as other unseen side effects. The preempt could possibly simply be reset at resume time,
> > but the point remains.
>
> eh? memcpy just copies memory. Maybe your meant copy_*_user()?
See arch/i386/lib/memcpy.c The 3dnow routine does kernel_fpu_begin()/..end()
which futzes with preempt count. What I'm missing though is that the count
afterwards should be the same as it was before the memcpy. Why is this
a problem for the suspend folks?
Dave
Hi.
Andrew Morton wrote:
> Nigel Cunningham <[email protected]> wrote:
>
>> We were avoiding the use of memcpy because it messes up the preempt count with 3DNow, and
>> potentially as other unseen side effects. The preempt could possibly simply be reset at resume time,
>> but the point remains.
>
>
> eh? memcpy just copies memory. Maybe your meant copy_*_user()?
At some stage, you copy the page that contains the preempt count for the process that is doing the
suspending. If you use memcpy on a 3Dnow machine, the preempt count is incremented prior to doing
the copy of the page. Then, at resume time, it is one too high.
Regards,
Nigel
--
Nigel & Michelle Cunningham
C/- Westminster Presbyterian Church Belconnen
61 Templeton Street, Cook, ACT 2614.
+61 (417) 100 574 (mobile)
[email protected] wrote:
> At this point it is okay to memcpy - it is copying pagedir, at that point we are outside any critical session. --p
Ah. So you're not doing the atomic copy in this routine? Humble apologies.
Nigel
--
Nigel & Michelle Cunningham
C/- Westminster Presbyterian Church Belconnen
61 Templeton Street, Cook, ACT 2614.
+61 (417) 100 574 (mobile)
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On Saturday 12 June 2004 18:07, Nigel Cunningham wrote:
> Hi.
> At some stage, you copy the page that contains the preempt count for the
> process that is doing the suspending. If you use memcpy on a 3Dnow machine,
> the preempt count is incremented prior to doing the copy of the page. Then,
> at resume time, it is one too high.
Well, if we know that it is one too high, why not decrement right after the
resume?
Jeff.
- --
"Reality is merely an illusion, albeit a very persistent one."
- Albert Einstein
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (GNU/Linux)
iD8DBQFAy5KfwFP0+seVj/4RAlTXAJsFHALWNJ+mgVu7xEhfsk2Hqcq/wwCgmEmh
EpOv1mA9B35xcbzxT+lSHIo=
=Bn1K
-----END PGP SIGNATURE-----
Nigel Cunningham <[email protected]> wrote:
>
> At some stage, you copy the page that contains the preempt count for the process that is doing the
> suspending. If you use memcpy on a 3Dnow machine, the preempt count is incremented prior to doing
> the copy of the page. Then, at resume time, it is one too high.
Nope, this function only copies the swsusp page data structure so
this is irrelevant.
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Hi!
> > At some stage, you copy the page that contains the preempt count for the
> > process that is doing the suspending. If you use memcpy on a 3Dnow machine,
> > the preempt count is incremented prior to doing the copy of the page. Then,
> > at resume time, it is one too high.
>
> Well, if we know that it is one too high, why not decrement right after the
> resume?
You are suggesting very dirty hack. See archives why it is basically
undoable.
Pavel
--
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
Hi!
> > > We were avoiding the use of memcpy because it messes up the preempt count with 3DNow, and
> > > potentially as other unseen side effects. The preempt could possibly simply be reset at resume time,
> > > but the point remains.
> >
> > eh? memcpy just copies memory. Maybe your meant copy_*_user()?
>
> See arch/i386/lib/memcpy.c The 3dnow routine does kernel_fpu_begin()/..end()
> which futzes with preempt count. What I'm missing though is that the count
> afterwards should be the same as it was before the memcpy. Why is this
> a problem for the suspend folks?
It does not hurt here, but as someone else explained already it is a
problem when doing atomic copy of memory. (You increment, than copy
whole memory, you decrement; but you only decrement in original, not
in copy...)
Pavel
--
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!