[PATCH][RFC] swsusp: speed up image restoring on x86-64

[PATCH][RFC] swsusp: speed up image restoring on x86-64

[Rafael J. Wysocki]

Hi,

The following patch speeds up the restoring of swsusp images on x86-64
and makes the assembly code more readable (tested and works on AMD64).  It's
against 2.6.11-rc1-mm1, but applies to 2.6.11-rc1-mm2.  Please consifer for applying.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

--- linux-2.6.11-rc1-mm1/arch/x86_64/kernel/suspend_asm.S	2004-12-24 22:35:28.000000000 +0100
+++ linux-2.6.11-rc1-mm1-rjw/arch/x86_64/kernel/suspend_asm.S	2005-01-20 17:28:30.000000000 +0100
@@ -49,43 +49,28 @@
 	movq	%rcx, %cr3;
 	movq	%rax, %cr4;  # turn PGE back on
 
+	movq	pagedir_nosave(%rip), %rdx
+	/* compute the limit */
 	movl	nr_copy_pages(%rip), %eax
-	xorl	%ecx, %ecx
-	movq	$0, %r10
 	testl	%eax, %eax
 	jz	done
-.L105:
-	xorl	%esi, %esi
-	movq	$0, %r11
-	jmp	.L104
-	.p2align 4,,7
-copy_one_page:
-	movq	%r10, %rcx
-.L104:
-	movq	pagedir_nosave(%rip), %rdx
-	movq	%rcx, %rax
-	salq	$5, %rax
-	movq	8(%rdx,%rax), %rcx
-	movq	(%rdx,%rax), %rax
-	movzbl	(%rsi,%rax), %eax
-	movb	%al, (%rsi,%rcx)
+	shlq	$5, %rax; # multiply by sizeof(struct pbe)
+	addq	%rdx, %rax
+loop:
+	/* get addresses from the pbe and copy the page */
+	movq	(%rdx), %rsi
+	movq	8(%rdx), %rdi
+	movq	$512, %rcx
+	rep
+	movsq
 
-	movq	%cr3, %rax;  # flush TLB
-	movq	%rax, %cr3;
+	movq	%cr3, %rcx;  # flush TLB
+	movq	%rcx, %cr3;
 
-	movq	%r11, %rax
-	incq	%rax
-	cmpq	$4095, %rax
-	movq	%rax, %rsi
-	movq	%rax, %r11
-	jbe	copy_one_page
-	movq	%r10, %rax
-	incq	%rax
-	movq	%rax, %rcx
-	movq	%rax, %r10
-	mov	nr_copy_pages(%rip), %eax
-	cmpq	%rax, %rcx
-	jb	.L105
+	/* progress to the next pbe */
+	addq	$32, %rdx; # add sizeof(struct pbe)
+	cmpq	%rax, %rdx
+	jb	loop
 done:
 	movl	$24, %eax
 	movl	%eax, %ds



Greets,
RJW


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[Pavel Machek]

Hi!

> The following patch speeds up the restoring of swsusp images on x86-64
> and makes the assembly code more readable (tested and works on AMD64).  It's
> against 2.6.11-rc1-mm1, but applies to 2.6.11-rc1-mm2.  Please consifer for applying.

Can you really measure the speedup? If you want cheap way to speed it up,
kill cr3 manipulation.

Anyway, this is likely to clash with hugang's work; I'd prefer this not to be applied.

				Pavel

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

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[Rafael J. Wysocki]

On Thursday, 20 of January 2005 21:59, Pavel Machek wrote:
> Hi!
> 
> > The following patch speeds up the restoring of swsusp images on x86-64
> > and makes the assembly code more readable (tested and works on AMD64).  It's
> > against 2.6.11-rc1-mm1, but applies to 2.6.11-rc1-mm2.  Please consifer for applying.
> 
> Can you really measure the speedup?

In terms of time?  Probably I can, but I prefer to measure it in terms of the numbers of
operations to be performed.

With this patch, at least 8 times less memory accesses are required to restore an image
than without it, and in the original code cr3 is reloaded after copying each _byte_,
let alone the SIB arithmetics.  I'd expect it to be 10 times faster or so.

The readability of code is also important, IMHO.

> If you want cheap way to speed it up, kill cr3 manipulation.

Sure, but I think it's there for a reason.

> Anyway, this is likely to clash with hugang's work; I'd prefer this not to be applied.

I am aware of that, but you are not going to merge the hugang's patches soon, are you?
If necessary, I can change the patch to work with his code (hugang, what do you think?).

Greets,
RJW


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[Pavel Machek]

Hi!

> > > The following patch speeds up the restoring of swsusp images on x86-64
> > > and makes the assembly code more readable (tested and works on AMD64).  It's
> > > against 2.6.11-rc1-mm1, but applies to 2.6.11-rc1-mm2.  Please consifer for applying.
> > 
> > Can you really measure the speedup?
> 
> In terms of time?  Probably I can, but I prefer to measure it in terms of the numbers of
> operations to be performed.
> 
> With this patch, at least 8 times less memory accesses are required to restore an image
> than without it, and in the original code cr3 is reloaded after copying each _byte_,
> let alone the SIB arithmetics.  I'd expect it to be 10 times faster
> or so.

Well, 8 times less cr3 reloads may be significant... for the copy
loop. Speeding up copy loop that takes  ... 100msec?... of whole
resume (30 seconds) does not seem too important to me.

> The readability of code is also important, IMHO.

It did not seem too much better to me.

> > If you want cheap way to speed it up, kill cr3 manipulation.
> 
> Sure, but I think it's there for a reason.

Reason is "to crash it early if we have wrong pagetables".

> > Anyway, this is likely to clash with hugang's work; I'd prefer this not to be applied.
> 
> I am aware of that, but you are not going to merge the hugang's patches soon, are you?
> If necessary, I can change the patch to work with his code (hugang, what do you think?).

I think it is just not worth the effort.
										Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[Rafael J. Wysocki]

Hi,

On Thursday, 20 of January 2005 23:06, Pavel Machek wrote:
> Hi!
> 
> > > > The following patch speeds up the restoring of swsusp images on x86-64
> > > > and makes the assembly code more readable (tested and works on AMD64).  It's
> > > > against 2.6.11-rc1-mm1, but applies to 2.6.11-rc1-mm2.  Please consifer for applying.
> > > 
> > > Can you really measure the speedup?
> > 
> > In terms of time?  Probably I can, but I prefer to measure it in terms of the numbers of
> > operations to be performed.
> > 
> > With this patch, at least 8 times less memory accesses are required to restore an image
> > than without it, and in the original code cr3 is reloaded after copying each _byte_,
> > let alone the SIB arithmetics.  I'd expect it to be 10 times faster
> > or so.
> 
> Well, 8 times less cr3 reloads may be significant... for the copy
> loop. Speeding up copy loop that takes  ... 100msec?... of whole
> resume (30 seconds) does not seem too important to me.
> 
> > The readability of code is also important, IMHO.
> 
> It did not seem too much better to me.

Well, the beauty is in the eye of the beholder. :-)

Still, it shrinks the code (22 lines vs 37 lines), it uses less GPRs (5 vs 7), it uses less
SIB arithmetics (0 vs 4 times), it uses a well known scheme for copying data pages.
As far as the result is concerned, it is equivalent to the existing code, but it's simpler
(and faster).  IMO, simpler code is always easier to understand.


> > > If you want cheap way to speed it up, kill cr3 manipulation.
> > 
> > Sure, but I think it's there for a reason.
> 
> Reason is "to crash it early if we have wrong pagetables".
> 
> > > Anyway, this is likely to clash with hugang's work; I'd prefer this not to be applied.
> > 
> > I am aware of that, but you are not going to merge the hugang's patches soon, are you?
> > If necessary, I can change the patch to work with his code (hugang, what do you think?).
> 
> I think it is just not worth the effort.

Why?  It won't take much time.  I've spent more time for writing the messages
in this thread ... ;-)

Greets,
RJW


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[Pavel Machek]

Hi!

