lkml.org 
[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
    From
    Date
    Hi Ingo.

    On Thu, 2007-04-19 at 09:04 +0200, Ingo Molnar wrote:
    > * 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.

    I really would like to get it into Linus' tree but Pavel doesn't want it
    (obviously!) and I haven't got together enough of a case yet to convince
    Andrew. I yet another here's-why-I-think-it-should-be-merged email in
    the works (poor Andrew!) but there are too many other things on my plate
    at the mo.

    > 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?)

    Right. A good portion (~2000 lines) of that is documentation.

    > 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().

    I'll look at that, but I'm not sure if it's a good idea - this is for
    during the atomic copy & restore, when DEBUG_PAGEALLOC is enabled on
    x86. Other things might touch memory in ways we don't want. It's only
    needed for slab pages that get unmapped but not freed.

    As far as the module exports go, I'm not expecting them to get merged. I
    like building Suspend2 as modules (it helps speed the development
    cycle), and see it as potentially useful for embedded but IMO there are
    too many export symbols to make merging that code a possibility. This is
    why they're all in one file rather than sprinkled through the files that
    define the symbols.

    > 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?

    Yeah. When I was recently debugging, I found that cpu hotplugging is
    using something marked __init which is causing the machine to
    spontaneously reboot when cpus are replugged if DEBUG_PAGEALLOC is
    enabled. Haven't had the time to get back to it, and also need some help
    with the approach (what makes the machine reboot in this case instead of
    oopsing, and how do I stop it?).

    > + if(PageHighMem(page))
    > + return 0;
    >
    > coding style.

    Oh. The space missing after the if? Ok.

    > + 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 :)

    Ok.

    > +#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?

    Ok. I've just separated that into it's own file/module, so that will be
    straightforward to do.

    > +++ 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?

    The modifications do three things.

    First, we're seeking to keep the LRU static once while we're suspending.
    I originally sought to achieve that by avoiding entering the vmscan.c
    logic (not as drastic as it sounds - Suspend2 is the only thing
    running!). I think it was Nick who said he'd rather see it the pages
    unlinked and kept safe that way, so now I do that. Oh, as part of this,
    I separated out the code from shrink_inactive_list that returns isolated
    pages, since relink_lru_lists uses it too.

    The other part is (prior to the above) seeking to get in a situation
    where we have enough memory available to do the cycle. I used to use
    shrink_all_zones, but some users with lots of Highmem were finding
    issues that let me to take a more per-zone based approach, hence
    shrink_one_zone.

    The last thing is a patch Rafael recently posted to improve kswapd
    freezing.

    > +++ 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)

    Yeah, I guess mm makes more sense.

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

    Thanks for the feedback. Although I'd like to see Suspend2 merged, I've
    been feeling a bit like it's a lost cause. It's nice to get some
    encouragement.

    Nigel
    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2007-04-19 11:09    [W:4.119 / U:0.352 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site