lkml.org 
[lkml]   [2011]   [Jan]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    SubjectRe: [PATCH 1/3] PM / Wakeup: Add missing memory barriers
    Date
    On Wednesday, January 26, 2011, Alan Stern wrote:
    > On Tue, 25 Jan 2011, Rafael J. Wysocki wrote:
    >
    > > From: Rafael J. Wysocki <rjw@sisk.pl>
    > >
    > > The memory barrier in wakeup_source_deactivate() is supposed to
    > > prevent the callers of pm_wakeup_pending() and pm_get_wakeup_count()
    > > from seeing the new value of events_in_progress (0, in particular)
    > > and the old value of event_count at the same time. However, if
    > > wakeup_source_deactivate() is executed by CPU0 and, for instance,
    > > pm_wakeup_pending() is executed by CPU1, where both processors can
    > > reorder operations, the memory barrier in wakeup_source_deactivate()
    > > doesn't affect CPU1 which can reorder reads. In that case CPU1 may
    > > very well decide to fetch event_count before it's modified and
    > > events_in_progress after it's been updated, so pm_wakeup_pending()
    > > may fail to detect a wakeup event. This issue can be addressed by
    > > adding a read memory barrier in pm_wakeup_pending() that will enforce
    > > events_in_progress to be read before event_count.
    > >
    > > For similar reason, a read memory barrier should be added to
    > > pm_get_wakeup_count().
    >
    > How come this is implemented using memory barriers rather than a lock?
    > Is it because this is potentially a fairly hot path?
    >
    > New memory barriers are supposed to have comments present in the code,
    > explaining why they are needed.
    >
    > Ideally you could do away with the need for synchronization entirely.
    > For example, events_in_progress and event_count could be stored as two
    > 16-bit values stuffed into a single atomic variable. Then they could
    > both be read or updated simultaneously.

    OK, the patch below appears to work for me. Can you have a look at it, please?

    Rafael


    ---
    drivers/base/power/wakeup.c | 82 +++++++++++++++++++++++++++++++-------------
    1 file changed, 58 insertions(+), 24 deletions(-)

    Index: linux-2.6/drivers/base/power/wakeup.c
    ===================================================================
    --- linux-2.6.orig/drivers/base/power/wakeup.c
    +++ linux-2.6/drivers/base/power/wakeup.c
    @@ -24,12 +24,48 @@
    */
    bool events_check_enabled;

    -/* The counter of registered wakeup events. */
    -static atomic_t event_count = ATOMIC_INIT(0);
    -/* A preserved old value of event_count. */
    +#define EVENT_COUNT_BITS (sizeof(atomic_t) * 4)
    +#define MAX_EVENT_COUNT ((1 << EVENT_COUNT_BITS) - 1)
    +
    +/* Combined counters of registered wakeup events and events in progress. */
    +static atomic_t combined_event_count = ATOMIC_INIT(0);
    +
    +static unsigned int split_counters(unsigned int *inpr, unsigned int *cnt)
    +{
    + unsigned int comb = atomic_read(&combined_event_count);
    +
    + *inpr = (comb >> EVENT_COUNT_BITS);
    + *cnt = comb & MAX_EVENT_COUNT;
    + return comb;
    +}
    +
    +static unsigned int merge_counters(unsigned int inpr, unsigned int cnt)
    +{
    + return (inpr << EVENT_COUNT_BITS) | cnt;
    +}
    +
    +static void update_events_in_progress(void)
    +{
    + unsigned int cnt, inpr, old, new;
    +
    + do {
    + old = split_counters(&inpr, &cnt);
    + new = merge_counters(inpr + 1, cnt);
    + } while (atomic_cmpxchg(&combined_event_count, old, new) != old);
    +}
    +
    +static void update_counters(void)
    +{
    + unsigned int cnt, inpr, old, new;
    +
    + do {
    + old = split_counters(&inpr, &cnt);
    + new = merge_counters(inpr - 1, cnt + 1);
    + } while (atomic_cmpxchg(&combined_event_count, old, new) != old);
    +}
    +
    +/* A preserved old value of event counter. */
    static unsigned int saved_count;
    -/* The counter of wakeup events being processed. */
    -static atomic_t events_in_progress = ATOMIC_INIT(0);

    static DEFINE_SPINLOCK(events_lock);

    @@ -333,7 +369,7 @@ static void wakeup_source_activate(struc
    ws->timer_expires = jiffies;
    ws->last_time = ktime_get();

    - atomic_inc(&events_in_progress);
    + update_events_in_progress();
    }

    /**
    @@ -419,15 +455,7 @@ static void wakeup_source_deactivate(str

    del_timer(&ws->timer);

    - /*
    - * event_count has to be incremented before events_in_progress is
    - * modified, so that the callers of pm_check_wakeup_events() and
    - * pm_save_wakeup_count() don't see the old value of event_count and
    - * events_in_progress equal to zero at the same time.
    - */
    - atomic_inc(&event_count);
    - smp_mb__before_atomic_dec();
    - atomic_dec(&events_in_progress);
    + update_counters();
    }

    /**
    @@ -582,8 +610,10 @@ bool pm_wakeup_pending(void)

    spin_lock_irqsave(&events_lock, flags);
    if (events_check_enabled) {
    - ret = ((unsigned int)atomic_read(&event_count) != saved_count)
    - || atomic_read(&events_in_progress);
    + unsigned int inpr, cnt;
    +
    + split_counters(&inpr, &cnt);
    + ret = (cnt != saved_count || inpr > 0);
    events_check_enabled = !ret;
    }
    spin_unlock_irqrestore(&events_lock, flags);
    @@ -605,19 +635,22 @@ bool pm_wakeup_pending(void)
    */
    bool pm_get_wakeup_count(unsigned int *count)
    {
    - bool ret;
    + unsigned int inpr, cnt;

    if (capable(CAP_SYS_ADMIN))
    events_check_enabled = false;

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

    - ret = !atomic_read(&events_in_progress);
    - *count = atomic_read(&event_count);
    - return ret;
    + split_counters(&inpr, &cnt);
    + *count = cnt;
    + return !inpr;
    }

    /**
    @@ -631,11 +664,12 @@ bool pm_get_wakeup_count(unsigned int *c
    */
    bool pm_save_wakeup_count(unsigned int count)
    {
    + unsigned int inpr, cnt;
    bool ret = false;

    spin_lock_irq(&events_lock);
    - if (count == (unsigned int)atomic_read(&event_count)
    - && !atomic_read(&events_in_progress)) {
    + split_counters(&inpr, &cnt);
    + if (cnt == count && inpr == 0) {
    saved_count = count;
    events_check_enabled = true;
    ret = true;

    \
     
     \ /
      Last update: 2011-01-27 00:15    [W:0.034 / U:30.740 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site