> > > The readability of code is also important, IMHO.
> > 
> > It did not seem too much better to me.
> 
> Well, the beauty is in the eye of the beholder. :-)
> 
> Still, it shrinks the code (22 lines vs 37 lines), it uses less GPRs (5 vs 7), it uses less
> SIB arithmetics (0 vs 4 times), it uses a well known scheme for copying data pages.
> As far as the result is concerned, it is equivalent to the existing code, but it's simpler
> (and faster).  IMO, simpler code is always easier to understand.
> 
> 
> > > > If you want cheap way to speed it up, kill cr3 manipulation.
> > > 
> > > Sure, but I think it's there for a reason.
> > 
> > Reason is "to crash it early if we have wrong pagetables".
> > 
> > > > Anyway, this is likely to clash with hugang's work; I'd prefer this not to be applied.
> > > 
> > > I am aware of that, but you are not going to merge the hugang's patches soon, are you?
> > > If necessary, I can change the patch to work with his code (hugang, what do you think?).
> > 
> > I think it is just not worth the effort.
> 
> Why?  It won't take much time.  I've spent more time for writing the messages
> in this thread ... ;-)

Well, I know that current code works. It was produced by C compiler,
btw. Now, new code works for you, but it was not in kernel for 4
releases, and... this code is pretty subtle. And it is hand-made, not
C produced.

So... your code may be better but I do not think it is so much better
that I'd like to risk it.

								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[Rafael J. Wysocki]

Hi,

On Friday, 21 of January 2005 00:06, Pavel Machek wrote:
> Hi!
> 
> > > > The readability of code is also important, IMHO.
> > > 
> > > It did not seem too much better to me.
> > 
> > Well, the beauty is in the eye of the beholder. :-)
> > 
> > Still, it shrinks the code (22 lines vs 37 lines), it uses less GPRs (5 vs 7), it uses less
> > SIB arithmetics (0 vs 4 times), it uses a well known scheme for copying data pages.
> > As far as the result is concerned, it is equivalent to the existing code, but it's simpler
> > (and faster).  IMO, simpler code is always easier to understand.
> > 
> > 
> > > > > If you want cheap way to speed it up, kill cr3 manipulation.
> > > > 
> > > > Sure, but I think it's there for a reason.
> > > 
> > > Reason is "to crash it early if we have wrong pagetables".
> > > 
> > > > > Anyway, this is likely to clash with hugang's work; I'd prefer this not to be applied.
> > > > 
> > > > I am aware of that, but you are not going to merge the hugang's patches soon, are you?
> > > > If necessary, I can change the patch to work with his code (hugang, what do you think?).
> > > 
> > > I think it is just not worth the effort.
> > 
> > Why?  It won't take much time.  I've spent more time for writing the messages
> > in this thread ... ;-)
> 
> Well, I know that current code works. It was produced by C compiler,
> btw. Now, new code works for you, but it was not in kernel for 4
> releases, and... this code is pretty subtle.

Now, I'm confused. :-)  It's roughly this:

struct pbe *pbe = pagedir_nosave, *end;
unsigned n = nr_copy_pages;
if (n) {
	end = pbe + n;
	do {
		memcpy((void *)pbe->orig_address, (void *)pbe->address, PAGE_SIZE);
		pbe++;
	} while (pbe < end);
}

where memcpy() is of course a hand-written inline that includes the cr3 manipulation,
and pbe, end, n are registers.

> And it is hand-made, not C produced.

Yes, it is.

> So... your code may be better but I do not think it is so much better
> that I'd like to risk it.

Now, that's clear. :-)

Anyway, if anyone could test it or look at it and say a word, please do so.

Greets,
RJW


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[Pavel Machek]

Hi!

> > Well, I know that current code works. It was produced by C compiler,
> > btw. Now, new code works for you, but it was not in kernel for 4
> > releases, and... this code is pretty subtle.
> 
> Now, I'm confused. :-)  It's roughly this:
> 
> struct pbe *pbe = pagedir_nosave, *end;
> unsigned n = nr_copy_pages;
> if (n) {
> 	end = pbe + n;
> 	do {
> 		memcpy((void *)pbe->orig_address, (void *)pbe->address, PAGE_SIZE);
> 		pbe++;
> 	} while (pbe < end);
> }
> 
> where memcpy() is of course a hand-written inline that includes the cr3 manipulation,
> and pbe, end, n are registers.

For example it may not use any variable in memory, and may not use
stack, as memory changes under its hands. Plus assembly is always
subtle ;-).

								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[Rafael J. Wysocki]

On Friday, 21 of January 2005 11:06, Pavel Machek wrote:
> Hi!
> 
> > > Well, I know that current code works. It was produced by C compiler,
> > > btw. Now, new code works for you, but it was not in kernel for 4
> > > releases, and... this code is pretty subtle.
> > 
> > Now, I'm confused. :-)  It's roughly this:
> > 
> > struct pbe *pbe = pagedir_nosave, *end;
> > unsigned n = nr_copy_pages;
> > if (n) {
> > 	end = pbe + n;
> > 	do {
> > 		memcpy((void *)pbe->orig_address, (void *)pbe->address, PAGE_SIZE);
> > 		pbe++;
> > 	} while (pbe < end);
> > }
> > 
> > where memcpy() is of course a hand-written inline that includes the cr3 manipulation,
> > and pbe, end, n are registers.
> 
> For example it may not use any variable in memory, and may not use
> stack, as memory changes under its hands.

Which is not a big problem on x86-64. :-)

> Plus assembly is always subtle ;-).

And that's what makes it interesting.

Greets,
RJW


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[hugang]

On Thu, Jan 20, 2005 at 10:46:37PM +0100, Rafael J. Wysocki wrote:
> On Thursday, 20 of January 2005 21:59, Pavel Machek wrote:
> 
> Sure, but I think it's there for a reason.
> 
> > Anyway, this is likely to clash with hugang's work; I'd prefer this not to be applied.
> 
> I am aware of that, but you are not going to merge the hugang's patches soon, are you?
> If necessary, I can change the patch to work with his code (hugang, what do you think?).
> 
I like this patch, And I change my code with this, Please have a look,
It pass in qemu X86_64. :)

Full patch still can get from
 http://soulinfo.com/~hugang/swsusp/2005-1-21/

here is only x86_64 part.

--- 2.6.11-rc1-mm1/arch/x86_64/kernel/suspend_asm.S	2004-12-30 14:56:35.000000000 +0800
+++ 2.6.11-rc1-mm1-swsusp-x86_64/arch/x86_64/kernel/suspend_asm.S	2005-01-21 10:13:15.000000000 +0800
@@ -35,6 +35,7 @@ ENTRY(swsusp_arch_suspend)
 	call swsusp_save
 	ret
 
+	.section    .data.nosave
 ENTRY(swsusp_arch_resume)
 	/* set up cr3 */	
 	leaq	init_level4_pgt(%rip),%rax
@@ -49,43 +50,32 @@ ENTRY(swsusp_arch_resume)
 	movq	%rcx, %cr3;
 	movq	%rax, %cr4;  # turn PGE back on
 
-	movl	nr_copy_pages(%rip), %eax
-	xorl	%ecx, %ecx
-	movq	$0, %r10
-	testl	%eax, %eax
-	jz	done
-.L105:
-	xorl	%esi, %esi
-	movq	$0, %r11
-	jmp	.L104
-	.p2align 4,,7
-copy_one_page:
-	movq	%r10, %rcx
-.L104:
-	movq	pagedir_nosave(%rip), %rdx
-	movq	%rcx, %rax
-	salq	$5, %rax
-	movq	8(%rdx,%rax), %rcx
-	movq	(%rdx,%rax), %rax
-	movzbl	(%rsi,%rax), %eax
-	movb	%al, (%rsi,%rcx)
-
-	movq	%cr3, %rax;  # flush TLB
-	movq	%rax, %cr3;
-
-	movq	%r11, %rax
-	incq	%rax
-	cmpq	$4095, %rax
-	movq	%rax, %rsi
-	movq	%rax, %r11
-	jbe	copy_one_page
-	movq	%r10, %rax
-	incq	%rax
-	movq	%rax, %rcx
-	movq	%rax, %r10
-	mov	nr_copy_pages(%rip), %eax
-	cmpq	%rax, %rcx
-	jb	.L105
+	movq	pagedir_nosave(%rip), %rax
+	testq	%rax, %rax
+	je		done
+
+copyback_page:
+	movq	24(%rax), %r9
+	xorl	%r8d, %r8d
+
+copy_one_pgdir:
+	movq    8(%rax), %rdi
+	testq   %rdi, %rdi
+	je  	done
+	movq    (%rax), %rsi
+	movq    $512, %rcx
+	rep
+	movsq
+
+	incq    %r8
+	addq    $32, %rax
+	cmpq    $127, %r8
+	jbe 	copy_one_pgdir; # copy one pgdir
+
+	testq   %r9, %r9
+	movq    %r9, %rax
+	jne 	copyback_page
+
 done:
 	movl	$24, %eax
 	movl	%eax, %ds

