Re: [PATCH/RFC] Add support to resume swsusp from initrd

From: Pavel Machek (pavel_at_suse.cz)
Date: 12/07/04

  • Next message: Per Jessen: "Re: 2.4.28 - kswapd excessive cpu usage under heavy IO" 
    Date:	Tue, 7 Dec 2004 10:44:39 +0100
To: Matthew Garrett <mjg59@srcf.ucam.org>

    Ahoj!

    > Ok, how does this one look? (applies on top of the __init patch from
    > last time)

    It looks way better than last time :-).

    > resume_device has been renamed to swsusp_resume_device to avoid
    > confusion with the resume_device function in drivers/power. If a resume
    > device has been set, it's assumed that userspace has been started. If
    > not, it behaves as before. name_to_dev_t is only called if userspace
    > hasn't been started - otherwise, we depend on the values provided by
    > userspace being correct. If userspace has been started, we freeze tasks
    > and free memory before starting the resume. If this looks ok, I'll merge
    > it to your changes after 2.6.10.

    Ugh, we probably should always stop tasks and always free memory. To
    get code paths tested etc.

    > @@ -28,7 +29,7 @@
    > extern int swsusp_read(void);
    > extern int swsusp_resume(void);
    > extern int swsusp_free(void);
    > -
    > +extern dev_t swsusp_resume_device;
    >
    > static int noresume = 0;
    > char resume_file[256] = CONFIG_PM_STD_PARTITION;

    Move it to include/linux/suspend.h

    > @@ -223,6 +224,18 @@
    >
    > pr_debug("PM: Reading pmdisk image.\n");
    >
    > + if (swsusp_resume_device) {
    > + /* We want to be really sure that userspace isn't touching
    > + anything at this point... */
    > + if (freeze_processes()) {
    > + goto Done;
    > + }
    > +
    > + /* And then make sure that we have enough memory to do the
    > + resume */
    > + free_some_memory();
    > + }
    > +
    > if ((error = swsusp_read()))
    > goto Done;
    >

    This should not be conditional.

    > @@ -327,8 +340,42 @@
    >
    > power_attr(disk);
    >
    > +static ssize_t resume_show(struct subsystem * subsys, char * buf) {
    > + return sprintf(buf,"%d:%d\n", MAJOR(swsusp_resume_device),
    > + MINOR(swsusp_resume_device));
    > +}
    > +
    > +static ssize_t resume_store(struct subsystem * s, const char * buf, size_t n)
    > +{
    > + int error = 0;
    > + int len;
    > + char *p;
    > + unsigned maj, min;
    > + dev_t (res);

    Why the ()s?

    > + p = memchr(buf, '\n', n);
    > + len = p ? p - buf : n;
    > +
    > + if (sscanf(buf, "%u:%u", &maj, &min) == 2) {
    > + res = MKDEV(maj, min);
    > + if (maj == MAJOR(res) && min == MINOR(res)) {

    You mkdev, than test that MKDEV worked? Could you add a comment why
    its needed?

    I'd write it as "if (sscanf ... !=2) return -EINVAL". If (MKDEV is
    broken) return -EINVAL, but Andrew would hate me then.

    > @@ -1220,13 +1220,16 @@
    > {
    > int error;
    >
    > - if (!strlen(resume_file))
    > - return -ENOENT;
    > + if (!swsusp_resume_device) {
    > + if (!strlen(resume_file))
    > + return -ENOENT;
    >
    > - resume_device = name_to_dev_t(resume_file);
    > - pr_debug("swsusp: Resume From Partition: %s\n", resume_file);
    > + swsusp_resume_device = name_to_dev_t(resume_file);
    > + pr_debug("swsusp: Resume From Partition: %s\n", resume_file);
    > + }

    So... if userspace echos "0:0" into resume file, you attempt to do the
    resume, and oops the kernel? Why not doing name_to_dev_t,
    unconditionally, while doing resume_setup? And probably kill
    CONFIG_PM_STD_PARTITION; I do not like idea of kernel automagically
    trying to resume without anything on command line anyway.

                                                                    Pavel 

    -- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!
-
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/
  • Next message: Per Jessen: "Re: 2.4.28 - kswapd excessive cpu usage under heavy IO"

    Relevant Pages

    • Re: [RFC] kernel facilities for cache prefetching
      ... at boot time, while we still have enough free memory? ... though may be unnecessary once we can rely on userspace ... pre-caching tools to do it in a more accurate and efficient way. ...
      (Linux-Kernel)
    • Re: [PATCH 2/3] swsusp: move snapshot-handling functions to snapshot.c
      ... >> userspace relocates it ... > process cannot freeze the other tasks, suspend devices etc., so it ... certainly need some way to free memory, ... send the line "unsubscribe linux-kernel" in ...
      (Linux-Kernel)
    • Re: Conveying memory pressure to userspace?
      ... is trying its best to to free memory by for instance dropping, ... possibly more valuable caches, userspace is left blissfully unaware. ... cpusets export a notion of memory pressure to user space. ...
      (Linux-Kernel)
    • Re: [1/1][PATCH] nproc v2: netlink access to /proc information
      ... > The presumed wrong assumptions underlying broken tools of the future ... > making it easy to write correct applications (or in fixing broken apps ... knowledge of the CONFIG_MMU usage models and/or whatever userspace ... send the line "unsubscribe linux-kernel" in ...
      (Linux-Kernel)
    • Re: User space out of memory approach
      ... > userspace provided candidate list. ... > instead of fixing the real problem is a real interesting engineering ... that after the source of trouble is fixed it is worth to ... send the line "unsubscribe linux-kernel" in ...
      (Linux-Kernel)