From patchwork Tue Sep 18 03:12:53 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: John Stultz X-Patchwork-Id: 11489 Return-Path: X-Original-To: patchwork@peony.canonical.com Delivered-To: patchwork@peony.canonical.com Received: from fiordland.canonical.com (fiordland.canonical.com [91.189.94.145]) by peony.canonical.com (Postfix) with ESMTP id 5176E23FA1 for ; Tue, 18 Sep 2012 03:13:13 +0000 (UTC) Received: from mail-ie0-f180.google.com (mail-ie0-f180.google.com [209.85.223.180]) by fiordland.canonical.com (Postfix) with ESMTP id AFDDDA18523 for ; Tue, 18 Sep 2012 03:13:12 +0000 (UTC) Received: by ieak11 with SMTP id k11so10231006iea.11 for ; Mon, 17 Sep 2012 20:13:12 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-forwarded-to:x-forwarded-for:delivered-to:received-spf:from:to:cc :subject:date:message-id:x-mailer:in-reply-to:references :mime-version:content-type:content-transfer-encoding:x-cbid :x-ibm-iss-spamdetectors:x-ibm-iss-detailinfo:x-gm-message-state; bh=LRiEjq0XLo1pRIsYtI8q2TTfBu2R+YGoH/Uywa2SuSo=; b=JzVCfpp1lTbxD1S9yTBWb+y3DVmMZlv4f0xrzqKa7pi2cdetdOpnLWSYF446h2L2ce 09eSc1rINRidkXiqmfooAP8EAFpoOIZGAB9RyVkq1JqXHiQJd+Rswi/XGW8NShTEB+2E d8HLCpJ/YFXxf3QQGVOqSQhZkRdPfZ+Tv3SgCEvyoeQ8Mz3CtbweMRm1bjEGYkV1MsJ6 PU2H6YklyHcBegqSaJxot3lwMVM/Cref05uTSbHiobTEpXph/doTR8i0Bwz1J/yW4qgn BpzdRtwmM0EqTCLpcp4aNe1Yqa6Qbs2+8pxAgmaUbnYe/jYzWKNSyA1a6PNjyyhldLRC JRlg== Received: by 10.50.237.41 with SMTP id uz9mr8891770igc.43.1347937992066; Mon, 17 Sep 2012 20:13:12 -0700 (PDT) X-Forwarded-To: linaro-patchwork@canonical.com X-Forwarded-For: patch@linaro.org linaro-patchwork@canonical.com Delivered-To: patches@linaro.org Received: by 10.50.184.232 with SMTP id ex8csp350834igc; Mon, 17 Sep 2012 20:13:10 -0700 (PDT) Received: by 10.50.185.200 with SMTP id fe8mr12760290igc.0.1347937990214; Mon, 17 Sep 2012 20:13:10 -0700 (PDT) Received: from e3.ny.us.ibm.com (e3.ny.us.ibm.com. [32.97.182.143]) by mx.google.com with ESMTPS id eg10si12973202igc.47.2012.09.17.20.13.09 (version=TLSv1/SSLv3 cipher=OTHER); Mon, 17 Sep 2012 20:13:10 -0700 (PDT) Received-SPF: neutral (google.com: 32.97.182.143 is neither permitted nor denied by best guess record for domain of john.stultz@linaro.org) client-ip=32.97.182.143; Authentication-Results: mx.google.com; spf=neutral (google.com: 32.97.182.143 is neither permitted nor denied by best guess record for domain of john.stultz@linaro.org) smtp.mail=john.stultz@linaro.org Received: from /spool/local by e3.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 17 Sep 2012 23:13:09 -0400 Received: from d01relay02.pok.ibm.com (9.56.227.234) by e3.ny.us.ibm.com (192.168.1.103) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Mon, 17 Sep 2012 23:13:08 -0400 Received: from d01av02.pok.ibm.com (d01av02.pok.ibm.com [9.56.224.216]) by d01relay02.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q8I3D7xk169430; Mon, 17 Sep 2012 23:13:07 -0400 Received: from d01av02.pok.ibm.com (loopback [127.0.0.1]) by d01av02.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q8I3D6h2011813; Tue, 18 Sep 2012 00:13:07 -0300 Received: from kernel-pok.stglabs.ibm.com (kernel.stglabs.ibm.com [9.114.214.19]) by d01av02.pok.ibm.com (8.14.4/8.13.1/NCO v10.0 AVin) with ESMTP id q8I3D65N011794; Tue, 18 Sep 2012 00:13:06 -0300 From: John Stultz To: Linux Kernel Cc: John Stultz , =?UTF-8?q?Arve=20Hj=C3=B8nnev=C3=A5g?= , Colin Cross , Thomas Gleixner Subject: [PATCH 1/3] alarmtimer: Use hrtimer per-alarm instead of per-base Date: Mon, 17 Sep 2012 23:12:53 -0400 Message-Id: <1347937975-34169-2-git-send-email-john.stultz@linaro.org> X-Mailer: git-send-email 1.7.9.5 In-Reply-To: <1347937975-34169-1-git-send-email-john.stultz@linaro.org> References: <1347937975-34169-1-git-send-email-john.stultz@linaro.org> MIME-Version: 1.0 x-cbid: 12091803-8974-0000-0000-00000DEE6AAA X-IBM-ISS-SpamDetectors: X-IBM-ISS-DetailInfo: BY=3.00000294; HX=3.00000196; KW=3.00000007; PH=3.00000001; SC=3.00000007; SDB=6.00175089; UDB=6.00039653; UTC=2012-09-18 03:13:09 X-Gm-Message-State: ALoCoQlCivBqo6iOBFstVwLGnDKALdvfiWGeKyGnwuueooxMA/9cKcFI5O/dMrUz0Wu69vPg5Ux2 Arve Hjønnevåg reported numerous crashes from the "BUG_ON(timer->state != HRTIMER_STATE_CALLBACK)" check in __run_hrtimer after it called alarmtimer_fired. It ends up the alarmtimer code was not properly handling possible failures of hrtimer_try_to_cancel, and because these faulres occur when the underlying base hrtimer is being run, this limits the ability to properly handle modifications to any alarmtimers on that base. Because much of the logic duplicates the hrtimer logic, it seems that we might as well have a per-alarmtimer hrtimer, and avoid the extra complextity of trying to multiplex many alarmtimers off of one hrtimer. Thus this patch moves the hrtimer to the alarm structure and simplifies the management logic. Cc: Arve Hjønnevåg Cc: Colin Cross Cc: Thomas Gleixner Reported-by: Arve Hjønnevåg Signed-off-by: John Stultz --- include/linux/alarmtimer.h | 3 +- kernel/time/alarmtimer.c | 93 +++++++++++++------------------------------- 2 files changed, 28 insertions(+), 68 deletions(-) diff --git a/include/linux/alarmtimer.h b/include/linux/alarmtimer.h index 96c5c24..f122c9f 100644 --- a/include/linux/alarmtimer.h +++ b/include/linux/alarmtimer.h @@ -35,6 +35,7 @@ enum alarmtimer_restart { */ struct alarm { struct timerqueue_node node; + struct hrtimer timer; enum alarmtimer_restart (*function)(struct alarm *, ktime_t now); enum alarmtimer_type type; int state; @@ -43,7 +44,7 @@ struct alarm { void alarm_init(struct alarm *alarm, enum alarmtimer_type type, enum alarmtimer_restart (*function)(struct alarm *, ktime_t)); -void alarm_start(struct alarm *alarm, ktime_t start); +int alarm_start(struct alarm *alarm, ktime_t start); int alarm_try_to_cancel(struct alarm *alarm); int alarm_cancel(struct alarm *alarm); diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c index aa27d39..8c763ac 100644 --- a/kernel/time/alarmtimer.c +++ b/kernel/time/alarmtimer.c @@ -37,7 +37,6 @@ static struct alarm_base { spinlock_t lock; struct timerqueue_head timerqueue; - struct hrtimer timer; ktime_t (*gettime)(void); clockid_t base_clockid; } alarm_bases[ALARM_NUMTYPE]; @@ -130,21 +129,16 @@ static inline void alarmtimer_rtc_timer_init(void) { } * @base: pointer to the base where the timer is being run * @alarm: pointer to alarm being enqueued. * - * Adds alarm to a alarm_base timerqueue and if necessary sets - * an hrtimer to run. + * Adds alarm to a alarm_base timerqueue * * Must hold base->lock when calling. */ static void alarmtimer_enqueue(struct alarm_base *base, struct alarm *alarm) { + WARN_ON(alarm->state & ALARMTIMER_STATE_ENQUEUED); + timerqueue_add(&base->timerqueue, &alarm->node); alarm->state |= ALARMTIMER_STATE_ENQUEUED; - - if (&alarm->node == timerqueue_getnext(&base->timerqueue)) { - hrtimer_try_to_cancel(&base->timer); - hrtimer_start(&base->timer, alarm->node.expires, - HRTIMER_MODE_ABS); - } } /** @@ -152,28 +146,17 @@ static void alarmtimer_enqueue(struct alarm_base *base, struct alarm *alarm) * @base: pointer to the base where the timer is running * @alarm: pointer to alarm being removed * - * Removes alarm to a alarm_base timerqueue and if necessary sets - * a new timer to run. + * Removes alarm to a alarm_base timerqueue * * Must hold base->lock when calling. */ static void alarmtimer_remove(struct alarm_base *base, struct alarm *alarm) { - struct timerqueue_node *next = timerqueue_getnext(&base->timerqueue); - if (!(alarm->state & ALARMTIMER_STATE_ENQUEUED)) return; timerqueue_del(&base->timerqueue, &alarm->node); alarm->state &= ~ALARMTIMER_STATE_ENQUEUED; - - if (next == &alarm->node) { - hrtimer_try_to_cancel(&base->timer); - next = timerqueue_getnext(&base->timerqueue); - if (!next) - return; - hrtimer_start(&base->timer, next->expires, HRTIMER_MODE_ABS); - } } @@ -188,42 +171,23 @@ static void alarmtimer_remove(struct alarm_base *base, struct alarm *alarm) */ static enum hrtimer_restart alarmtimer_fired(struct hrtimer *timer) { - struct alarm_base *base = container_of(timer, struct alarm_base, timer); - struct timerqueue_node *next; + struct alarm *alarm = container_of(timer, struct alarm, timer); + struct alarm_base *base = &alarm_bases[alarm->type]; unsigned long flags; - ktime_t now; int ret = HRTIMER_NORESTART; int restart = ALARMTIMER_NORESTART; spin_lock_irqsave(&base->lock, flags); - now = base->gettime(); - while ((next = timerqueue_getnext(&base->timerqueue))) { - struct alarm *alarm; - ktime_t expired = next->expires; - - if (expired.tv64 > now.tv64) - break; - - alarm = container_of(next, struct alarm, node); - - timerqueue_del(&base->timerqueue, &alarm->node); - alarm->state &= ~ALARMTIMER_STATE_ENQUEUED; - - alarm->state |= ALARMTIMER_STATE_CALLBACK; - spin_unlock_irqrestore(&base->lock, flags); - if (alarm->function) - restart = alarm->function(alarm, now); - spin_lock_irqsave(&base->lock, flags); - alarm->state &= ~ALARMTIMER_STATE_CALLBACK; + alarmtimer_remove(base, alarm); + spin_unlock_irqrestore(&base->lock, flags); - if (restart != ALARMTIMER_NORESTART) { - timerqueue_add(&base->timerqueue, &alarm->node); - alarm->state |= ALARMTIMER_STATE_ENQUEUED; - } - } + if (alarm->function) + restart = alarm->function(alarm, base->gettime()); - if (next) { - hrtimer_set_expires(&base->timer, next->expires); + spin_lock_irqsave(&base->lock, flags); + if (restart != ALARMTIMER_NORESTART) { + hrtimer_set_expires(&alarm->timer, alarm->node.expires); + alarmtimer_enqueue(base, alarm); ret = HRTIMER_RESTART; } spin_unlock_irqrestore(&base->lock, flags); @@ -324,6 +288,9 @@ void alarm_init(struct alarm *alarm, enum alarmtimer_type type, enum alarmtimer_restart (*function)(struct alarm *, ktime_t)) { timerqueue_init(&alarm->node); + hrtimer_init(&alarm->timer, alarm_bases[type].base_clockid, + HRTIMER_MODE_ABS); + alarm->timer.function = alarmtimer_fired; alarm->function = function; alarm->type = type; alarm->state = ALARMTIMER_STATE_INACTIVE; @@ -334,17 +301,19 @@ void alarm_init(struct alarm *alarm, enum alarmtimer_type type, * @alarm: ptr to alarm to set * @start: time to run the alarm */ -void alarm_start(struct alarm *alarm, ktime_t start) +int alarm_start(struct alarm *alarm, ktime_t start) { struct alarm_base *base = &alarm_bases[alarm->type]; unsigned long flags; + int ret; spin_lock_irqsave(&base->lock, flags); - if (alarmtimer_active(alarm)) - alarmtimer_remove(base, alarm); alarm->node.expires = start; alarmtimer_enqueue(base, alarm); + ret = hrtimer_start(&alarm->timer, alarm->node.expires, + HRTIMER_MODE_ABS); spin_unlock_irqrestore(&base->lock, flags); + return ret; } /** @@ -358,18 +327,12 @@ int alarm_try_to_cancel(struct alarm *alarm) { struct alarm_base *base = &alarm_bases[alarm->type]; unsigned long flags; - int ret = -1; - spin_lock_irqsave(&base->lock, flags); - - if (alarmtimer_callback_running(alarm)) - goto out; + int ret; - if (alarmtimer_is_queued(alarm)) { + spin_lock_irqsave(&base->lock, flags); + ret = hrtimer_try_to_cancel(&alarm->timer); + if (ret >= 0) alarmtimer_remove(base, alarm); - ret = 1; - } else - ret = 0; -out: spin_unlock_irqrestore(&base->lock, flags); return ret; } @@ -802,10 +765,6 @@ static int __init alarmtimer_init(void) for (i = 0; i < ALARM_NUMTYPE; i++) { timerqueue_init_head(&alarm_bases[i].timerqueue); spin_lock_init(&alarm_bases[i].lock); - hrtimer_init(&alarm_bases[i].timer, - alarm_bases[i].base_clockid, - HRTIMER_MODE_ABS); - alarm_bases[i].timer.function = alarmtimer_fired; } error = alarmtimer_rtc_interface_setup();