Re: [PATCH v7 1/7] perf/amd/iommu: Misc fix up perf_iommu_read

[Suravee Suthikulpanit]

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