-- 
Hu Gang       .-.
              /v\
             // \\ 
Linux User  /(   )\  [204016]
GPG Key ID   ^^-^^   http://soulinfo.com/~hugang/hugang.asc

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[Pavel Machek]

Hi!

> > Sure, but I think it's there for a reason.
> > 
> > > Anyway, this is likely to clash with hugang's work; I'd prefer this not to be applied.
> > 
> > I am aware of that, but you are not going to merge the hugang's patches soon, are you?
> > If necessary, I can change the patch to work with his code (hugang, what do you think?).
> > 
> I like this patch, And I change my code with this, Please have a look,
> It pass in qemu X86_64. :)
> 
> Full patch still can get from
>  http://soulinfo.com/~hugang/swsusp/2005-1-21/
> 
> here is only x86_64 part.

Okay, why not, if you are changing it anyway...


> --- 2.6.11-rc1-mm1/arch/x86_64/kernel/suspend_asm.S	2004-12-30 14:56:35.000000000 +0800
> +++ 2.6.11-rc1-mm1-swsusp-x86_64/arch/x86_64/kernel/suspend_asm.S	2005-01-21 10:13:15.000000000 +0800
> @@ -35,6 +35,7 @@ ENTRY(swsusp_arch_suspend)
>  	call swsusp_save
>  	ret
>  
> +	.section    .data.nosave
>  ENTRY(swsusp_arch_resume)
>  	/* set up cr3 */	
>  	leaq	init_level4_pgt(%rip),%rax

But why does it go into data section?

> @@ -49,43 +50,32 @@ ENTRY(swsusp_arch_resume)
>  	movq	%rcx, %cr3;
>  	movq	%rax, %cr4;  # turn PGE back on
>  
> -	movl	nr_copy_pages(%rip), %eax
> -	xorl	%ecx, %ecx
> -	movq	$0, %r10
> -	testl	%eax, %eax
> -	jz	done
> -.L105:
> -	xorl	%esi, %esi
> -	movq	$0, %r11
> -	jmp	.L104
> -	.p2align 4,,7
> -copy_one_page:
> -	movq	%r10, %rcx
> -.L104:
> -	movq	pagedir_nosave(%rip), %rdx
> -	movq	%rcx, %rax
> -	salq	$5, %rax
> -	movq	8(%rdx,%rax), %rcx
> -	movq	(%rdx,%rax), %rax
> -	movzbl	(%rsi,%rax), %eax
> -	movb	%al, (%rsi,%rcx)
> -
> -	movq	%cr3, %rax;  # flush TLB
> -	movq	%rax, %cr3;
> -
> -	movq	%r11, %rax
> -	incq	%rax
> -	cmpq	$4095, %rax
> -	movq	%rax, %rsi
> -	movq	%rax, %r11
> -	jbe	copy_one_page
> -	movq	%r10, %rax
> -	incq	%rax
> -	movq	%rax, %rcx
> -	movq	%rax, %r10
> -	mov	nr_copy_pages(%rip), %eax
> -	cmpq	%rax, %rcx
> -	jb	.L105
> +	movq	pagedir_nosave(%rip), %rax
> +	testq	%rax, %rax
> +	je		done
> +
> +copyback_page:
> +	movq	24(%rax), %r9
> +	xorl	%r8d, %r8d
> +

Are you sure %r8 and %r9 are caller-saved? I'd use low registers if I
were you, they look nincer and generate shorter opcodes ;-).

								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[Andi Kleen]

> > +copyback_page:
> > +	movq	24(%rax), %r9
> > +	xorl	%r8d, %r8d
> > +
> 
> Are you sure %r8 and %r9 are caller-saved? I'd use low registers if I
> were you, they look nincer and generate shorter opcodes ;-).

They are.

-Andi

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[Pavel Machek]

Hi!

> Full patch still can get from
>  http://soulinfo.com/~hugang/swsusp/2005-1-21/

>From a short look:

core.eatmem.diff of course helps, but is wrong. You should talk to
akpm to find out why shrink_all_memory is not doing its job.

i386: +       repz movsl %ds:(%esi),%es:(%edi)
I do not think movsl has any parameters. What is repz? Repeat as long
as it is non-zero? I think this should be "rep movsl".


core:
@@ -576,92 +989,31 @@ static void copy_data_pages(void)
                for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn) {
                        if (saveable(zone, &zone_pfn)) {
                                struct page * page;
+                               pbe = find_pbe_by_index(pagedir_nosave, nr_copy_pages-to_copy);
+                               BUG_ON(pbe == NULL);
                                page = pfn_to_page(zone_pfn + zone->zone_start_pfn);

Don't you introduce O(n^2) behaviour here? Should not it be something
like pbe_next? And it is the only user of find_pbe_by_index().

I think that read_one_pbe() is too short to be uninlined... Same for
read_one_pagedir and write_one_pbe().

alloc_one_pagedir: why not just alloc page as zeroed?

Okay, it is still too big to merge directly. Would it be possible to
get mod_printk_progress(), introduce *_for_each (but leave there old
implementations), introduce pagedir_free() (but leave old
implementation). Better collision code should already be there, that
should make patch smaller, too. Try not to move code around.

That may be mergeable before 2.6.11...
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[Rafael J. Wysocki]

Hi,

On Friday, 21 of January 2005 11:30, Pavel Machek wrote:
> Hi!
> 
> > Full patch still can get from
> >  http://soulinfo.com/~hugang/swsusp/2005-1-21/
> 
> From a short look:
> 
> core.eatmem.diff of course helps, but is wrong. You should talk to
> akpm to find out why shrink_all_memory is not doing its job.
> 
> i386: +       repz movsl %ds:(%esi),%es:(%edi)
> I do not think movsl has any parameters. What is repz? Repeat as long
> as it is non-zero?

No, it's "repeat until %ecx is zero or ZF is cleared", but the latter never happens
with movsl.  It's intended for cmpsl, scasl and friends (the assembler should
complain about using it here).

> I think this should be "rep movsl".

Yes, it should.


> core:
> @@ -576,92 +989,31 @@ static void copy_data_pages(void)
>                 for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn) {
>                         if (saveable(zone, &zone_pfn)) {
>                                 struct page * page;
> +                               pbe = find_pbe_by_index(pagedir_nosave, nr_copy_pages-to_copy);
> +                               BUG_ON(pbe == NULL);
>                                 page = pfn_to_page(zone_pfn + zone->zone_start_pfn);
> 
> Don't you introduce O(n^2) behaviour here? Should not it be something
> like pbe_next? And it is the only user of find_pbe_by_index().
> 
> I think that read_one_pbe() is too short to be uninlined... Same for
> read_one_pagedir and write_one_pbe().
> 
> alloc_one_pagedir: why not just alloc page as zeroed?
> 
> Okay, it is still too big to merge directly. Would it be possible to
> get mod_printk_progress(), introduce *_for_each (but leave there old
> implementations), introduce pagedir_free() (but leave old
> implementation). Better collision code should already be there, that
> should make patch smaller, too. Try not to move code around.

I have a suggestion.

hugang, you are currently replacing an array of pbes with a list of arrays
of pbes contained within individual pages.

I would go further and replace it with a single one-directional list
of pbes.  Namely, I would modify "struct pbe" in the following way:

struct pbe {
	unsigned long address;
	unsigned long orig_address;
	swp_entry_t swap_address;	
	struct pbe *next;
};

(AFAICT, the "dummy" field is only used by hugang - as a pointer)
and I would define "for_each_pbe()" as:

#define for_each_pbe(pbe, pblist) \
	for (pbe = pblist;  pbe;  pbe = pbe->next)

Then, the only non-trivial changes would be in alloc_pagedir() and
in swsusp_pagedir_relocate(), where I would need to link pbes to
each other.

This also would make the assembly parts independent of the
sizeof(struct pbe), which is currently hardcoded there.

What do you think?

Greets,
RJW


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[hugang]

On Fri, Jan 21, 2005 at 02:42:54PM +0100, Rafael J. Wysocki wrote:
> Hi,
> 
> 
> No, it's "repeat until %ecx is zero or ZF is cleared", but the latter never happens
> with movsl.  It's intended for cmpsl, scasl and friends (the assembler should
> complain about using it here).
> 
> > I think this should be "rep movsl".
> 
> Yes, it should.
> 
I'll change my code.

> 
> I have a suggestion.
> 
> hugang, you are currently replacing an array of pbes with a list of arrays
> of pbes contained within individual pages.
> 
> I would go further and replace it with a single one-directional list
> of pbes.  Namely, I would modify "struct pbe" in the following way:
> 
> struct pbe {
> 	unsigned long address;
> 	unsigned long orig_address;
> 	swp_entry_t swap_address;	
> 	struct pbe *next;
> };
> 
> (AFAICT, the "dummy" field is only used by hugang - as a pointer)
> and I would define "for_each_pbe()" as:
> 
> #define for_each_pbe(pbe, pblist) \
> 	for (pbe = pblist;  pbe;  pbe = pbe->next)
> 
> Then, the only non-trivial changes would be in alloc_pagedir() and
> in swsusp_pagedir_relocate(), where I would need to link pbes to
> each other.
> 
> This also would make the assembly parts independent of the
> sizeof(struct pbe), which is currently hardcoded there.
> 
> What do you think?
Thanks for point that, That's better solution than current, I'll change
current code to this.

I'm think about, how can I make chang smaller.

-- 
Hu Gang       .-.
              /v\
             // \\ 
Linux User  /(   )\  [204016]
GPG Key ID   ^^-^^   http://soulinfo.com/~hugang/hugang.asc

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[Pavel Machek]

Hi!

> > Okay, it is still too big to merge directly. Would it be possible to
> > get mod_printk_progress(), introduce *_for_each (but leave there old
> > implementations), introduce pagedir_free() (but leave old
> > implementation). Better collision code should already be there, that
> > should make patch smaller, too. Try not to move code around.
> 
> I have a suggestion.
> 
> hugang, you are currently replacing an array of pbes with a list of arrays
> of pbes contained within individual pages.

Looks good to me. You still want to be able to loop over pages (for
relocation etc), but code is going to get a lot cleaner.

Oh be warned that last pointer on the page is used to linking pages
together on-disk.
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[Rafael J. Wysocki]

On Friday, 21 of January 2005 03:23, hugang@soulinfo.com wrote:
> On Thu, Jan 20, 2005 at 10:46:37PM +0100, Rafael J. Wysocki wrote:
> > On Thursday, 20 of January 2005 21:59, Pavel Machek wrote:
> > 
> > Sure, but I think it's there for a reason.
> > 
> > > Anyway, this is likely to clash with hugang's work; I'd prefer this not to be applied.
> > 
> > I am aware of that, but you are not going to merge the hugang's patches soon, are you?
> > If necessary, I can change the patch to work with his code (hugang, what do you think?).
> > 
> I like this patch, And I change my code with this, Please have a look,
> It pass in qemu X86_64. :)

