mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Frederic Weisbecker <fweisbec@gmail.com>
To: LKML <linux-kernel@vger.kernel.org>
Cc: Frederic Weisbecker <fweisbec@gmail.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Oleg Nesterov <oleg@redhat.com>,
	Kosaki Motohiro <kosaki.motohiro@jp.fujitsu.com>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: [RFC PATCH 01/10] posix-timers: Remove dead thread posix cpu timers caching
Date: Sat, 12 Oct 2013 21:06:17 +0200	[thread overview]
Message-ID: <1381604786-21293-2-git-send-email-fweisbec@gmail.com> (raw)
In-Reply-To: <1381604786-21293-1-git-send-email-fweisbec@gmail.com>

When a task is exiting or has exited, its posix cpu timers
don't tick anymore and won't elapse further. It's too late
for them to expire.

So any further call to timer_gettime() on these timers will
return the same remaining expiry time.

The current code optimize this by caching the remaining delta
and storing it where we use to save the absolute expiration time.
This way, the future calls to timer_gettime() won't need to
compute the difference between the absolute expiration time and
the current time anymore.

Now this optimization doesn't seem to bring much value. Computing
the timer remaining delta is not very costly. Fetching the timer
value OTOH can be costly in two ways:

* CPUCLOCK_SCHED read requires to lock the target's rq. But some
optimizations are on the way to make task_sched_runtime() not holding
the rq lock of a non-running target.

* CPUCLOCK_VIRT/CPUCLOCK_PROF read simply consist in fetching
current->utime/current->stime except when the system uses full
dynticks cputime accounting. The latter requires a per task lock
in order to correctly compute user and system time. But once the
target is dead, this lock shouldn't be contended anyway.

Also some recent bugfixes (see a0b2062b0904ef07944c4a6e4d0f88ee44f1e9f2
"posix_timers: fix racy timer delta caching on task exit") have shown
long standing issues on posix cpu timers that were gross enough
to deduce that fetching a timer on a dead target is probably not a very
common operation.

Given that this caching complicates the code significantly for
few wins, let's remove it on single thread timers.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Oleg Nesterov <oleg@redhat.com>
Cc: Kosaki Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 kernel/posix-cpu-timers.c | 34 ++++++++++++++++------------------
 1 file changed, 16 insertions(+), 18 deletions(-)

