lkml.org 
[lkml]   [2017]   [Jan]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH v7 1/7] perf/amd/iommu: Misc fix up perf_iommu_read
From
Date
Peter,

On 1/11/17 18:57, Peter Zijlstra wrote:
> On Mon, Jan 09, 2017 at 09:33:41PM -0600, Suravee Suthikulpanit wrote:
>> This patch contains the following minor fixup:
>> * Fixed overflow handling since u64 delta would lose the MSB sign bit.
>
> Please explain.. afaict this actually introduces a bug.

I'm changing the u64 to s64 ..... (see below)

>
>> diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c
>> index b28200d..f387baf 100644
>> --- a/arch/x86/events/amd/iommu.c
>> +++ b/arch/x86/events/amd/iommu.c
>> @@ -319,29 +319,30 @@ static void perf_iommu_start(struct perf_event *event, int flags)
>>
>> static void perf_iommu_read(struct perf_event *event)
>> {
>> - u64 count = 0ULL;
>> - u64 prev_raw_count = 0ULL;
>> - u64 delta = 0ULL;
>> + u64 cnt, prev;
>> + s64 delta;

.... (here) because we had a discussion (https://lkml.org/lkml/2016/2/18/325),
and you suggested the following:

Your overflow handling is broken, you want delta to be s64. Otherwise:

delta >>= COUNTER_SHIFT;

ends up as a SHR and you loose the MSB sign bits.

>> struct hw_perf_event *hwc = &event->hw;
>> pr_debug("perf: amd_iommu:perf_iommu_read\n");
>>
>> amd_iommu_pc_get_set_reg_val(_GET_DEVID(event),
>> _GET_BANK(event), _GET_CNTR(event),
>> - IOMMU_PC_COUNTER_REG, &count, false);
>> + IOMMU_PC_COUNTER_REG, &cnt, false);
>>
>> /* IOMMU pc counter register is only 48 bits */
>> - count &= 0xFFFFFFFFFFFFULL;
>> + cnt &= GENMASK_ULL(48, 0);
>>
>> - prev_raw_count = local64_read(&hwc->prev_count);
>> - if (local64_cmpxchg(&hwc->prev_count, prev_raw_count,
>> - count) != prev_raw_count)
>> - return;
>> + prev = local64_read(&hwc->prev_count);
>>
>> - /* Handling 48-bit counter overflowing */
>> - delta = (count << COUNTER_SHIFT) - (prev_raw_count << COUNTER_SHIFT);
>> + /*
>> + * Since we do not enable counter overflow interrupts,
>> + * we do not have to worry about prev_count changing on us.
>> + */
>
> So you cannot group this event with a software event that reads this
> from their sample?

Not sure if I understand you point here. When you say sample, I assume you mean
the profiling mode used w/ perf record. These counters are not supported for
sampling mode. So, we only perf stat (i.e. counting mode).

Thanks,
Suravee

\
 
 \ /
  Last update: 2017-01-15 03:37    [W:0.134 / U:1.716 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site