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

    * 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
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-04-19 09:09    [W:0.030 / U:29.632 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site