* Nigel Cunningham <[EMAIL PROTECTED]> 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: CONFIG_SUSPEND2_REPLACE_SWSUSP=y 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 code. 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 up?) 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; +EXPORT_SYMBOL(suspend2_faulted); should be done via the pagefault notifier chain mechanism. Also, all the exports you added should be EXPORT_SYMBOL_GPL(). this: - 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 +#define FREEZER_USERSPACE_FROZEN 1 +#define FREEZER_FULLY_ON 2 should be: +#define FREEZER_OFF 0 +#define FREEZER_USERSPACE_FROZEN 1 +#define FREEZER_FULLY_ON 2 (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.), right? +++ 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. Ingo - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/