linux-kernel - Re: [PATCH 4/8] ARCv2: perf: Support sampling events using overflow interrupts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 22 Jun 2015 15:57:23 +0000
From:	Alexey Brodkin <Alexey.Brodkin@...opsys.com>
To:	"peterz@...radead.org" <peterz@...radead.org>
CC:	"Vineet.Gupta1@...opsys.com" <Vineet.Gupta1@...opsys.com>,
	"linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"arnd@...db.de" <arnd@...db.de>,
	"acme@...nel.org" <acme@...nel.org>,
	"arc-linux-dev@...opsys.com" <arc-linux-dev@...opsys.com>
Subject: Re: [PATCH 4/8] ARCv2: perf: Support sampling events using overflow
 interrupts

Hi Peter,

On Mon, 2015-06-15 at 17:48 +0200, Peter Zijlstra wrote:
> On Tue, Jun 09, 2015 at 05:49:28PM +0530, Vineet Gupta wrote:
> > From: Alexey Brodkin <abrodkin@...opsys.com>
> 
> -ENOCHANGELOG
> 
> > Cc: Peter Zijlstra <peterz@...radead.org>
> > Cc: Arnaldo Carvalho de Melo <acme@...nel.org>
> > Signed-off-by: Alexey Brodkin <abrodkin@...opsys.com>
> > Signed-off-by: Vineet Gupta <vgupta@...opsys.com>
> > ---
> 
> >  struct arc_pmu {
> >     struct pmu      pmu;
> > +   int             has_interrupts;
> 
> we have pmu::flags & PERF_PMU_CAP_NO_INTERRUPT

Ah... if we set that flag perf_event_open() won't allow user to use
sampling events anyways so we may drop "arc_pmu.has_interrupts"
completely, right?

> 
> > @@ -186,7 +189,8 @@ static int arc_pmu_event_init(struct perf_event 
> > *event)
> >             hwc->last_period = hwc->sample_period;
> >             local64_set(&hwc->period_left, hwc
> > ->sample_period);
> >     } else
> > -           return -ENOENT;
> > +           if (!arc_pmu->has_interrupts)
> > +                   return -ENOENT;
> 
> Same as before, first determine if the event is yours, then return a
> fatal error.

Indeed with my note above that check makes no sense - perf framework
will do that same check instead.

> > @@ -307,6 +311,17 @@ static void arc_pmu_stop(struct perf_event 
> > *event, int flags)
> >     struct hw_perf_event *hwc = &event->hw;
> >     int idx = hwc->idx;
> >  
> > +   /* Disable interrupt for this counter */
> > +   if (is_sampling_event(event)) {
> 
> but but but, a sampling event needs the interrupt enabled?

Indeed sampling event needs the interrupt enabled. What's wrong with
that fact? Or I'm missing something?

> > +           /*
> > +            * Reset interrupt flag by writing of 1. This is 
> > required
> > +            * to make sure pending interrupt was not left.
> > +            */
> 
> Would not typically the interrupt latch be a property of the 
> interrupt
> controller, not the device generating it?
> 
> That is, how can the device programming affect pending interrupts?

The point here is our hardware counters are built in CPU core and
interrupt line from PMU block is wired directly to the top-level INTC.

Moreover that one IRQ line from PMU (or HW event counters how we call
them) is a logical OR of interrupts from each even counter (we may have
from 1 to 32 counters built-in). That means to clear a pending
interrupt from PMU block we need to clear states of individual
interrupts (per counter) in PMU.

> > +           write_aux_reg(ARC_REG_PCT_INT_ACT, 1 << idx);
> > +           write_aux_reg(ARC_REG_PCT_INT_CTRL,
> > +                    
> >      read_aux_reg(ARC_REG_PCT_INT_CTRL) & ~(1 << idx));
> > +   }
> > +
> 
> > +   if (is_sampling_event(event)) {
> > +           /* Mimic full counter overflow as other arches do 
> > */
> 
> With this you mean the pretending we have 63bit of overflow counter?

Correct. In more details it is covered in my comments in [PATCH 3/8].

> > +           write_aux_reg(ARC_REG_PCT_INT_CNTL, arc_pmu
> > ->max_period &
> > +                                               0xffffffff);
> 
> Would not (u32)arc_pmu->max_period, be clearer?

True, makes sense.

> > +           write_aux_reg(ARC_REG_PCT_INT_CNTH,
> > +                         (arc_pmu->max_period >> 32));
> 
> But should you not program: min(period, max_period) instead? If the
> requested period is shorter than your max period you do not want to
> program the max. Or are you missing a negative somewhere?

Here we just program the limit which is used for triggering an
interrupt. We don't care about "period" being > than "max_period"
because in arc_pmu_start()->arc_pmu_event_set_period() we do that check
when actually setting start value of a counter.

> That is, program the max_period for !sampling events to deal with
> overflow folding.

Once again we deal with overflow folding in arc_pmu_event_set_period()
even for !sampling events. But there's no need to program max_period in
PMU registers because hardware doesn't care about it - we just use that
value in software. Moreover if HW lacks PMU IRQ support then attempt to
access missing registers (for example ARC_REG_PCT_INT_CNT) will
inevitably lead to instruction error exception.

-Alexey--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
Please read the FAQ at  http://www.tux.org/lkml/