[lkml]   [2007]   [Apr]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [Suspend2-devel] Re: CFS and suspend2: hang in atomic copy

* Nigel Cunningham <> wrote:

> From subsequent emails, I think you already got your answer, but just
> in case...
> Yes, if you enabled "Replace swsusp by default" and you already had it
> set up for getting swsusp to resume. If not, and you're using an
> initrd/ramfs, you'll need to modify it to echo
> > /sys/power/suspend2/do_resume after /sys and /proc are mounted but
> prior to mounting / and so on.

yeah, went with the default suggested by your patch:


and it was pretty easy to set things up. I used "echo disk >
/sys/power/state" to trigger it.

In hindsight it was all pretty straightforward and suspend2 worked
beautifully on an UP and on an SMP system i tried. So in exchange for
suspend2 folks debugging a bug in CFS here's some suspend2 review
feedback ;) Any plans about moving suspend2 to the upstream kernel? It
should be pretty easy for it to co-exist with the current swsuspend

The patch has quite some size:

89 files changed, 16452 insertions(+), 69 deletions(-)

that should obviously be split up into more than a dozen sub-patches,
and fed to lkml with the small ones first. (unless it already is split

i cannot comment on the kernel/power/ bits (they are way too large
anyway), other than that they look pretty clean visually, but the
lowlevel arch and generic kernel bits look sane in detail too, sans a
few mostly trivial cleanliness issues:

+int suspend2_faulted = 0;

should be done via the pagefault notifier chain mechanism. Also, all the
exports you added should be EXPORT_SYMBOL_GPL().


- ClearPageReserved(virt_to_page(addr));
- init_page_count(virt_to_page(addr));
+ //ClearPageReserved(virt_to_page(addr));
+ //init_page_count(virt_to_page(addr));

looks like there's a buglet in there still somewhere?

+ if(PageHighMem(page))
+ return 0;

coding style.

+ BUG_ON( test_suspend_state(SUSPEND_RUNNING) && /* Suspend2, that is */

make this a WARN_ON() or a WARN_ON_ONCE() - that way you have a chance
to even get feedback from users, instead of a 'uhm, X froze' report.

+#define FREEZER_OFF 0

should be:

+#define FREEZER_OFF 0

(you want your reviewers have an pleasant time reading your code :)

+#define NETLINK_SUSPEND2_USERUI 20 /* For suspend2's userui */

IIRC userui was at the center of suspend2 merge flames, right? So you
might want to layer it ontop a less flashy suspend2-core and thus get
90% of your patch upstream?

+++ linux/mm/vmscan.c

the MM impact looks quite nontrivial. But i suspect this is unavoidable,
because you zap portions of the pagecache on the way to disk, so when it
comes back it results in a different pagecache (new lru lists, etc.),

+++ linux/lib/dyn_pageflags.c

shouldnt this be in mm/dyn_pageflags.c? Plus it would be nice to use
some other core kernel user for this infrastructure. (but it's not a
necessity i guess)

but ... again, the patch looks sane all around.

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to
More majordomo info at
Please read the FAQ at

 \ /
  Last update: 2007-04-19 09:09    [W:0.454 / U:24.212 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site