diff --git a/kernel/posix-cpu-timers.c b/kernel/posix-cpu-timers.c
index c7f31aa..b803265 100644
--- a/kernel/posix-cpu-timers.c
+++ b/kernel/posix-cpu-timers.c
@@ -787,7 +787,6 @@ static void posix_cpu_timer_get(struct k_itimer *timer, struct itimerspec *itp)
 {
 	unsigned long long now;
 	struct task_struct *p = timer->it.cpu.task;
-	int clear_dead;
 
 	/*
 	 * Easy part: convert the reload time.
@@ -801,6 +800,7 @@ static void posix_cpu_timer_get(struct k_itimer *timer, struct itimerspec *itp)
 	}
 
 	if (unlikely(p == NULL)) {
+		WARN_ON_ONCE(CPUCLOCK_PERTHREAD(timer->it_clock));
 		/*
 		 * This task already died and the timer will never fire.
 		 * In this case, expires is actually the dead value.
@@ -816,7 +816,6 @@ static void posix_cpu_timer_get(struct k_itimer *timer, struct itimerspec *itp)
 	 */
 	if (CPUCLOCK_PERTHREAD(timer->it_clock)) {
 		cpu_clock_sample(timer->it_clock, p, &now);
-		clear_dead = p->exit_state;
 	} else {
 		read_lock(&tasklist_lock);
 		if (unlikely(p->sighand == NULL)) {
@@ -832,22 +831,20 @@ static void posix_cpu_timer_get(struct k_itimer *timer, struct itimerspec *itp)
 			goto dead;
 		} else {
 			cpu_timer_sample_group(timer->it_clock, p, &now);
-			clear_dead = (unlikely(p->exit_state) &&
-				      thread_group_empty(p));
+			if (unlikely(p->exit_state) && thread_group_empty(p)) {
+				read_unlock(&tasklist_lock);
+				/*
+				 * We've noticed that the thread is dead, but
+				 * not yet reaped.  Take this opportunity to
+				 * drop our task ref.
+				 */
+				clear_dead_task(timer, now);
+				goto dead;
+			}
 		}
 		read_unlock(&tasklist_lock);
 	}
 
-	if (unlikely(clear_dead)) {
-		/*
-		 * We've noticed that the thread is dead, but
-		 * not yet reaped.  Take this opportunity to
-		 * drop our task ref.
-		 */
-		clear_dead_task(timer, now);
-		goto dead;
-	}
-
 	if (now < timer->it.cpu.expires) {
 		sample_to_timespec(timer->it_clock,
 				   timer->it.cpu.expires - now,
@@ -1062,11 +1059,13 @@ void posix_cpu_timer_schedule(struct k_itimer *timer)
 	struct task_struct *p = timer->it.cpu.task;
 	unsigned long long now;
 
-	if (unlikely(p == NULL))
+	if (unlikely(p == NULL)) {
+		WARN_ON_ONCE(CPUCLOCK_PERTHREAD(timer->it_clock));
 		/*
 		 * The task was cleaned up already, no future firings.
 		 */
 		goto out;
+	}
 
 	/*
 	 * Fetch the current sample and update the timer's expiry time.
@@ -1074,10 +1073,9 @@ void posix_cpu_timer_schedule(struct k_itimer *timer)
 	if (CPUCLOCK_PERTHREAD(timer->it_clock)) {
 		cpu_clock_sample(timer->it_clock, p, &now);
 		bump_cpu_timer(timer, now);
-		if (unlikely(p->exit_state)) {
-			clear_dead_task(timer, now);
+		if (unlikely(p->exit_state))
 			goto out;
-		}
+
 		read_lock(&tasklist_lock); /* arm_timer needs it.  */
 		spin_lock(&p->sighand->siglock);
 	} else {
-- 
1.8.3.1


  reply	other threads:[~2013-10-12 19:06 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-10-12 19:06 posix-timers: Various cleanups Frederic Weisbecker
2013-10-12 19:06 ` Frederic Weisbecker [this message]
2013-10-12 19:06 ` [RFC PATCH 02/10] posix-timers: Remove dead process posix cpu timers caching Frederic Weisbecker
2013-10-12 19:06 ` [RFC PATCH 03/10] posix-timers: Cleanup reaped target handling Frederic Weisbecker
2013-10-12 19:06 ` [RFC PATCH 04/10] posix-timers: Remove dead task special case Frederic Weisbecker
2013-10-12 19:06 ` [RFC PATCH 05/10] posix-timers: Remove useless clock sample on timers cleanup Frederic Weisbecker
2013-10-12 19:06 ` [RFC PATCH 06/10] posix-timers: Consolidate posix_cpu_clock_get() Frederic Weisbecker
2013-10-12 19:06 ` [RFC PATCH 07/10] posix-timers: Use sighand lock instead of tasklist_lock for task clock sample Frederic Weisbecker
2013-10-12 19:06 ` [RFC PATCH 08/10] posix-timers: Use sighand lock instead of tasklist_lock on timer deletion Frederic Weisbecker
2013-10-12 19:06 ` [RFC PATCH 09/10] posix-timers: Remove remaining uses of tasklist_lock Frederic Weisbecker
2013-10-12 19:06 ` [RFC PATCH 10/10] posix-timers: Convert abuses of BUG_ON to WARN_ON Frederic Weisbecker

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1381604786-21293-2-git-send-email-fweisbec@gmail.com \
    --to=fweisbec@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=kosaki.motohiro@jp.fujitsu.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=oleg@redhat.com \
    --cc=peterz@infradead.org \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®