lkml.org 
[lkml]   [2012]   [Mar]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 2/5] perf: generic intel uncore support
Overall the driver looks rather good. Thanks.

On Wed, Mar 28, 2012 at 02:43:15PM +0800, Yan, Zheng wrote:
> +static void uncore_perf_event_update(struct intel_uncore_box *box,
> + struct perf_event *event)
> +{
> + raw_spin_lock(&box->lock);

I think a raw lock would be only needed if the uncore was called
from the scheduler context switch, which it should not be.

So you can use a normal lock instead of a raw lock.


> +static void uncore_pmu_start_hrtimer(struct intel_uncore_box *box)
> +{
> + __hrtimer_start_range_ns(&box->hrtimer,
> + ns_to_ktime(UNCORE_PMU_HRTIMER_INTERVAL), 0,
> + HRTIMER_MODE_REL_PINNED, 0);
> +}

Can probably do some slack to be more friendly for power.

> +static struct intel_uncore_box *
> +uncore_pmu_find_box(struct intel_uncore_pmu *pmu, int phyid)
> +{
> + struct intel_uncore_box *box;
> +
> + rcu_read_lock();

I'm not sure RCU is really needed here, are any of those paths
time critical? But ok shouldn't hurt either.

> +static int __init uncore_cpu_init(void)
> +{
> + int ret, cpu;
> +
> + switch (boot_cpu_data.x86_model) {
> + default:
> + return 0;
> + }

Needs a case? Always returns?

> +
> + ret = uncore_types_init(msr_uncores);
> + if (ret)
> + return ret;
> +
> + get_online_cpus();
> + for_each_online_cpu(cpu)
> + uncore_cpu_prepare(cpu);
> +
> + preempt_disable();
> + smp_call_function(uncore_cpu_setup, NULL, 1);
> + uncore_cpu_setup(NULL);
> + preempt_enable();

That's on_each_cpu()


-Andi
--
ak@linux.intel.com -- Speaking for myself only.


\
 
 \ /
  Last update: 2012-03-28 11:27    [from the cache]
©2003-2011 Jasper Spaans