Re: [tip:perf/core] perf/x86: Add generic Intel uncore PMU support

[Andrew Morton <akpm@linux-foundation.org>]

On Wed, 20 Jun 2012 03:54:20 -0700
"tip-bot for Yan, Zheng" <zheng.z.yan@intel.com> wrote:

> This patch adds the generic Intel uncore PMU support, including helper
> functions that add/delete uncore events, a hrtimer that periodically
> polls the counters to avoid overflow and code that places all events
> for a particular socket onto a single cpu.
> 
> The code design is based on the structure of Sandy Bridge-EP's uncore
> subsystem, which consists of a variety of components, each component
> contains one or more "boxes".
> 
> (Tooling support follows in the next patches.)
>
> ...
>
> +static enum hrtimer_restart uncore_pmu_hrtimer(struct hrtimer *hrtimer)
> +{
> +	struct intel_uncore_box *box;
> +	unsigned long flags;
> +	int bit;
> +
> +	box = container_of(hrtimer, struct intel_uncore_box, hrtimer);
> +	if (!box->n_active || box->cpu != smp_processor_id())
> +		return HRTIMER_NORESTART;
> +	/*
> +	 * disable local interrupt to prevent uncore_pmu_event_start/stop
> +	 * to interrupt the update process
> +	 */
> +	local_irq_save(flags);
> +
> +	for_each_set_bit(bit, box->active_mask, UNCORE_PMC_IDX_MAX)
> +		uncore_perf_event_update(box, box->events[bit]);
> +
> +	local_irq_restore(flags);
> +
> +	hrtimer_forward_now(hrtimer, ns_to_ktime(UNCORE_PMU_HRTIMER_INTERVAL));
> +	return HRTIMER_RESTART;
> +}

i386 allmodconfig:

arch/x86/kernel/cpu/perf_event_intel_uncore.c: In function 'uncore_pmu_hrtimer':
arch/x86/kernel/cpu/perf_event_intel_uncore.c:728: warning: integer overflow in expression
arch/x86/kernel/cpu/perf_event_intel_uncore.c: In function 'uncore_pmu_start_hrtimer':
arch/x86/kernel/cpu/perf_event_intel_uncore.c:735: warning: integer overflow in expression


Should we even be compiling this code in an i386 build?


Regardless of that, we have some head-scratching to do:


#define UNCORE_PMU_HRTIMER_INTERVAL       (60 * NSEC_PER_SEC)

and

#define NSEC_PER_SEC    1000000000L

and 60 billion doesn't fit in 32 bits.  So do we fix the
perf_event_intel_uncore.c callsites?  Or do we fix the
UNCORE_PMU_HRTIMER_INTERVAL definition?  Or do we fix the NSEC_PER_SEC
definition?

I'm thinking perhaps the latter.  What *is* the type of a nanosecond in
Linux?  include/linux/ktime.h is pretty insistent that it is u64.  If
so, NSEC_PER_SEC should logically have type ULL.  But changing both its
size and signedness is a pretty big change.