Re: SWSUSP Discontiguous pagedir patch

Patrick Mochel
2003-03-03T00:08:37Z

Hi there.

> 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.

All in all, I think the idea of saving state to swap is dangerous for
various reasons. However, I like some of the other concepts of the code,
and will use them in developing a more palatable mechanism of doing STDs
(hehe, I love saying that). Once I've successfully broken out the pieces I
want to reuse, I'll post the cumulative patch. In the meantime, the
incremental diffs can be viewed here:

http://ldm.bkbits.net:8080/linux-2.5-power

In the meantime, I do have some comments on your patch..

> 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.

> +void do_magic(int resume)
> +{
> + if (!resume) {
> + do_magic_suspend_1();
> + save_processor_state(); /* We need to capture registers and memory at "same time" */
> + asm ( "movl %esp, saved_context_esp\n\t"
> + "movl %eax, saved_context_eax\n\t"
> + "movl %ebx, saved_context_ebx\n\t"
> + "movl %ecx, saved_context_ecx\n\t"
> + "movl %edx, saved_context_edx\n\t"
> + "movl %ebp, saved_context_ebp\n\t"
> + "movl %esi, saved_context_esi\n\t"
> + "movl %edi, saved_context_edi\n\t"

On x86, %eax, %ecx, and %edx are local scratch registers, and don't need
to be saved. Note that gcc may use them, so check the assembly output.

> +/*
> + * Final function for resuming: after copying the pages to their original
> + * position, it restores the register state.
> + *
> + * What about page tables? Writing data pages may toggle
> + * accessed/dirty bits in our page tables. That should be no problems
> + * with 4MB page tables. That's why we require have_pse.
> + *
> + * This loops destroys stack from under itself, so it better should
> + * not use any stack space, itself. When this function is entered at
> + * resume time, we move stack to _old_ place. This is means that this
> + * function must use no stack and no local variables in registers,
> + * until calling restore_processor_context();
> + *
> + * Critical section here: noone should touch saved memory after
> + * do_magic_resume_1; copying works, because nr_copy_pages,
> + * pagedir_nosave, loop and loop2 are nosavedata.
> + */

Do you have something against indenting comments? ;)

> + 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();
}

Is __flush_tlb() really necessary?

> diff -ruN linux-2.5.63/include/linux/page-flags.h linux-2.5.63-01/include/linux/page-flags.h
> --- linux-2.5.63/include/linux/page-flags.h 2003-02-20 07:59:33.000000000 +1300
> +++ linux-2.5.63-01/include/linux/page-flags.h 2003-02-20 08:28:31.000000000 +1300
> @@ -74,6 +74,7 @@
> #define PG_mappedtodisk 17 /* Has blocks allocated on-disk */
> #define PG_reclaim 18 /* To be reclaimed asap */
> #define PG_compound 19 /* Part of a compound page */
> +#define PG_collides 20 /* swsusp - page used in save image */
>
> /*
> * Global page accounting. One instance per CPU. Only unsigned longs are
> @@ -256,6 +257,9 @@
> #define SetPageCompound(page) set_bit(PG_compound, &(page)->flags)
> #define ClearPageCompound(page) clear_bit(PG_compound, &(page)->flags)
>
> +#define PageCollides(page) test_bit(PG_collides, &(page)->flags)
> +#define SetPageCollides(page) set_bit(PG_collides, &(page)->flags)
> +#define ClearPageCollides(page) clear_bit(PG_collides, &(page)->flags)
> /*
> * The PageSwapCache predicate doesn't use a PG_flag at this time,
> * but it may again do so one day.
> diff -ruN linux-2.5.63/include/linux/suspend.h linux-2.5.63-01/include/linux/suspend.h
> --- linux-2.5.63/include/linux/suspend.h 2003-01-15 17:00:58.000000000 +1300
> +++ linux-2.5.63-01/include/linux/suspend.h 2003-02-20 08:27:36.000000000 +1300
> @@ -34,7 +34,7 @@
> char version[20];
> int num_cpus;
> int page_size;
> - suspend_pagedir_t *suspend_pagedir;
> + suspend_pagedir_t **suspend_pagedir;
> unsigned int num_pbes;
> struct swap_location {
> char filename[SWAP_FILENAME_MAXLENGTH];
> @@ -42,6 +42,8 @@
> };
>
> #define SUSPEND_PD_PAGES(x) (((x)*sizeof(struct pbe))/PAGE_SIZE+1)
> +#define PAGEDIR_CAPACITY(x) (((x)*PAGE_SIZE/sizeof(struct pbe)))
> +#define PAGEDIR_ENTRY(pagedir, i) (pagedir[i/PAGEDIR_CAPACITY(1)] + (i%PAGEDIR_CAPACITY(1)))
>
> /* mm/vmscan.c */
> extern int shrink_mem(void);
> @@ -61,7 +63,7 @@
> extern void thaw_processes(void);
>
> extern unsigned int nr_copy_pages __nosavedata;
> -extern suspend_pagedir_t *pagedir_nosave __nosavedata;
> +extern suspend_pagedir_t **pagedir_nosave __nosavedata;
>
> /* Communication between kernel/suspend.c and arch/i386/suspend.c */
>

This, and the rest of the deleted patch, are dubious. Once you start
adding

- more page flag bits
- functions that use double pointers

big warning alarms start going off I haven't looked that far into it yet,
but I suspect there are some design issues there that should get resolved.

-pat