public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Peter Zijlstra <peterz@infradead.org>
To: Tejun Heo <tj@kernel.org>
Cc: mingo@elte.hu, efault@gmx.de, avi@redhat.com, paulus@samba.org,
	acme@redhat.com, linux-kernel@vger.kernel.org
Subject: Re: [RFC PATCHSET] sched,perf: unify tracers in sched and move perf on top of TP
Date: Wed, 05 May 2010 11:06:40 +0200	[thread overview]
Message-ID: <1273050400.1642.229.camel@laptop> (raw)
In-Reply-To: <4BE0FB68.7080403@kernel.org>

On Wed, 2010-05-05 at 07:00 +0200, Tejun Heo wrote:
> Hello,
> 
> On 05/04/2010 07:29 PM, Peter Zijlstra wrote:
> >> * 0001-0007: Unify the three tracers (tracepoints, perf_events and
> >>   preempt/sched notifiers) in scheduler.
> > 
> > Right, so I really don't like the SCHED_EVENT() thing much, esp the 3
> > different function postfixes.
> 
> Yeap, it's not the prettiest thing.  I thought about renaming all of
> them so that they share the same postfix but then again I need a way
> to tell the script which to enable which is the easiest with
> specifying postfixes as macro argument.  Any better ideas?

Add more NOP functions for the missing bits I guess, but that gets
cumbersome too.

> >> * 0008-0012: Move perf hooks in sched on top of tracepoints if TPs are
> >>              enabled.
> > 
> > This seems to add a terrible amount of overhead for no particular
> > reason.
> 
> Hmm... What overhead?

Well, the perf_{inc,dec}_nr_events() stuff, that's massively painful. If
that is the price to pay for using tracepoints then I'd rather not.

Also, the whole magic dance to keep !CONFIG_TRACEPOINTS working doesn't
make the code any saner.

> What matters more here is avoiding the overhead when perf or whatever
> tracing mechanism is disabled.  When both perf and TPs are compiled in
> but not enabled, moving perf hooks on top of TPs means that the sched
> core code doesn't have to call into extra functions for perf and TPs
> can be nooped.  ie. less overhead.  Also, even when perf is enabled,
> there isn't any inherent extra runtime overhead other than during
> enabling/disabling which again is a much colder path.

Well, in the current code we get the call overhead, but all perf
functions bail on !nr_events. We could invert that and have an inline
function check nr_events and only then do the call.

> The only place where noticeable overhead is added is the extra pair of
> irq enable/disable that I added for sched_out hook.  After glancing
> through what perf does during context switch, I thought that overhead
> wouldn't be anything noticeable but if it is, they can definitely be
> separated.  The only purpose of that change was colocating sched
> notifiers and perf hooks.

Right, recent Intel chips seem to do much better at IRQ disable than say
P4 and IA64, and poking at the PMU MSRs is way more painful, not sure
how it would balance out for PowerPC (or anything other) though.

> Also, if a few more perf hooks are converted to use the same
> mechanism, it would be possible to put perf properly on top of TPs
> other than init/exit code paths which will be cleaner for the rest of
> the kernel and there won't be any extra runtime overhead.
> 
> The code will be much cleaner if perf depends on TPs.  With perf hooks
> completely removed, there won't be much point in SCHED_EVENT and the
> messy ifdefs in perf can also go away.  Would this be a possibility?

Well, I already utterly hate that x86 can't build with !
CONFIG_PERF_EVENTS, also requiring CONFIG_TRACEPOINTS is going the wrong
way.

