Re: SWSUSP Discontiguous pagedir patch

[Pavel Machek]

Hi!

> > Thus, I still think we can go with the patch I submitted before. I've
> > rediffed it against 2.5.63 (less the bits already applied).
> 
> I've spent the last week reading, reviewing, and rewriting major portions 
> of swsusp. I've actually been reasonably impressed, once I was able to get 
> the code into a much more readable state. 

:-).

> > diff -ruN linux-2.5.63/arch/i386/kernel/suspend.c linux-2.5.63-01/arch/i386/kernel/suspend.c
> > --- linux-2.5.63/arch/i386/kernel/suspend.c	2003-02-20 08:25:26.000000000 +1300
> > +++ linux-2.5.63-01/arch/i386/kernel/suspend.c	2003-02-20 08:27:36.000000000 +1300
> 
> Thank you for putting this back in C, it's much appreciated. 

Actually, it can not be put back in C. Manipulating stack pointer from
gcc inline assembly is just undefined. Its back in C so we can edit
it, but it needs to get back to assembly before merging with Linus.

> > +	for (loop=0; loop < nr_copy_pages; loop++) {
> > +		/* You may not call something (like copy_page) here: see above */
> > +		for (loop2=0; loop2 < PAGE_SIZE; loop2++) {
> > +			*(((char *)(PAGEDIR_ENTRY(pagedir_nosave,loop)->orig_address))+loop2) =
> > +				*(((char *)(PAGEDIR_ENTRY(pagedir_nosave,loop)->address))+loop2);
> > +			__flush_tlb();
> > +		}
> > +	}
> 
> This is better done as 
> 
> 	for (loop = 0; loop < nr_copy_pagse; loop++) {
> 		memcpy((char *)pagedir_nosave[loop].orig_address,
> 		       (char *)pagedir_nosave[loop].address,
> 		       PAGE_SIZE);
> 		__flush_tlb();
> 	}

Hehe, try it.

You may not do function call at this point, because you are
overwriting your stack. See mails with Andi Kleen. This *needs* to be
in assembly. 

> Is __flush_tlb() really necessary? 

Its there to prevent Heisenbugs.
								Pavel
-- 
Horseback riding is like software...
...vgf orggre jura vgf serr.