Re: [PATCH 2/2] perf tools: Cope with sparsely-numbered CPUs

From: Ingo Molnar
Date: Tue Dec 15 2009 - 07:09:16 EST



* Paul Mackerras <paulus@xxxxxxxxx> wrote:

> For system-wide monitoring, the perf tools currently ask how many CPUs are
> online, and then instantiate perf_events for CPUs 0 ... N-1. This doesn't
> work correctly when CPUs are numbered sparsely. For example, a four-core
> POWER6 in single-thread mode will have CPUs 0, 2, 4 and 6. The perf tools
> will try to open counters on CPUs 0, 1, 2 and 3, and either fail with an
> error message or silently ignore CPUs 4 and 6.
>
> This fixes the problem by making perf stat, perf record and perf top
> create counters for increasing CPU numbers until they have a counter
> for each online CPU. If the attempt to create a counter on a given
> CPU fails, we get an ENODEV error and we just move on to the next CPU.
> To avoid an infinite loop in case the number of online CPUs gets
> reduced while we are creating counters, we re-read the number of
> online CPUs when we fail to create a counter on some CPU.
>
> Reported-by: Michael Neuling <mikey@xxxxxxxxxxx>
> Tested-by: Michael Neuling <mikey@xxxxxxxxxxx>
> Cc: stable@xxxxxxxxxx
> Signed-off-by: Paul Mackerras <paulus@xxxxxxxxx>
> ---
> tools/perf/builtin-record.c | 36 ++++++++++++++++++++++++++++--------
> tools/perf/builtin-stat.c | 15 +++++++++++++--
> tools/perf/builtin-top.c | 27 +++++++++++++++++++--------
> 3 files changed, 60 insertions(+), 18 deletions(-)

nice fix!

The linecount bloat is a bit worrying though. I'm wondering, since we have 3
loops now (and possibly more upcoming), wouldnt it be a cleaner fix to have
some generic idiom of 'loop through all online cpus' somewhere in lib/*.c?

This would work better in the long run than spreading all the sysconf calls
and special cases across all those callsites. (new tools will inevitably get
it wrong as well)

As a practical matter we can commit your fix and do the cleanup/consolidation
as a separate patch, to not hold up your fix (and to help fix/bisect any
problems that would happen due to the consolidation) - as long as a
consolidation patch is forthcoming as well.

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/