I've always explicitly avoided depending on tracepoints, and I'd very
much like to keep it that way.


  reply	other threads:[~2010-05-05  9:06 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-05-04 12:38 [RFC PATCHSET] sched,perf: unify tracers in sched and move perf on top of TP Tejun Heo
2010-05-04 12:38 ` [PATCH 01/12] sched: drop @cpu argument from sched_in preempt notifier Tejun Heo
2010-05-04 17:11   ` Peter Zijlstra
2010-05-04 12:38 ` [PATCH 02/12] sched: rename preempt_notifiers to sched_notifiers and refactor implementation Tejun Heo
2010-05-04 12:38 ` [PATCH 03/12] perf: add perf_event_task_migrate() Tejun Heo
2010-05-05  5:08   ` Frederic Weisbecker
2010-05-05  5:16     ` Tejun Heo
2010-05-05  9:11     ` Peter Zijlstra
2010-05-05  9:37       ` Tejun Heo
2010-05-05  9:50         ` Peter Zijlstra
2010-05-05  9:56           ` Tejun Heo
2010-05-04 12:38 ` [PATCH 04/12] perf: add @rq to perf_event_task_sched_out() Tejun Heo
2010-05-04 17:11   ` Peter Zijlstra
2010-05-04 17:22     ` Tejun Heo
2010-05-04 17:42       ` Peter Zijlstra
2010-05-04 18:22       ` Steven Rostedt
2010-05-04 18:26         ` Peter Zijlstra
2010-05-04 18:32           ` Steven Rostedt
2010-05-05  4:48             ` Tejun Heo
2010-05-05  9:58             ` Tejun Heo
2010-05-07 18:41           ` [tip:sched/core] sched: Remove rq argument to the tracepoints tip-bot for Peter Zijlstra
2010-05-04 12:38 ` [PATCH 05/12] perf: move perf_event_task_sched_in() next to fire_sched_notifiers_in() Tejun Heo
2010-05-04 12:38 ` [PATCH 06/12] sched: relocate fire_sched_notifiers_out() and trace_sched_switch() Tejun Heo
2010-05-04 12:38 ` [PATCH 07/12] sched: coalesce event notifiers Tejun Heo
2010-05-04 12:38 ` [PATCH 08/12] sched: add switch_in and tick tracepoints Tejun Heo
2010-05-04 12:38 ` [PATCH 09/12] perf: factor out perf_event_switch_clones() Tejun Heo
2010-05-04 12:38 ` [PATCH 10/12] perf: make nr_events an int and add perf_online_mutex to protect it Tejun Heo
2010-05-04 12:38 ` [PATCH 11/12] perf: prepare to move sched perf functions on top of tracepoints Tejun Heo
2010-05-04 12:38 ` [PATCH 12/12] perf: " Tejun Heo
2010-05-04 17:29 ` [RFC PATCHSET] sched,perf: unify tracers in sched and move perf on top of TP Peter Zijlstra
2010-05-05  5:00   ` Tejun Heo
2010-05-05  9:06     ` Peter Zijlstra [this message]
2010-05-05  9:32       ` Tejun Heo
2010-05-05  9:51         ` Peter Zijlstra
2010-05-05  9:54           ` Tejun Heo
2010-05-05 11:38             ` Peter Zijlstra
2010-05-05 12:28               ` Tejun Heo
2010-05-05 16:55                 ` Ingo Molnar
2010-05-05 18:12                   ` Peter Zijlstra
2010-05-05 18:16                     ` Peter Zijlstra
2010-05-05 18:30                       ` Frederic Weisbecker
2010-05-06  6:28                         ` Ingo Molnar
2010-05-06  7:11                           ` Tejun Heo
2010-05-06  8:27                             ` Ingo Molnar
2010-05-06  8:41                               ` Tejun Heo
2010-05-06  8:18                           ` Avi Kivity
2010-05-06  6:31                     ` Ingo Molnar
2010-05-06  7:04                       ` Peter Zijlstra
2010-05-06  7:11                         ` Ingo Molnar
2010-05-06  7:29                           ` Tejun Heo
2010-05-06  7:33                             ` Tejun Heo
2010-05-05 12:33               ` Avi Kivity
2010-05-05 13:09                 ` Tejun Heo
2010-05-10  5:20             ` Paul Mackerras
2010-05-10  5:48               ` Tejun Heo

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=1273050400.1642.229.camel@laptop \
    --to=peterz@infradead.org \
    --cc=acme@redhat.com \
    --cc=avi@redhat.com \
    --cc=efault@gmx.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@elte.hu \
    --cc=paulus@samba.org \
    --cc=tj@kernel.org \
    /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