All of lore.kernel.org
 help / color / mirror / Atom feed
From: Ingo Molnar <mingo@elte.hu>
To: Paul Mackerras <paulus@samba.org>
Cc: "Zhang, Yanmin" <yanmin_zhang@linux.intel.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] perf_counter: Make software counters work as per-cpu counters
Date: Mon, 9 Feb 2009 12:48:52 +0100	[thread overview]
Message-ID: <20090209114852.GF18757@elte.hu> (raw)
In-Reply-To: <18832.5815.706605.434454@cargo.ozlabs.ibm.com>


* Paul Mackerras <paulus@samba.org> wrote:

> Impact: kernel crash fix
> 
> Yanmin Zhang reported that using a PERF_COUNT_TASK_CLOCK software
> counter as a per-cpu counter would reliably crash the system, because
> it calls __task_delta_exec with a null pointer.  The page fault,
> context switch and cpu migration counters also won't function
> correctly as per-cpu counters since they reference the current task.
> 
> This fixes the problem by redirecting the task_clock counter to the
> cpu_clock counter when used as a per-cpu counter, and by implementing
> per-cpu page fault, context switch and cpu migration counters.
> 
> Along the way, this:
> 
> - Initializes counter->ctx earlier, in perf_counter_alloc, so that
>   sw_perf_counter_init can use it
> - Adds code to kernel/sched.c to count task migrations into each
>   cpu, in rq->nr_migrations_in
> - Exports the per-cpu context switch and task migration counts
>   via new functions added to kernel/sched.c
> - Makes sure that if sw_perf_counter_init fails, we don't try to
>   initialize the counter as a hardware counter.  Since the user has
>   passed a negative, non-raw event type, they clearly don't intend
>   for it to be interpreted as a hardware event.
> 
> Signed-off-by: Paul Mackerras <paulus@samba.org>

Very nice, thanks Paul!

> I'm a little concerned about the use of u64 for the existing
> rq->nr_switches and the new rq->nr_migrations_in.  On 32-bit machines
> this will get updated in two halves, so there is a very small but
> non-zero probability that reading it will give a bogus value.  It's
> not clear to me what the best way to fix it is, since not all 32-bit
> platforms have atomic64_t.  Maybe make nr_switches and
> nr_migrations_in unsigned long, or atomic_t?

It used to be 'just stats' so we dont really mind. But now that we
expose it in a bit more systematic way i agree that it should be
fixed. Updating to 'unsigned long' sounds good to me.

> I have some sort of git problem with my perfcounters.git repository.
> I'll let you know when I have that sorted out.

ok - i've applied your patch from email to allow Yanmin to test tip:master.

	Ingo

  reply	other threads:[~2009-02-09 11:49 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-02-09 11:42 [PATCH] perf_counter: Make software counters work as per-cpu counters Paul Mackerras
2009-02-09 11:48 ` Ingo Molnar [this message]
2009-02-10  2:21   ` Zhang, Yanmin
2009-02-10 13:14     ` Ingo Molnar

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=20090209114852.GF18757@elte.hu \
    --to=mingo@elte.hu \
    --cc=linux-kernel@vger.kernel.org \
    --cc=paulus@samba.org \
    --cc=yanmin_zhang@linux.intel.com \
    /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.