Looks good.  I'll test it later today.

Greets,
RJW


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 1291 bytes --]

On Friday, 21 of January 2005 13:32, Rafael J. Wysocki wrote:
> On Friday, 21 of January 2005 03:23, hugang@soulinfo.com wrote:
> > On Thu, Jan 20, 2005 at 10:46:37PM +0100, Rafael J. Wysocki wrote:
> > > On Thursday, 20 of January 2005 21:59, Pavel Machek wrote:
> > > 
> > > Sure, but I think it's there for a reason.
> > > 
> > > > Anyway, this is likely to clash with hugang's work; I'd prefer this not to be applied.
> > > 
> > > I am aware of that, but you are not going to merge the hugang's patches soon, are you?
> > > If necessary, I can change the patch to work with his code (hugang, what do you think?).
> > > 
> > I like this patch, And I change my code with this, Please have a look,
> > It pass in qemu X86_64. :)
> 
> Looks good.  I'll test it later today.

It works, but I had to change the "core" patch, so that it applied cleanly to
a "fresh" 2.6.11-rc1-mm2 (the patch that I used is attached).  I also removed
some "pr_debug()" statemets that didn't help me at all. :-)

I noticed that it was significantly slower at writing to and reading from swap
than the unpatched swsusp.

Greets,
RJW


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

[-- Attachment #2: 2005-1-18.core-2.6.11-rc1-mm2.patch --]
[-- Type: text/x-diff, Size: 21622 bytes --]

--- /home/rafael/tmp/kernel/testing/linux-2.6.11-rc1-mm2/kernel/power/swsusp.c	2005-01-21 18:30:41.000000000 +0100
+++ linux-2.6.11-rc1-mm2/kernel/power/swsusp.c	2005-01-21 19:46:57.000000000 +0100
@@ -76,7 +76,6 @@
 extern const void __nosave_begin, __nosave_end;
 
 /* Variables to be preserved over suspend */
-static int pagedir_order_check;
 static int nr_copy_pages_check;
 
 extern char resume_file[];
@@ -99,7 +98,6 @@
  */
 suspend_pagedir_t *pagedir_nosave __nosavedata = NULL;
 static suspend_pagedir_t *pagedir_save;
-static int pagedir_order __nosavedata = 0;
 
 #define SWSUSP_SIG	"S1SUSPEND"
 
@@ -259,8 +257,402 @@
 	return error;
 }
 
