From patchwork Fri Sep 28 17:44:29 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Viresh Kumar X-Patchwork-Id: 11856 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 B968623EFB for ; Fri, 28 Sep 2012 17:44:53 +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 48CF7A18061 for ; Fri, 28 Sep 2012 17:44:53 +0000 (UTC) Received: by ieje10 with SMTP id e10so7519323iej.11 for ; Fri, 28 Sep 2012 10:44:52 -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:x-gm-message-state; bh=B1KsH2tJ/6wu+IZRDS1Yw5lLIB92rKPSsnqaenE81IE=; b=j+haiF+4iKRcNOnXuwOQEdkCCDz3HxwavtK+RT+hpbp1iPl4/reTn+igplsWLmSLBb J72dxxZ5WfpFs05O/0AEQmH7zU9k+Tmyr377eMuZV5XKGSbtRKXjb4jKNHE/cTKejvjg zL656Zoj/JJy5sJ/Tgu9jG4/wJqxINTshfg1TPzI/wAjD6AXBbXL+f+NFoVakZI4IqN4 0GqDUOUGzyJEhKrsll92Xsfr0bP6v9IQMJCm6a5ptk+I6MMvt8CDRGyK8C9z0tsIFs4b V5fP6fHiJB3AQhk464Egur0/2KNTNpVyLMhxxDjh2KtICP1AHjE4FkpeEXiF6Qy3aykR 5GjQ== Received: by 10.50.150.198 with SMTP id uk6mr2362573igb.43.1348854290221; Fri, 28 Sep 2012 10:44:50 -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 ex8csp484997igc; Fri, 28 Sep 2012 10:44:44 -0700 (PDT) Received: by 10.50.85.226 with SMTP id k2mr2320016igz.61.1348854283735; Fri, 28 Sep 2012 10:44:43 -0700 (PDT) Received: from mail-pb0-f50.google.com (mail-pb0-f50.google.com [209.85.160.50]) by mx.google.com with ESMTPS id pe8si11310969pbc.160.2012.09.28.10.44.43 (version=TLSv1/SSLv3 cipher=OTHER); Fri, 28 Sep 2012 10:44:43 -0700 (PDT) Received-SPF: neutral (google.com: 209.85.160.50 is neither permitted nor denied by best guess record for domain of viresh.kumar@linaro.org) client-ip=209.85.160.50; Authentication-Results: mx.google.com; spf=neutral (google.com: 209.85.160.50 is neither permitted nor denied by best guess record for domain of viresh.kumar@linaro.org) smtp.mail=viresh.kumar@linaro.org Received: by pbcmd4 with SMTP id md4so3150577pbc.37 for ; Fri, 28 Sep 2012 10:44:43 -0700 (PDT) Received: by 10.68.209.136 with SMTP id mm8mr21932175pbc.146.1348854282930; Fri, 28 Sep 2012 10:44:42 -0700 (PDT) Received: from localhost ([122.178.194.119]) by mx.google.com with ESMTPS id iq3sm5916059pbc.5.2012.09.28.10.44.37 (version=TLSv1/SSLv3 cipher=OTHER); Fri, 28 Sep 2012 10:44:42 -0700 (PDT) From: Viresh Kumar To: tglx@linutronix.de, linux-kernel@vger.kernel.org Cc: pjt@google.com, paul.mckenney@linaro.org, tj@kernel.org, suresh.b.siddha@intel.com, venki@google.com, mingo@redhat.com, peterz@infradead.org, robin.randhawa@arm.com, Steve.Bannister@arm.com, Arvind.Chauhan@arm.com, amit.kucheria@linaro.org, vincent.guittot@linaro.org, linaro-dev@lists.linaro.org, patches@linaro.org, Viresh Kumar Subject: [RFC] timer: Migrate running timer Date: Fri, 28 Sep 2012 23:14:29 +0530 Message-Id: <72a0df374d6e1da3b55071030e38354f7d64b604.1348854133.git.viresh.kumar@linaro.org> X-Mailer: git-send-email 1.7.12.rc2.18.g61b472e X-Gm-Message-State: ALoCoQkpigO3ERSo16bDQCV96YBdIbbyXyX/ZUU6h2Uj3lF8uKA+pJWnnOKMoqWpVVNDEw1vO0lr Hi Thomas, I haven't tested it much till now. I am sending this patch just to check if the initial idea looks fine to you guys or not. Till now, we weren't migrating a running timer because with migration del_timer_sync() can't detect that the timer's handler yet has not finished. Now, when can we actually to reach to the code (inside __mod_timer()) where base->running_timer == timer ? i.e. We are trying to migrate current timer I can see only following case: - Timer re-armed itself. i.e. Currently we are running interrupt handler of a timer and it rearmed itself from there. At this time user might have called del_timer_sync() or not. If not, then there is no harm in re-arming the timer? Now, when somebody tries to delete a timer, obviously he doesn't want to run it any more for now. So, why should we ever re-arm a timer, which is scheduled for deletion? This patch tries to fix "migration of running timer", assuming above theory is correct :) So, now when we get a call to del_timer_sync(), we will mark it scheduled for deletion in an additional variable. This would be checked whenever we try to modify/arm it again. If it was currently scheduled for deletion, we must not modify/arm it. And so, whenever we reach to the situation where: base->running_timer == timer We are sure, nobody is waiting in del_timer_sync(). We will clear this flag as soon as the timer is deleted, so that it can be started again after deleting it successfully. Waiting for initial comments on it. Signed-off-by: Viresh Kumar --- include/linux/timer.h | 1 + kernel/timer.c | 58 +++++++++++++++++++++++++++++---------------------- 2 files changed, 34 insertions(+), 25 deletions(-) diff --git a/include/linux/timer.h b/include/linux/timer.h index 8c5a197..ea36ce9 100644 --- a/include/linux/timer.h +++ b/include/linux/timer.h @@ -22,6 +22,7 @@ struct timer_list { unsigned long data; int slack; + int sched_del; #ifdef CONFIG_TIMER_STATS int start_pid; diff --git a/kernel/timer.c b/kernel/timer.c index 1cf8a91..536e7a3 100644 --- a/kernel/timer.c +++ b/kernel/timer.c @@ -620,6 +620,7 @@ static void do_init_timer(struct timer_list *timer, unsigned int flags, timer->entry.next = NULL; timer->base = (void *)((unsigned long)base | flags); timer->slack = -1; + timer->sched_del = 0; #ifdef CONFIG_TIMER_STATS timer->start_site = NULL; timer->start_pid = -1; @@ -727,38 +728,35 @@ __mod_timer(struct timer_list *timer, unsigned long expires, base = lock_timer_base(timer, &flags); + if (timer->sched_del) { + /* Don't schedule it again, as it is getting deleted */ + ret = -EBUSY; + goto out_unlock; + } + ret = detach_if_pending(timer, base, false); if (!ret && pending_only) goto out_unlock; debug_activate(timer, expires); - /* - * Should we try to migrate timer? - * However we can't change timer's base while it is running, otherwise - * del_timer_sync() can't detect that the timer's handler yet has not - * finished. This also guarantees that the timer is serialized wrt - * itself. - */ - if (likely(base->running_timer != timer)) { #if defined(CONFIG_NO_HZ) && defined(CONFIG_SMP) - if (!pinned && get_sysctl_timer_migration()) - cpu = get_nohz_timer_target(); - else - cpu = smp_processor_id(); -#else + if (!pinned && get_sysctl_timer_migration()) + cpu = get_nohz_timer_target(); + else cpu = smp_processor_id(); +#else + cpu = smp_processor_id(); #endif - new_base = per_cpu(tvec_bases, cpu); - - if (base != new_base) { - /* See the comment in lock_timer_base() */ - timer_set_base(timer, NULL); - spin_unlock(&base->lock); - base = new_base; - spin_lock(&base->lock); - timer_set_base(timer, base); - } + new_base = per_cpu(tvec_bases, cpu); + + if (base != new_base) { + /* See the comment in lock_timer_base() */ + timer_set_base(timer, NULL); + spin_unlock(&base->lock); + base = new_base; + spin_lock(&base->lock); + timer_set_base(timer, base); } timer->expires = expires; @@ -1037,9 +1035,11 @@ EXPORT_SYMBOL(try_to_del_timer_sync); */ int del_timer_sync(struct timer_list *timer) { -#ifdef CONFIG_LOCKDEP + struct tvec_base *base; unsigned long flags; +#ifdef CONFIG_LOCKDEP + /* * If lockdep gives a backtrace here, please reference * the synchronization rules above. @@ -1049,6 +1049,12 @@ int del_timer_sync(struct timer_list *timer) lock_map_release(&timer->lockdep_map); local_irq_restore(flags); #endif + + /* Timer is scheduled for deletion, don't let it re-arm itself */ + base = lock_timer_base(timer, &flags); + timer->sched_del = 1; + spin_unlock_irqrestore(&base->lock, flags); + /* * don't use it in hardirq context, because it * could lead to deadlock. @@ -1056,8 +1062,10 @@ int del_timer_sync(struct timer_list *timer) WARN_ON(in_irq() && !tbase_get_irqsafe(timer->base)); for (;;) { int ret = try_to_del_timer_sync(timer); - if (ret >= 0) + if (ret >= 0) { + timer->sched_del = 0; return ret; + } cpu_relax(); } }