All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jeremy Fitzhardinge <jeremy@goop.org>
To: Ingo Molnar <mingo@elte.hu>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Avi Kivity <avi@redhat.com>, Ingo Molnar <mingo@redhat.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Andi Kleen <ak@linux.intel.com>, "H. Peter Anvin" <hpa@zytor.com>
Subject: Re: [PATCH RFC] sched: add notifier for process migration
Date: Wed, 14 Oct 2009 09:15:29 -0700	[thread overview]
Message-ID: <4AD5F921.8080007@goop.org> (raw)
In-Reply-To: <20091014070508.GI784@elte.hu>

On 10/14/09 00:05, Ingo Molnar wrote:
> * Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>
>   
>> @@ -1981,6 +1989,12 @@ void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
>>  #endif
>>  		perf_swcounter_event(PERF_COUNT_SW_CPU_MIGRATIONS,
>>  				     1, 1, NULL, 0);
>> +
>> +		tmn.task = p;
>> +		tmn.from_cpu = old_cpu;
>> +		tmn.to_cpu = new_cpu;
>> +
>> +		atomic_notifier_call_chain(&task_migration_notifier, 0, &tmn);
>>     
> We already have one event notifier there - look at the 
> perf_swcounter_event() callback. Why add a second one for essentially 
> the same thing?
>
> We should only put a single callback there - a tracepoint defined via 
> TRACE_EVENT() - and any secondary users can register a callback to the 
> tracepoint itself.
>
> There's many similar places in the kernel - with notifier chains and 
> also with a need to get tracepoints there. The fastest (and most 
> consistent) solution is to add just a single event callback facility.
>   

My specific use case for this notifier is to provide a "you've been
migrated" counter to usermode via a fixmap page, as part of the work to
extend kernel/pvclock.c to implement vread for vsyscall use.  I probably
should have referred to that explicitly in the comment for the patch to
give a concrete motivation and rationale.

This means that on applicable systems - ie, running virtualized under
Xen or KVM - this will be something that will be installed early in boot
and called for the entire uptime of the system.  Since we don't want a
strong permanent coupling between that particular piece of
arch-independent scheduler code and an arch-specific piece of
functionality, it seemed like a notifier is a good fit.

(Note that this callback is generally useful on all systems for the
vgetcpu vsyscall; it would allow us to use the "tcache" parameter to
provide results which are both fast and 100% accurate, by deferring the
use of expensive lsl/rdtscp instructions until it *knows* the cpu has
changed.)

I tend to view the intent of tracepoints as more a diagnostic tool which
are inserted and removed dynamically as a way of instrumenting a running
system, and the tracepoints themselves don't have side-effects required
for correct running of the system.

More handwavingly, I see the semantics of a tracepoint is basically a
flag-fall showing that a particular piece of kernel code has been
called, whereas notifications are that a particular event has occurred
(which may not be associated with any specific piece of code being
executed).  This notion of "task X has been migrated from cpu A to B"
seems like a fairly high-level concept; the fact that it can be
implemented by hooking a single piece of code is side-effect of the
modularity of the scheduler rather than anything relating to the event
itself.

Functionally, tracepoints and notifiers do have broad similarities. 
Should they be unified?  I don't know, but they do seem to serve
distinct roles.

    J

      parent reply	other threads:[~2009-10-14 16:16 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-10-09 21:01 [PATCH RFC] sched: add notifier for process migration Jeremy Fitzhardinge
2009-10-09 22:02 ` Peter Zijlstra
2009-10-09 22:43   ` Jeremy Fitzhardinge
2009-10-10  7:14     ` Peter Zijlstra
2009-10-10  9:05       ` Avi Kivity
2009-10-10  9:24         ` Peter Zijlstra
2009-10-10  9:36           ` Jeremy Fitzhardinge
2009-10-10 10:12             ` Peter Zijlstra
2009-10-13 21:25               ` Jeremy Fitzhardinge
2009-10-14  7:05                 ` Ingo Molnar
2009-10-14  9:26                   ` Peter Zijlstra
2009-10-14 10:37                     ` Avi Kivity
2009-10-14 14:41                     ` Jason Baron
2009-10-14 16:15                   ` Jeremy Fitzhardinge [this message]

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=4AD5F921.8080007@goop.org \
    --to=jeremy@goop.org \
    --cc=ak@linux.intel.com \
    --cc=avi@redhat.com \
    --cc=hpa@zytor.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@elte.hu \
    --cc=mingo@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.