lkml.org 
[lkml]   [2011]   [Oct]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: lsusd - The Linux SUSpend Daemon
    On Fri, 21 Oct 2011 22:00:13 -0400 (EDT) Alan Stern
    <stern@rowland.harvard.edu> wrote:

    > On Sat, 22 Oct 2011, NeilBrown wrote:
    >
    > > > > It uses files in /var/run/suspend for all communication.
    > > >
    > > > I'm not so keen on using files for communication. At best, they are
    > > > rather awkward for two-way messaging. If you really want to use them,
    > > > then at least put them on a non-backed filesystem, like something under
    > > > /dev.
    > >
    > > Isn't /var/run a tmpfs filesystem? It should be.
    > > Surely /run is, so in the new world order the files should probably go
    > > there. But that is just a detail.
    >
    > On my Fedora-14 systems there is no /run, and /var/run is a regular
    > directory in a regular filesystem.
    >
    > > I like files... I particularly like 'flock' to block suspend. The
    > > rest.... whatever..
    > > With files, you only need a context switch when there is real communication.
    > > With sockets, every message sent must be read so there will be a context
    > > switch.
    > >
    > > Maybe we could do something with futexes...
    >
    > Not easily -- as far as I can tell, futexes enjoy relatively little
    > support. In any case, they provide the same service as a mutex, which
    > means you'd have to build a shared lock on top of them.
    >
    > > > > lsusd does not try to be event-loop based because:
    > > > > - /sys/power/wakeup_count is not pollable. This could probably be
    > > > > 'fixed' but I want code to work with today's kernel. It will probably
    > > >
    > > > Why does this matter?
    > >
    > > In my mind an event based program should never block. Every action should be
    > > non-blocking and only taken when 'poll' says it can.
    > > Reading /sys/power/wakeup_count can be read non-blocking, but you cannot find
    > > out when it is sensible to try to read it again. So it doesn't fit.
    >
    > There shouldn't be any trouble about making wakeup_count pollable. It
    > also would need to respect nonblocking reads, which it currently does
    > not do.

    Hmm.. you are correct. I wonder why I thought it did support non-blocking
    reads...
    I guess it was the code for handling an interrupted system call.

    I feel a bit uncomfortable with the idea of sysfs files that block but I
    don't think I can convincingly argue against it.
    A non-blocking flag could be passed in, but it would be a very messy change -
    lots of function call signatures changing needlessly: we would need a flag
    to the 'show' method ... or add a 'show_nonblock' method which would also be
    ugly.


    But I think there is a need to block - if there is an in-progress event then
    it must be possible to wait for it to complete as it may not be visible to
    userspace until then.
    We could easily enable 'poll' for wakeup_count and then make it always
    non-blocking, but I'm not really sure I want to require programs to use poll,
    only to allow them. And without using poll there is no way to wait.

    As wakeup_count really has to be single-access we could possibly fudge
    something by remembering the last value read (like we remember the last value
    written).

    - if the current count is different from the last value read, then return
    it even if there are in-progress events.
    - if the current count is the same as the last value read, then block until
    there are no in-progress events and return the new value.
    - enable sysfs_poll on wakeup_count by calling sysfs_notify_dirent at the
    end of wakeup_source_deactivated .... or calling something in
    kernel/power/main.c which calls that. However we would need to make
    sysfs_notify_dirent a lot lighter weight first. Maybe I should do that.

    Then a process that uses 'poll' could avoid reading wakeup_count except when
    it has changed, and then it won't block. And a process that doesn't use poll
    can block by simply reading twice - either explicitly or by going around a
    read then write and it fails
    loop a second time.

    I'm not sure I'm completely comfortable with that, but it is the best I could
    come up with.


    >
    > At the worst, you could always have a separate thread to read
    > wakeup_count.

    Maybe. I'm tempted simply not to worry about the short delay after all, but
    the moment I do that, someone will use a wakeup_source to disable suspend
    for a longer period of time (just like I suggested) and suddenly it won't be
    a short delay after all.

    >
    > > > > - I cannot get an event notification when a lock is removed from a
    > > > > file. :-( And I think locks are an excellent light-weight
    > > > > mechanism for blocking suspend.
    > > >
    > > > Except for this one drawback. Socket connections are superior in that
    > > > regard.
    > >
    > > I'm very happy for someone else write an all-socket based daemon.
    >
    > Hmmm... Maybe I'll take you up on that.
    >
    >
    > > > > lsused will send a 'S' message to the client and await an 'R' reply
    > > > > (S == suspend, R == ready). When all replies are in, lsused will
    > > > > allow the suspend to complete. When it does (or aborts), it will send
    > > > > 'A' (awake) to those clients to which it sent 'S'.
    > > >
    > > > But not to the client which failed to send an 'R'?
    > >
    > > Every client must send an R before suspend can continue.
    >
    > I was referring to the case where you abort before receiving an 'R'.
    > The current suspend attempt will fail, but then what happens during the
    > next attempt?

    There is no situation in which I abort before receiving an 'R'.
    I wait until all 'R's are in until I even check if an abort was requested.


    NeilBrown



    Sample untested patch to allow non-blocking reads and poll on wakeup_count.

    diff --git a/drivers/base/power/wakeup.c b/drivers/base/power/wakeup.c
    index 84f7c7d..54543ba 100644
    --- a/drivers/base/power/wakeup.c
    +++ b/drivers/base/power/wakeup.c
    @@ -44,6 +44,8 @@ static void split_counters(unsigned int *cnt, unsigned int *inpr)

    /* A preserved old value of the events counter. */
    static unsigned int saved_count;
    +/* Record last value that was read from events counter. */
    +static unsigned int last_read_count;

    static DEFINE_SPINLOCK(events_lock);

    @@ -410,6 +412,7 @@ static void wakeup_source_deactivate(struct wakeup_source *ws)
    {
    ktime_t duration;
    ktime_t now;
    + unsigned int comb;

    ws->relax_count++;
    /*
    @@ -440,7 +443,10 @@ static void wakeup_source_deactivate(struct wakeup_source *ws)
    * Increment the counter of registered wakeup events and decrement the
    * couter of wakeup events in progress simultaneously.
    */
    - atomic_add(MAX_IN_PROGRESS, &combined_event_count);
    + comb = atomic_add_return(MAX_IN_PROGRESS, &combined_event_count);
    +
    + if ((comb >> IN_PROGRESS_BITS) == last_read_count + 1)
    + wakeup_count_changed();
    }

    /**
    @@ -624,15 +630,24 @@ bool pm_get_wakeup_count(unsigned int *count)

    for (;;) {
    split_counters(&cnt, &inpr);
    - if (inpr == 0 || signal_pending(current))
    + if (inpr == 0 || cnt != last_read_count)
    break;
    + if (signal_pending(current))
    + return false;
    pm_wakeup_update_hit_counts();
    schedule_timeout_interruptible(msecs_to_jiffies(TIMEOUT));
    }

    + last_read_count = cnt;
    + /* If cnt has just changed, then last_read_count will be a bit
    + * old, so we won't block on the next read, only the one after.
    + * However this ensures wakeup_source_deactivate doesn't
    + * miss out on calling wakeup_count_changed() on a change.
    + */
    + smp_wmb();
    split_counters(&cnt, &inpr);
    *count = cnt;
    - return !inpr;
    + return true;
    }

    /**
    diff --git a/fs/sysfs/file.c b/fs/sysfs/file.c
    index 1ad8c93..41361ba 100644
    --- a/fs/sysfs/file.c
    +++ b/fs/sysfs/file.c
    @@ -444,18 +444,19 @@ static unsigned int sysfs_poll(struct file *filp, poll_table *wait)

    void sysfs_notify_dirent(struct sysfs_dirent *sd)
    {
    - struct sysfs_open_dirent *od;
    - unsigned long flags;
    + if (sd->s_attr.open) {
    + struct sysfs_open_dirent *od;
    + unsigned long flags;
    + spin_lock_irqsave(&sysfs_open_dirent_lock, flags);

    - spin_lock_irqsave(&sysfs_open_dirent_lock, flags);
    + od = sd->s_attr.open;
    + if (od) {
    + atomic_inc(&od->event);
    + wake_up_interruptible(&od->poll);
    + }

    - od = sd->s_attr.open;
    - if (od) {
    - atomic_inc(&od->event);
    - wake_up_interruptible(&od->poll);
    + spin_unlock_irqrestore(&sysfs_open_dirent_lock, flags);
    }
    -
    - spin_unlock_irqrestore(&sysfs_open_dirent_lock, flags);
    }
    EXPORT_SYMBOL_GPL(sysfs_notify_dirent);

    diff --git a/include/linux/suspend.h b/include/linux/suspend.h
    index 6bbcef2..e42b7f9 100644
    --- a/include/linux/suspend.h
    +++ b/include/linux/suspend.h
    @@ -296,6 +296,7 @@ extern bool events_check_enabled;
    extern bool pm_wakeup_pending(void);
    extern bool pm_get_wakeup_count(unsigned int *count);
    extern bool pm_save_wakeup_count(unsigned int count);
    +extern void wakeup_count_changed(void);
    #else /* !CONFIG_PM_SLEEP */

    static inline int register_pm_notifier(struct notifier_block *nb)
    diff --git a/kernel/power/main.c b/kernel/power/main.c
    index 6c601f8..6b3cd80 100644
    --- a/kernel/power/main.c
    +++ b/kernel/power/main.c
    @@ -231,6 +231,8 @@ power_attr(state);
    * are any wakeup events detected after 'wakeup_count' was written to.
    */

    +struct sysfs_dirent *wakeup_count_dirent;
    +
    static ssize_t wakeup_count_show(struct kobject *kobj,
    struct kobj_attribute *attr,
    char *buf)
    @@ -253,6 +255,12 @@ static ssize_t wakeup_count_store(struct kobject *kobj,
    return -EINVAL;
    }

    +void wakeup_count_change(void)
    +{
    + if (wakeup_count_dirent)
    + sysfs_notify_dirent(wakeup_count_dirent);
    +}
    +
    power_attr(wakeup_count);
    #endif /* CONFIG_PM_SLEEP */

    @@ -342,7 +350,11 @@ static int __init pm_init(void)
    power_kobj = kobject_create_and_add("power", NULL);
    if (!power_kobj)
    return -ENOMEM;
    - return sysfs_create_group(power_kobj, &attr_group);
    + error = sysfs_create_group(power_kobj, &attr_group);
    +#ifdef CONFIG_PM_SLEEP
    + wakeup_count_dirent = sysfs_get_dirent(power_kobj->sd, NULL, "wakeup_count");
    +#endif
    + return error;
    }

    core_initcall(pm_init);


    [unhandled content-type:application/pgp-signature]
    \
     
     \ /
      Last update: 2011-10-23 10:25    [W:0.040 / U:153.748 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site