Re: [PATCH 2/5] perf: generic intel uncore support

From: Yan, Zheng
Date: Wed Mar 28 2012 - 07:24:21 EST


On 03/28/2012 05:24 PM, Andi Kleen wrote:
> 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.
>
It's not time critical. but using RCU here is as simple as
using lock. So I decided to use RCU.


>> +static int __init uncore_cpu_init(void)
>> +{
>> + int ret, cpu;
>> +
>> + switch (boot_cpu_data.x86_model) {
>> + default:
>> + return 0;
>> + }
>
> Needs a case? Always returns?

later patches add code to here

>
>> +
>> + 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()
>
will switch to on_each_cpu() in later version of patches

Thanks
Yan, Zheng
>
> -Andi

--
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/