+static int mod_progress = 1;
+
+static void inline mod_printk_progress(int i)
+{
+	if (mod_progress == 0) mod_progress = 1;
+	if (!(i%100))
+		printk( "\b\b\b\b%3d%%", i / mod_progress );
+}
+
+#define PBE_PAGE_NUMS		(PAGE_SIZE/sizeof(struct pbe))
+#define PBE_IS_PAGE_END(x) \
+	( PAGE_SIZE - sizeof(struct pbe) == ((x) - ((~(PAGE_SIZE - 1)) & (x))) )
+
+/**
+ * pgdir_for_each - iterate over a pagedir list
+ * @pos:	the &suspend_pagedir_t to use as a loop conter.
+ * @n:		another &suspend_pagedir_t to use as tempory storage.
+ * @head:	the head for your list.
+ */
+#define pgdir_for_each(pos, n, head) \
+	for(pos = head, n = pos ? (suspend_pagedir_t*)pos->dummy.val : NULL; \
+			pos != NULL; \
+			pos = n, n = pos ? (suspend_pagedir_t *)pos->dummy.val : NULL)
+
+/**
+ * pbe_for_each - iterate over a page backup entry list
+ * @pos:	the &struct pbe to use as a loop conter.
+ * @n		another &struct pbe to use as tempory storage.
+ * @index:	show current index 
+ * @max:	max index in this list
+ * @head:	the head for your list.
+ */
+#define pbe_for_each(pos, n, index, max, head) \
+	for(pos = head, index = 0,\
+			n = pos ? (struct pbe *)pos->dummy.val : NULL; \
+		(pos != NULL) && (index < max); \
+		pos = (PBE_IS_PAGE_END((unsigned long)pos)) ? n : \
+			((struct pbe *)((unsigned long)pos + sizeof(struct pbe))), \
+			index ++, \
+			n = pos ? (struct pbe*)pos->dummy.val : NULL)
+
+/**
+ * find_pbe_by_index - find a pbe by index
+ * @head:	the head for your list.
+ *
+ * @return:	NULL is failed.
+ */
+static inline struct pbe *find_pbe_by_index(struct pbe *head, int index)
+{
+	unsigned long p = 0;
+	struct pbe *pbe, *next;
+
+	pgdir_for_each(pbe, next, head) {
+		if (p == index / PBE_PAGE_NUMS) {
+			pbe = (struct pbe *)((unsigned long)pbe + 
+					(index % PBE_PAGE_NUMS) * sizeof(struct pbe));
+			return pbe;
+		}
+		p ++;
+	}
+	return (NULL);
+}
+
+/**
+ * pagedir_free - free the pagedir list storage 
+ * @head:	the head for your list.
+ */
+static inline void pagedir_free(suspend_pagedir_t *head)
+{
+	suspend_pagedir_t *next, *cur;
+	pgdir_for_each(cur, next, head) 
+		free_page((unsigned long)cur);
+}
+
+static int bio_read_page(pgoff_t page_off, void * page);
+
+/**
+ *  write_one_pbe -	 write a page backup entry to swap device
+ *  @p:		the page backup entry pointer
+ *  @data:	the data page pointer
+ *  @cur:	current index 
+ *
+ *  @return: 0 is ok.
+ */
+static int inline write_one_pbe(struct pbe *p, void *data, int cur)
+{
+	int error = 0;
+
+	mod_printk_progress(cur);
+
+	error = write_page((unsigned long)data, &p->swap_address);
+	if (error) return error;
+	return 0;
+}
+static int inline read_one_pbe(struct pbe *p, void *data, int cur)
+{
+	int error = 0;
+
+	mod_printk_progress(cur);
+
+	error = bio_read_page(swp_offset(p->swap_address), data);
+	if (error) return error;
+	return 0;
+}
+
+/**
+ * Returns true if given address/order collides with any orig_address 
+ */
+static int inline does_collide_order(unsigned long addr, int order) 
+{
+	int i;
+
+	for (i=0; i < (1<<order); i++)
+		if(!PageNosaveFree(virt_to_page(addr + i * PAGE_SIZE)))
+			return 1;
+	return 0;
+}
+
+
+static void **eaten_memory = NULL;
+
+/**
+ * swsusp_get_safe_free_page - a get_free_pages wrapper.
+ * 
+ * in resume we must make sure the page not collide with old pages, first call 
+ * get_free_page, if that's collide, adding it in a easy list, then try next, 
+ * until get un colliede page.
+ *
+ * @collide:	if true enable collide check.
+ * 
+ */
+static void *swsusp_get_safe_free_page(int collide)
+{
+	void *addr = NULL;
+	void **c = eaten_memory;
+
+	do {
+		if (addr) {
+			eaten_memory = (void**)addr;
+			*eaten_memory = c;
+			c = eaten_memory;
+			printk(".");
+		}
+		addr = (void*)__get_free_pages(GFP_ATOMIC | __GFP_COLD, 0);
+		if (!addr) {
+			printk("out of memory\n");
+			return NULL;
+		}
+	} while (collide && does_collide_order((unsigned long)addr, 0));
+
+	if (collide)
+		ClearPageNosaveFree(virt_to_page(addr)); 
+
+	return addr;
+}
+
+/**
+ * alloc_one_pagedir - allocate a new pagedir 
+ * @prev:	last pagedir pointer
+ * @collide:
+ * 
+ */
+static suspend_pagedir_t * alloc_one_pagedir(suspend_pagedir_t *prev, 
+		int collide)
+{
+	suspend_pagedir_t *pgdir = NULL;
+	int i;
+
+	pgdir = (suspend_pagedir_t *)swsusp_get_safe_free_page(collide);
+
+	if (!pgdir) 
+		return NULL;
+
+	for (i = 0; i < PBE_PAGE_NUMS; i++) {
+		pgdir[i].dummy.val = 0;
+		pgdir[i].address = 0;
+		pgdir[i].orig_address = 0;
+		if (prev)
+			prev[i].dummy.val= (unsigned long)pgdir;
+	}
+
+	return (pgdir);
+}
+
+/* calc_nums - Determine the nums of allocation needed for pagedir_save. */
+static int calc_nums(int nr_copy)
+{
+	int diff = 0, ret = 0;
+	do {
+		diff = (nr_copy / PBE_PAGE_NUMS) - ret + 1;
+		if (diff) {
+			ret += diff;
+			nr_copy += diff;
+		}
+	} while (diff);
+	return nr_copy;
+}
 
 /**
+ * alloc_pagedir - Allocate the page directory.
+ * @pbe:
+ * @pbe_nums:	needs how many page backup entry.
+ * @collide:	true is need do collide check.
+ * @page_nums:	if we knows need how many pages for pagedir set it, 
+ * 			    only using resume stage, 0 is known.
+ *
+ * @return: 	< 0 is failed.
+ */
+static int alloc_pagedir(struct pbe **pbe, 
+		int pbe_nums, int collide, int page_nums)
+{
+	unsigned int nums = 0;
+	unsigned int after_alloc = pbe_nums;
+	suspend_pagedir_t *prev = NULL, *cur = NULL;
+
+	if (page_nums)
+		after_alloc = PBE_PAGE_NUMS * page_nums;
+	else
+		after_alloc = calc_nums(after_alloc);
+	for (nums = 0 ; nums < after_alloc ; nums += PBE_PAGE_NUMS) {
+		cur = alloc_one_pagedir(prev, collide);
+		if (!cur) { /* get page failed */
+			goto no_mem;
+		}
+		if (nums == 0) { /* setup the head */
+			*pbe = cur;
+		}
+		prev = cur;
+	}
+	return after_alloc - pbe_nums;
+
+no_mem:
+	pagedir_free(*pbe);
+	*pbe = NULL;
+
+	return (-ENOMEM);
+}
+
+static int __init check_one_pbe(struct pbe *p, int cur)
+{
+	unsigned long addr = 0;
+
+	addr = (unsigned long)swsusp_get_safe_free_page(1);
+	if(!addr)
+		return -ENOMEM;
+	p->address = addr;
+
+	return 0;
+}
+
+static void __init swsusp_copy_pagedir(suspend_pagedir_t *d_pgdir, 
+		suspend_pagedir_t *s_pgdir)
+{
+	int i = 0;
+
+	while (s_pgdir != NULL) {
+		suspend_pagedir_t *s_next = (suspend_pagedir_t *)s_pgdir->dummy.val;
+		suspend_pagedir_t *d_next = (suspend_pagedir_t *)d_pgdir->dummy.val;
+		for (i = 0; i < PBE_PAGE_NUMS; i++) {
+			d_pgdir->address = s_pgdir->address;
+			d_pgdir->orig_address = s_pgdir->orig_address;
+			d_pgdir->swap_address = s_pgdir->swap_address;
+			s_pgdir ++; d_pgdir ++;
+		}
+		d_pgdir = d_next;
+		s_pgdir = s_next;
+	};
+}
+
+/**
+ * We check here that pagedir & pages it points to won't collide with pages
+ * where we're going to restore from the loaded pages later
+ */
+static int __init check_pagedir(void)
+{
+	void **c, *f;
+	struct pbe *next, *pos;
+	int error, index;
+	suspend_pagedir_t *addr = NULL;
+	struct zone *zone;
+	unsigned long zone_pfn;
+
+	printk("Relocating pagedir ... ");
+	/* Set page flags */
+	for_each_zone(zone) {
+		for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn)
+			SetPageNosaveFree(pfn_to_page(zone_pfn + zone->zone_start_pfn));
+	}
+
+	/* Clear orig address */
+	pbe_for_each(pos, next, index, nr_copy_pages, pagedir_nosave) {
+		ClearPageNosaveFree(virt_to_page(pos->orig_address));
+	}
+	
+	error = alloc_pagedir(&addr, nr_copy_pages, 1, swsusp_info.pagedir_pages);
+	if (error < 0) {
+		return error;
+	}
+	swsusp_copy_pagedir(addr, pagedir_nosave);
+	pagedir_free(pagedir_nosave);
+
+	/* check copy address */
+	pbe_for_each(pos, next, index, nr_copy_pages, addr) {
+		error = check_one_pbe(pos, index);
+		BUG_ON(error);
+	}
+
+	/* free eaten memory */
+	c = eaten_memory;
+	while (c) {
+		printk(":");
+		f = c;
+		c = *c;
+		free_pages((unsigned long)f, 0);
+	}
+
+	printk(" done\n");
+
+	pagedir_nosave = addr;
+
+	return 0;
+}
+
+/**
+ * read_one_pagedir - read one pagedir from swap device 
+ * @pgdir:	the pgdir pointer
+ * @i:		
+ */
+static int __init read_one_pagedir(suspend_pagedir_t *pgdir, int i)
+{
+	unsigned long offset = swp_offset(swsusp_info.pagedir[i]);
+	unsigned long next;
+	int error = 0;
+
+	next = pgdir->dummy.val;
+	if ((error = bio_read_page(offset, (void *)pgdir))) {
+		return error;
+	}
+	pgdir->dummy.val = next;
+
+	return error;
+}
+
+/**
+ *  for_each_pbe_copy_back - 
+ *
+ *  That usefuly for help us writing the code in assemble code
+ *
+ *  1: enable CREATE_ASM_CODE
+ *  2: make kernel/power/swsusp.o
+ *  3: objdump -dx kernel/power/swsusp.o > /tmp/swsusp.s
+ *  4: vi /tmp/swsusp.s
+ *
+ */
+/*#define CREATE_ASM_CODE*/
+#ifdef CREATE_ASM_CODE
+#if 0 /* if your copy back code is running in real mode, enable it */
+#define GET_ADDRESS(x) __pa(x) 
+#else
+#define GET_ADDRESS(x) (x)
+#endif
+asmlinkage void for_each_pbe_copy_back(void)
+{
+	register struct pbe *pgdir, *next;
+
+	pgdir = pagedir_nosave;
+	while (pgdir != NULL) {
+		register unsigned long i;
+		pgdir = (struct pbe *)GET_ADDRESS(pgdir);
+		next = (struct pbe*)pgdir->dummy.val;
+		/* copy a suspend pagedir */
+		for (i = 0; i < PBE_PAGE_NUMS; i++, pgdir ++) {
+			register unsigned long *orig, *copy;
+			orig = (unsigned long *)pgdir->orig_address;
+			if (orig == 0) goto end;
+			orig = (unsigned long *)GET_ADDRESS(orig);
+			copy = (unsigned long *)GET_ADDRESS(pgdir->address);
+#if 1
+			/* copy page data */
+			for (i = 0; i < PAGE_SIZE / sizeof(unsigned long); i+=4) {
+				*(orig + i) = *(copy + i);
+				*(orig + i+1) = *(copy + i+1);
+				*(orig + i+2) = *(copy + i+2);
+				*(orig + i+3) = *(copy + i+3);
+			}
+#else
+			memcpy(orig, copy, PAGE_SIZE);
+#endif
+		}
+		pgdir = next;
+	}
+end:
+	panic("just asm code");
+}
+#endif
+/**
  *	data_free - Free the swap entries used by the saved image.
  *
  *	Walk the list of used swap entries and free each one. 
@@ -271,14 +663,15 @@
 {
 	swp_entry_t entry;
 	int i;
+	struct pbe *next, *pos;
 
-	for (i = 0; i < nr_copy_pages; i++) {
-		entry = (pagedir_nosave + i)->swap_address;
+	pbe_for_each(pos, next, i, nr_copy_pages, pagedir_nosave) {
+		entry = pos->swap_address;
 		if (entry.val)
 			swap_free(entry);
 		else
 			break;
-		(pagedir_nosave + i)->swap_address = (swp_entry_t){0};
+		pos->swap_address = (swp_entry_t){0};
 	}
 }
 
@@ -293,17 +686,15 @@
 {
 	int error = 0;
 	int i;
-	unsigned int mod = nr_copy_pages / 100;
+	struct pbe *pos, *next;
 
-	if (!mod)
-		mod = 1;
+	mod_progress = nr_copy_pages / 100;
 
 	printk( "Writing data to swap (%d pages)...     ", nr_copy_pages );
-	for (i = 0; i < nr_copy_pages && !error; i++) {
-		if (!(i%mod))
-			printk( "\b\b\b\b%3d%%", i / mod );
-		error = write_page((pagedir_nosave+i)->address,
-					  &((pagedir_nosave+i)->swap_address));
+	pbe_for_each(pos, next, i, nr_copy_pages, pagedir_nosave) {
+		BUG_ON(pos->orig_address == 0);
+		error = write_one_pbe(pos, (void*)pos->address, i);
+		if (error) break;
 	}
 	printk("\b\b\b\bdone\n");
 	return error;
@@ -373,15 +764,17 @@
 
 static int write_pagedir(void)
 {
-	unsigned long addr = (unsigned long)pagedir_nosave;
 	int error = 0;
-	int n = SUSPEND_PD_PAGES(nr_copy_pages);
-	int i;
+	int n = 0;
+	suspend_pagedir_t *pgdir, *next;
 
-	swsusp_info.pagedir_pages = n;
+	pgdir_for_each(pgdir, next, pagedir_nosave) {
+		error = write_page((unsigned long)pgdir, &swsusp_info.pagedir[n]);
+		if (error) break;
+		n ++;
+	}
 	printk( "Writing pagedir (%d pages)\n", n);
-	for (i = 0; i < n && !error; i++, addr += PAGE_SIZE)
-		error = write_page(addr, &swsusp_info.pagedir[i]);
+	swsusp_info.pagedir_pages = n;
 	return error;
 }
 
@@ -566,9 +959,9 @@
 {
 	struct zone *zone;
 	unsigned long zone_pfn;
-	struct pbe * pbe = pagedir_nosave;
+	struct pbe * pbe = NULL;
 	int to_copy = nr_copy_pages;
-	
+
 	for_each_zone(zone) {
 		if (is_highmem(zone))
 			continue;
@@ -576,92 +969,31 @@
 		for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn) {
 			if (saveable(zone, &zone_pfn)) {
 				struct page * page;
+				pbe = find_pbe_by_index(pagedir_nosave, nr_copy_pages-to_copy);
+				BUG_ON(pbe == NULL);
 				page = pfn_to_page(zone_pfn + zone->zone_start_pfn);
 				pbe->orig_address = (long) page_address(page);
+				BUG_ON(pbe->orig_address == 0);
+				BUG_ON(pbe->address == 0);
 				/* copy_page is not usable for copying task structs. */
 				memcpy((void *)pbe->address, (void *)pbe->orig_address, PAGE_SIZE);
