From: Patrick Mochel <mochel@osdl.org>
To: Nigel Cunningham <ncunningham@clear.net.nz>
Cc: Pavel Machek <pavel@ucw.cz>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: SWSUSP Discontiguous pagedir patch
Date: Sun, 2 Mar 2003 17:55:16 -0600 (CST) [thread overview]
Message-ID: <Pine.LNX.4.33.0303021731510.1120-100000@localhost.localdomain> (raw)
In-Reply-To: <1046487717.4616.22.camel@laptop-linux.cunninghams>
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
next prev parent reply other threads:[~2003-03-03 0:08 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1045784829.3821.10.camel@laptop-linux.cunninghams>
[not found] ` <20030223223757.GA120@elf.ucw.cz>
[not found] ` <1046136752.1784.15.camel@laptop-linux.cunninghams>
[not found] ` <20030227132024.GB27084@atrey.karlin.mff.cuni.cz>
2003-02-27 18:42 ` SWSUSP Discontiguous pagedirs Nigel Cunningham
2003-03-01 4:22 ` SWSUSP Discontiguous pagedir patch Nigel Cunningham
2003-03-02 23:55 ` Patrick Mochel [this message]
2003-03-03 2:06 ` Nigel Cunningham
2003-03-03 2:31 ` Nigel Cunningham
2003-03-03 12:30 ` Pavel Machek
2003-03-04 20:36 ` Patrick Mochel
2003-03-05 20:50 ` Pavel Machek
2003-03-05 21:52 ` Linux vs Windows temperature anomaly Jonathan Lundell
2003-03-05 23:11 ` Herman Oosthuysen
2003-03-05 23:38 ` Con Kolivas
2003-03-05 23:50 ` Russell King
2003-03-06 0:29 ` Ed Sweetman
2003-03-06 0:47 ` Trever L. Adams
2003-03-06 9:45 ` Russell King
2003-03-06 1:58 ` Jonathan Lundell
2003-03-06 7:18 ` Corvus Corax
2003-03-06 7:57 ` Ed Sweetman
2003-03-06 8:18 ` Corvus Corax
2003-03-06 8:58 ` Ed Sweetman
2003-03-06 15:41 ` Jesse Pollard
2003-03-06 14:27 ` Jesse Pollard
2003-03-06 2:57 ` David Rees
2003-03-06 6:12 ` Matthias Schniedermeyer
2003-03-06 16:07 ` Jonathan Lundell
2003-03-07 0:40 ` Horst von Brand
2003-03-05 18:02 ` SWSUSP Discontiguous pagedir patch Pavel Machek
2003-03-07 17:14 ` Patrick Mochel
2003-03-07 20:27 ` Pavel Machek
2003-03-09 19:39 ` Benjamin Herrenschmidt
2003-03-09 20:12 ` Pavel Machek
2003-03-10 16:49 ` Patrick Mochel
2003-03-10 19:23 ` Pavel Machek
2003-03-10 19:05 ` Patrick Mochel
2003-03-10 22:17 ` Nigel Cunningham
2003-03-10 23:20 ` Pavel Machek
2003-03-07 20:36 ` Pavel Machek
2003-03-10 16:51 ` Patrick Mochel
2003-03-10 19:12 ` Pavel Machek
2003-03-10 18:59 ` Patrick Mochel
2003-03-07 20:41 ` Pavel Machek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Pine.LNX.4.33.0303021731510.1120-100000@localhost.localdomain \
--to=mochel@osdl.org \
--cc=linux-kernel@vger.kernel.org \
--cc=ncunningham@clear.net.nz \
--cc=pavel@ucw.cz \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).