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

Ingo Molnar
2007-04-19T07:05:52Z

* 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