-				pbe++;
-				to_copy--;
+				to_copy --;
 			}
 		}
 	}
 	BUG_ON(to_copy);
 }
 
-
-/**
- *	calc_order - Determine the order of allocation needed for pagedir_save.
- *
- *	This looks tricky, but is just subtle. Please fix it some time.
- *	Since there are %nr_copy_pages worth of pages in the snapshot, we need
- *	to allocate enough contiguous space to hold 
- *		(%nr_copy_pages * sizeof(struct pbe)), 
- *	which has the saved/orig locations of the page.. 
- *
- *	SUSPEND_PD_PAGES() tells us how many pages we need to hold those 
- *	structures, then we call get_bitmask_order(), which will tell us the
- *	last bit set in the number, starting with 1. (If we need 30 pages, that
- *	is 0x0000001e in hex. The last bit is the 5th, which is the order we 
- *	would use to allocate 32 contiguous pages).
- *
- *	Since we also need to save those pages, we add the number of pages that
- *	we need to nr_copy_pages, and in case of an overflow, do the 
- *	calculation again to update the number of pages needed. 
- *
- *	With this model, we will tend to waste a lot of memory if we just cross
- *	an order boundary. Plus, the higher the order of allocation that we try
- *	to do, the more likely we are to fail in a low-memory situtation 
- *	(though	we're unlikely to get this far in such a case, since swsusp 
- *	requires half of memory to be free anyway).
- */
-
-
-static void calc_order(void)
-{
-	int diff = 0;
-	int order = 0;
-
-	do {
-		diff = get_bitmask_order(SUSPEND_PD_PAGES(nr_copy_pages)) - order;
-		if (diff) {
-			order += diff;
-			nr_copy_pages += 1 << diff;
-		}
-	} while(diff);
-	pagedir_order = order;
-}
-
-
-/**
- *	alloc_pagedir - Allocate the page directory.
- *
- *	First, determine exactly how many contiguous pages we need and
- *	allocate them.
- */
-
-static int alloc_pagedir(void)
-{
-	calc_order();
-	pagedir_save = (suspend_pagedir_t *)__get_free_pages(GFP_ATOMIC | __GFP_COLD,
-							     pagedir_order);
-	if (!pagedir_save)
-		return -ENOMEM;
-	memset(pagedir_save, 0, (1 << pagedir_order) * PAGE_SIZE);
-	pagedir_nosave = pagedir_save;
-	return 0;
-}
-
 /**
  *	free_image_pages - Free pages allocated for snapshot
  */
 
 static void free_image_pages(void)
 {
-	struct pbe * p;
+	struct pbe * p, * n;
 	int i;
 
-	p = pagedir_save;
-	for (i = 0, p = pagedir_save; i < nr_copy_pages; i++, p++) {
+	pbe_for_each(p, n, i, nr_copy_pages, pagedir_save) {
 		if (p->address) {
 			ClearPageNosave(virt_to_page(p->address));
 			free_page(p->address);
@@ -677,10 +1009,10 @@
 
 static int alloc_image_pages(void)
 {
-	struct pbe * p;
+	struct pbe * p, * n;
 	int i;
 
-	for (i = 0, p = pagedir_save; i < nr_copy_pages; i++, p++) {
+	pbe_for_each(p, n, i, nr_copy_pages, pagedir_save) {
 		p->address = get_zeroed_page(GFP_ATOMIC | __GFP_COLD);
 		if (!p->address)
 			return -ENOMEM;
@@ -694,7 +1026,7 @@
 	BUG_ON(PageNosave(virt_to_page(pagedir_save)));
 	BUG_ON(PageNosaveFree(virt_to_page(pagedir_save)));
 	free_image_pages();
-	free_pages((unsigned long) pagedir_save, pagedir_order);
+	pagedir_free(pagedir_save);
 }
 
 
@@ -752,18 +1084,19 @@
 	if (!enough_swap())
 		return -ENOSPC;
 
-	if ((error = alloc_pagedir())) {
+	if ((error = alloc_pagedir(&pagedir_save, nr_copy_pages, 0, 0)) < 0) {
 		printk(KERN_ERR "suspend: Allocating pagedir failed.\n");
 		return error;
 	}
+	pr_debug("alloc_pagedir: addon %d\n", error);
+	nr_copy_pages += error;
 	if ((error = alloc_image_pages())) {
 		printk(KERN_ERR "suspend: Allocating image pages failed.\n");
 		swsusp_free();
 		return error;
 	}
-
+	pagedir_nosave = pagedir_save;
 	nr_copy_pages_check = nr_copy_pages;
-	pagedir_order_check = pagedir_order;
 	return 0;
 }
 
@@ -867,7 +1200,6 @@
 asmlinkage int swsusp_restore(void)
 {
 	BUG_ON (nr_copy_pages_check != nr_copy_pages);
-	BUG_ON (pagedir_order_check != pagedir_order);
 	
 	/* Even mappings of "global" things (vmalloc) need to be fixed */
 	__flush_tlb_global();
@@ -893,109 +1225,6 @@
 	return error;
 }
 
-/* More restore stuff */
-
-/*
- * Returns true if given address/order collides with any orig_address 
- */
-static int __init does_collide_order(unsigned long addr, int order)
-{
-	int i;
-	
-	for (i=0; i < (1<<order); i++)
-		if (!PageNosaveFree(virt_to_page(addr + i * PAGE_SIZE)))
-			return 1;
-	return 0;
-}
-
-/*
- * We check here that pagedir & pages it points to won't collide with pages
- * where we're going to restore from the loaded pages later
- */
-static int __init check_pagedir(void)
-{
-	int i;
-
-	for(i=0; i < nr_copy_pages; i++) {
-		unsigned long addr;
-
-		do {
-			addr = get_zeroed_page(GFP_ATOMIC);
-			if(!addr)
-				return -ENOMEM;
-		} while (does_collide_order(addr, 0));
-
-		(pagedir_nosave+i)->address = addr;
-	}
-	return 0;
-}
-
-static int __init swsusp_pagedir_relocate(void)
-{
-	/*
-	 * We have to avoid recursion (not to overflow kernel stack),
-	 * and that's why code looks pretty cryptic 
-	 */
-	suspend_pagedir_t *old_pagedir = pagedir_nosave;
-	void **eaten_memory = NULL;
-	void **c = eaten_memory, *m, *f;
-	int ret = 0;
-	struct zone *zone;
-	int i;
-	struct pbe *p;
-	unsigned long zone_pfn;
-
-	printk("Relocating pagedir ");
-
-	/* Set page flags */
-
-	for_each_zone(zone) {
-        	for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn)
-                	SetPageNosaveFree(pfn_to_page(zone_pfn +
-					zone->zone_start_pfn));
-	}
-
-	/* Clear orig address */
-
-	for(i = 0, p = pagedir_nosave; i < nr_copy_pages; i++, p++) {
-		ClearPageNosaveFree(virt_to_page(p->orig_address));
-	}
-
-	if (!does_collide_order((unsigned long)old_pagedir, pagedir_order)) {
-		printk("not necessary\n");
-		return check_pagedir();
-	}
-
-	while ((m = (void *) __get_free_pages(GFP_ATOMIC, pagedir_order)) != NULL) {
-		if (!does_collide_order((unsigned long)m, pagedir_order))
-			break;
-		eaten_memory = m;
-		printk( "." ); 
-		*eaten_memory = c;
-		c = eaten_memory;
-	}
-
-	if (!m) {
-		printk("out of memory\n");
-		ret = -ENOMEM;
-	} else {
-		pagedir_nosave =
-			memcpy(m, old_pagedir, PAGE_SIZE << pagedir_order);
-	}
-
-	c = eaten_memory;
-	while (c) {
-		printk(":");
-		f = c;
-		c = *c;
-		free_pages((unsigned long)f, pagedir_order);
-	}
-	if (ret)
-		return ret;
-	printk("|\n");
-	return check_pagedir();
-}
-
 /**
  *	Using bio to read from swap.
  *	This code requires a bit more work than just using buffer heads
@@ -1110,7 +1339,6 @@
 		return -EPERM;
 	}
 	nr_copy_pages = swsusp_info.image_pages;
-	pagedir_order = get_bitmask_order(SUSPEND_PD_PAGES(nr_copy_pages));
 	return error;
 }
 
@@ -1146,23 +1374,20 @@
 
 static int __init data_read(void)
 {
-	struct pbe * p;
+	struct pbe * p, * n;
 	int error;
 	int i;
-	int mod = nr_copy_pages / 100;
 
-	if (!mod)
-		mod = 1;
+	if ((error = check_pagedir())) {
+		return -ENOMEM;
+	}
 
-	if ((error = swsusp_pagedir_relocate()))
-		return error;
+	mod_progress = nr_copy_pages / 100;
 
 	printk( "Reading image data (%d pages):     ", nr_copy_pages );
-	for(i = 0, p = pagedir_nosave; i < nr_copy_pages && !error; i++, p++) {
-		if (!(i%mod))
-			printk( "\b\b\b\b%3d%%", i / mod );
-		error = bio_read_page(swp_offset(p->swap_address),
-				  (void *)p->address);
+	pbe_for_each(p, n, i, nr_copy_pages, pagedir_nosave) {
+		error = read_one_pbe(p, (void*)p->address, i);
+		if (error) break;
 	}
 	printk(" %d done.\n",i);
 	return error;
@@ -1173,26 +1398,24 @@
 
 static int __init read_pagedir(void)
 {
-	unsigned long addr;
-	int i, n = swsusp_info.pagedir_pages;
-	int error = 0;
+	int error = 0, i = 0, n = swsusp_info.pagedir_pages;
+	suspend_pagedir_t *pgdir, *next;
 
-	addr = __get_free_pages(GFP_ATOMIC, pagedir_order);
-	if (!addr)
-		return -ENOMEM;
-	pagedir_nosave = (struct pbe *)addr;
+	error = alloc_pagedir(&pagedir_nosave, nr_copy_pages, 0, n);
+	if (error < 0)
+		return error;
 
 	pr_debug("swsusp: Reading pagedir (%d Pages)\n",n);
 
-	for (i = 0; i < n && !error; i++, addr += PAGE_SIZE) {
-		unsigned long offset = swp_offset(swsusp_info.pagedir[i]);
-		if (offset)
-			error = bio_read_page(offset, (void *)addr);
-		else
-			error = -EFAULT;
+	pgdir_for_each(pgdir, next, pagedir_nosave) {
+		error = read_one_pagedir(pgdir, i);
+		if (error) break;
+		pgdir[PBE_PAGE_NUMS-1].dummy.val = pgdir ? pgdir ->dummy.val : 0;
+		i++;
 	}
+	BUG_ON(i != n);
 	if (error)
-		free_pages((unsigned long)pagedir_nosave, pagedir_order);
+		pagedir_free(pagedir_nosave);
 	return error;
 }
 
@@ -1207,7 +1430,7 @@
 	if ((error = read_pagedir()))
 		return error;
 	if ((error = data_read()))
-		free_pages((unsigned long)pagedir_nosave, pagedir_order);
+		pagedir_free(pagedir_nosave);
 	return error;
 }
 

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[Andi Kleen]

> With this patch, at least 8 times less memory accesses are required to restore an image
> than without it, and in the original code cr3 is reloaded after copying each _byte_,
> let alone the SIB arithmetics.  I'd expect it to be 10 times faster or so.

Probably more. CR3 reload is a serializing operation and is really expensive.

-Andi

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[Rafael J. Wysocki]

On Thursday, 20 of January 2005 20:32, Rafael J. Wysocki wrote:
> Hi,
> 
> The following patch speeds up the restoring of swsusp images on x86-64
> and makes the assembly code more readable (tested and works on AMD64).  It's
> against 2.6.11-rc1-mm1, but applies to 2.6.11-rc1-mm2.  Please consifer for applying.

s/consifer/consider/

Sorry for the typo,
RJW


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[Andi Kleen]

On Thu, Jan 20, 2005 at 08:32:31PM +0100, Rafael J. Wysocki wrote:
> Hi,
> 
> The following patch speeds up the restoring of swsusp images on x86-64
> and makes the assembly code more readable (tested and works on AMD64).  It's
> against 2.6.11-rc1-mm1, but applies to 2.6.11-rc1-mm2.  Please consifer for applying.
> 
> Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Thanks. I applied it with some small changes to not hardcode any 
C fields. 

BTW Pavel, while reading the code I noticed some dubious things
in the code:

- The TLB flush doesn't flush global pages (turn of PGE and turn it
on again). Since that handles kernel pages which are marked global
this is surely wrong. 

- Also is it really needed to flush the TLB after each page and wouldn't
INVLPG be better here? Or do you want to flush other pages than the
just copied one there too? INVLPG would also take care of the global
pages at least on x86-64 (iirc there are some bugs in this regard on some
older i386 cpus) 

- There is a comment that says the code shouldn't use stack, but 
it definitely uses the stack for some things. Either the comment
or the code is wrong. Which is?


-Andi


The following patch speeds up the restoring of swsusp images on x86-64
and makes the assembly code more readable (tested and works on AMD64).  It's
against 2.6.11-rc1-mm1, but applies to 2.6.11-rc1-mm2.  Please consifer for applying.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

Changed by AK to not hardcode any C values and get them from offset.h
instead.

Signed-off-by: Andi Kleen <ak@suse.de>

Index: linux/arch/x86_64/kernel/suspend_asm.S
===================================================================
--- linux.orig/arch/x86_64/kernel/suspend_asm.S	2004-10-19 01:55:08.%N +0200
+++ linux/arch/x86_64/kernel/suspend_asm.S	2005-01-22 03:20:28.%N +0100
@@ -11,6 +12,7 @@
 #include <linux/linkage.h>
 #include <asm/segment.h>
 #include <asm/page.h>
+#include <asm/offset.h>
 
 ENTRY(swsusp_arch_suspend)
 
@@ -49,43 +51,31 @@
 	movq	%rcx, %cr3;
 	movq	%rax, %cr4;  # turn PGE back on
 
+	movq	pagedir_nosave(%rip), %rdx
+	/* compute the limit */
 	movl	nr_copy_pages(%rip), %eax
-	xorl	%ecx, %ecx
-	movq	$0, %r10
 	testl	%eax, %eax
 	jz	done
-.L105:
-	xorl	%esi, %esi
-	movq	$0, %r11
-	jmp	.L104
-	.p2align 4,,7
-copy_one_page:
-	movq	%r10, %rcx
-.L104:
-	movq	pagedir_nosave(%rip), %rdx
-	movq	%rcx, %rax
-	salq	$5, %rax
-	movq	8(%rdx,%rax), %rcx
-	movq	(%rdx,%rax), %rax
-	movzbl	(%rsi,%rax), %eax
-	movb	%al, (%rsi,%rcx)
-
-	movq	%cr3, %rax;  # flush TLB
-	movq	%rax, %cr3;
-
-	movq	%r11, %rax
-	incq	%rax
-	cmpq	$4095, %rax
-	movq	%rax, %rsi
-	movq	%rax, %r11
-	jbe	copy_one_page
-	movq	%r10, %rax
-	incq	%rax
-	movq	%rax, %rcx
-	movq	%rax, %r10
-	mov	nr_copy_pages(%rip), %eax
-	cmpq	%rax, %rcx
-	jb	.L105
+	movq	%rdx,%r8
+	movl	$SIZEOF_PBE,%r9d
+	mul		%r9  # with rax, clobbers rdx
+	movq 	%r8, %rdx
+	addq	%r8, %rax
+loop:
+	/* get addresses from the pbe and copy the page */
+	movq	pbe_address(%rdx), %rsi
+	movq	pbe_orig_address(%rdx), %rdi
+	movq	$512, %rcx
+	rep
+	movsq
+
+	movq	%cr3, %rcx;  # flush TLB
+	movq	%rcx, %cr3;
+
+	/* progress to the next pbe */
+	addq	$SIZEOF_PBE, %rdx
+	cmpq	%rax, %rdx
+	jb	loop
 done:
 	movl	$24, %eax
 	movl	%eax, %ds
Index: linux/arch/x86_64/kernel/asm-offsets.c
===================================================================
--- linux.orig/arch/x86_64/kernel/asm-offsets.c	2004-10-19 01:55:08.%N +0200
+++ linux/arch/x86_64/kernel/asm-offsets.c	2005-01-22 03:09:50.%N +0100
@@ -8,6 +8,7 @@
 #include <linux/stddef.h>
 #include <linux/errno.h> 
 #include <linux/hardirq.h>
+#include <linux/suspend.h>
 #include <asm/pda.h>
 #include <asm/processor.h>
 #include <asm/segment.h>
@@ -61,6 +62,8 @@
 	       offsetof (struct rt_sigframe32, uc.uc_mcontext));
 	BLANK();
 #endif
-
+	DEFINE(SIZEOF_PBE, sizeof(struct pbe));
+	DEFINE(pbe_address, offsetof(struct pbe, address));
+	DEFINE(pbe_orig_address, offsetof(struct pbe, orig_address));	
 	return 0;
 }

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[Pavel Machek]

Hi!

> > The following patch speeds up the restoring of swsusp images on x86-64
> > and makes the assembly code more readable (tested and works on AMD64).  It's
> > against 2.6.11-rc1-mm1, but applies to 2.6.11-rc1-mm2.  Please consifer for applying.
> > 
> > Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>
> 
> Thanks. I applied it with some small changes to not hardcode any 
> C fields. 
> 
> BTW Pavel, while reading the code I noticed some dubious things
> in the code:
> 
> - The TLB flush doesn't flush global pages (turn of PGE and turn it
> on again). Since that handles kernel pages which are marked global
> this is surely wrong. 
> 
> - Also is it really needed to flush the TLB after each page and wouldn't
> INVLPG be better here? Or do you want to flush other pages than the
> just copied one there too? INVLPG would also take care of the global
> pages at least on x86-64 (iirc there are some bugs in this regard on some
> older i386 cpus) 
> 
> - There is a comment that says the code shouldn't use stack, but 
> it definitely uses the stack for some things. Either the comment
> or the code is wrong. Which is?

This should address it... I attempted to put answers into the
comments, because probably everyone is interested in those....
									Pavel

--- clean/arch/x86_64/kernel/suspend_asm.S	2004-10-19 14:16:28.000000000 +0200
+++ linux/arch/x86_64/kernel/suspend_asm.S	2005-01-22 10:51:28.000000000 +0100
@@ -1,10 +1,16 @@
-/* Originally gcc generated, modified by hand
+/* Copyright 2004,2005 Pavel Machek <pavel@suse.cz>, Andi Kleen <ak@suse.de>, Rafael J. Wysocki <rjw@sisk.pl>
  *
- * This may not use any stack, nor any variable that is not "NoSave":
+ * Distribute under GPLv2.
+ * 
+ * swsusp_arch_resume may not use any stack, nor any variable that is
+ * not "NoSave" during copying pages:
  *
  * Its rewriting one kernel image with another. What is stack in "old"
  * image could very well be data page in "new" image, and overwriting
  * your own stack under you is bad idea.
+ *
+ * TLB flush is purely and debugging attempt to make it fail fast if we
+ * do something wrong. TLB is properly flushed in swsusp_restore.
  */
 	
 	.text



-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[Andi Kleen]

> + * TLB flush is purely and debugging attempt to make it fail fast if we
> + * do something wrong. TLB is properly flushed in swsusp_restore.

Did you measure it doesn't noticeable slow down suspend? CR3 reload is quite
expensive, and doing it for each page is quite often.

Also if you want to really flush everything you should do a global
flush.

-Andi

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[Pavel Machek]

Hi!

> > + * TLB flush is purely and debugging attempt to make it fail fast if we
> > + * do something wrong. TLB is properly flushed in swsusp_restore.
> 
> Did you measure it doesn't noticeable slow down suspend? CR3 reload is quite
> expensive, and doing it for each page is quite often.

It slows it down horribly on vmware (like adding 2 minutes). On normal
hardware, copying pages does not take long enough for me to notice.

> Also if you want to really flush everything you should do a global
> flush.

That cr3 reload can probably be just removed, because swsusp is now
stable...
								Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [PATCH][RFC] swsusp: speed up image restoring on x86-64

[Andi Kleen]

> > Also if you want to really flush everything you should do a global
> > flush.
> 
> That cr3 reload can probably be just removed, because swsusp is now
> stable...

I will remove it then.
-Andi