Re: [Suspend2-devel] Re: CFS and suspend2: hang in atomic copy

[Ingo Molnar <mingo@elte.hu>]

* Nigel Cunningham <nigel@nigel.suspend2.net> 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