LKML Archive on
help / color / mirror / Atom feed
From: Thomas Gleixner <>
To: Eric Dumazet <>
Cc: Peter Zijlstra <>,
	viresh kumar <>,
	Ingo Molnar <>,,,
	Steven Miao <>,
Subject: Re: [PATCH 1/2] timer: Avoid waking up an idle-core by migrate running timer
Date: Wed, 22 Apr 2015 20:56:35 +0200 (CEST)	[thread overview]
Message-ID: <alpine.DEB.2.11.1504222021240.13914@nanos> (raw)
In-Reply-To: <>

On Wed, 22 Apr 2015, Eric Dumazet wrote:
> Check commit 4a8e320c929991c9480 ("net: sched: use pinned timers")
> for a specific example of the problems that can be raised.

If you have a problem with the core timer code then it should be fixed
there and not worked around in some place which will ruin stuff for
power saving interested users. I'm so tired of this 'I fix it in my
sandbox' attitude, really. If the core code has a shortcoming we fix
it there right away because you are probably not the only one who runs
into that shortcoming. So if we don't fix it in the core we end up
with a metric ton of slightly different (or broken) workarounds which
affect the workload/system characteristics of other people.

Just for the record. Even the changelog of this commit is blatantly

  "We can see that timers get migrated into a single cpu, presumably
   idle at the time timers are set up."

The timer migration moves timers to non idle cpus to leave the idle
ones alone for power saving sake.

I can see and understand the reason why you want to avoid that, but I
have to ask the question whether this pinning is the correct behaviour
under all workloads and system characteristics. If yes, then the patch
is the right answer, if no, then it is simply the wrong approach.

> but /proc/sys/kernel/timer_migration adds a fair overhead in many
> workloads.
> get_nohz_timer_target() has to touch 3 cache lines per cpu...

And this is something we can fix and completely avoid if we think
about it. Looking at the code I have to admit that the out of line
call and the sysctl variable lookup is silly. But its not rocket
science to fix this.



  reply	other threads:[~2015-04-22 18:56 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-31  6:55 [PATCH 0/2] timer: Migrate running timers Viresh Kumar
2015-03-31  6:55 ` [PATCH 1/2] timer: Avoid waking up an idle-core by migrate running timer Viresh Kumar
2015-03-31 14:53   ` Peter Zijlstra
2015-04-14 23:13   ` Thomas Gleixner
2015-04-17  8:12     ` viresh kumar
2015-04-17  8:32       ` Ingo Molnar
2015-04-21 21:32       ` Thomas Gleixner
2015-04-21 21:54         ` Eric Dumazet
2015-04-22 15:29           ` Peter Zijlstra
2015-04-22 16:02             ` Eric Dumazet
2015-04-22 18:56               ` Thomas Gleixner [this message]
2015-04-22 19:59                 ` Eric Dumazet
2015-04-22 21:56                   ` Thomas Gleixner
2015-04-23  6:57                     ` Eric Dumazet
2015-04-23 12:45                       ` Thomas Gleixner
2015-04-25 18:37                         ` Eric Dumazet
2015-05-05 13:00                           ` Thomas Gleixner
2015-05-06 16:33                             ` Eric Dumazet
2015-04-15 15:54   ` Thomas Gleixner
2015-03-31  6:55 ` [PATCH 2/2] timer: Replace base-> 'running_timer' with 'busy' Viresh Kumar
2015-03-31 15:01 ` [PATCH 0/2] timer: Migrate running timers Viresh Kumar

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:

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

  git send-email \
    --in-reply-to=alpine.DEB.2.11.1504222021240.13914@nanos \ \ \ \ \ \ \ \ \ \
    --subject='Re: [PATCH 1/2] timer: Avoid waking up an idle-core by migrate running timer' \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).