[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 05/13] intel/VPMU: Clean up Intel VPMU code
>>> On 25.09.13 at 16:39, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote: > On 09/25/2013 09:55 AM, Jan Beulich wrote: >>>>> On 20.09.13 at 11:42, Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx> wrote: >>> + { >>> + msr_area[i].index = msr_area[i + 1].index; >>> + rdmsrl(msr_area[i].index, msr_area[i].data); >> This is clearly a side effect of the function call no-one would >> expect. Why do you do this? > > I don't understand what you are trying to say here. > > (And this is wrong, instead of rdmsr it should be > msr_area[i].data = msr_area[i + 1].data; > ) That was the very point - doing an MSR read here is clearly an unexpected side effect. >>> @@ -248,13 +230,13 @@ static void core2_vpmu_set_msr_bitmap(unsigned long >>> *msr_bitmap) >>> int i; >>> >>> /* Allow Read/Write PMU Counters MSR Directly. */ >>> - for ( i = 0; i < core2_fix_counters.num; i++ ) >>> + for ( i = 0; i < fixed_pmc_cnt; i++ ) >>> { >>> - clear_bit(msraddr_to_bitpos(core2_fix_counters.msr[i]), >>> msr_bitmap); >>> - clear_bit(msraddr_to_bitpos(core2_fix_counters.msr[i]), >>> + clear_bit(msraddr_to_bitpos(MSR_CORE_PERF_FIXED_CTR0 + i), > msr_bitmap); >>> + clear_bit(msraddr_to_bitpos(MSR_CORE_PERF_FIXED_CTR0 + i), >> Dropping the static array will make the handling here quite a bit more >> complicated should there ever appear a second dis-contiguous MSR >> range. > > Fixed counters range should always be contiguous per Intel SDM. Until the current range runs out... >>> @@ -262,32 +244,37 @@ static void core2_vpmu_set_msr_bitmap(unsigned long >>> *msr_bitmap) >>> } >>> >>> /* Allow Read PMU Non-global Controls Directly. */ >>> - for ( i = 0; i < core2_ctrls.num; i++ ) >>> - clear_bit(msraddr_to_bitpos(core2_ctrls.msr[i]), msr_bitmap); >>> - for ( i = 0; i < core2_get_pmc_count(); i++ ) >>> + for ( i = 0; i < arch_pmc_cnt; i++ ) >>> clear_bit(msraddr_to_bitpos(MSR_P6_EVNTSEL0+i), msr_bitmap); >>> + >>> + clear_bit(msraddr_to_bitpos(MSR_CORE_PERF_FIXED_CTR_CTRL), msr_bitmap); >>> + clear_bit(msraddr_to_bitpos(MSR_IA32_PEBS_ENABLE), msr_bitmap); >>> + clear_bit(msraddr_to_bitpos(MSR_IA32_DS_AREA), msr_bitmap); >> As you can see, this is already the case here. > > This is a different set of MSRs from from what you've commented on above. Sure, but the effect of breaking up a loop into individual operations is seen quite nicely here. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |