From: Peter Zijlstra <peterz@infradead.org> To: Mark Rutland <mark.rutland@arm.com> Cc: Neil Leeder <nleeder@codeaurora.org>, Catalin Marinas <catalin.marinas@arm.com>, WillDeacon <will.deacon@arm.com>, Ingo Molnar <mingo@redhat.com>, Arnaldo Carvalho de Melo <acme@kernel.org>, linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Mark Langsdorf <mlangsdo@redhat.com>, Mark Salter <msalter@redhat.com>, Jon Masters <jcm@redhat.com>, Timur Tabi <timur@codeaurora.org>, cov@codeaurora.org Subject: Re: [PATCH 2/2] soc: qcom: add l2 cache perf events driver Date: Thu, 9 Jun 2016 21:41:36 +0200 [thread overview] Message-ID: <20160609194136.GZ30154@twins.programming.kicks-ass.net> (raw) In-Reply-To: <20160609155615.GE12201@leverpostej> On Thu, Jun 09, 2016 at 04:56:16PM +0100, Mark Rutland wrote: > > >> +static irqreturn_t l2_cache__handle_irq(int irq_num, void *data) > > >> +{ > > >> + struct hml2_pmu *slice = data; > > >> + u32 ovsr; > > >> + int idx; > > >> + struct pt_regs *regs; > > >> + > > >> + ovsr = hml2_pmu__getreset_ovsr(); > > >> + if (!hml2_pmu__has_overflowed(ovsr)) > > >> + return IRQ_NONE; > > >> + > > >> + regs = get_irq_regs(); > > >> + > > >> + for (idx = 0; idx < l2cache_pmu.num_counters; idx++) { > > >> + struct perf_event *event = slice->events[idx]; > > >> + struct hw_perf_event *hwc; > > >> + struct perf_sample_data data; > > >> + > > >> + if (!event) > > >> + continue; > > >> + > > >> + if (!hml2_pmu__counter_has_overflowed(ovsr, idx)) > > >> + continue; > > >> + > > >> + l2_cache__event_update_from_slice(event, slice); > > >> + hwc = &event->hw; > > >> + > > >> + if (is_sampling_event(event)) { > > >> + perf_sample_data_init(&data, 0, hwc->last_period); > > > > > > I don't think sampling makes sense, given this is an uncore PMU and the > > > events are triggered by other CPUs. > > > > There is origin filtering so events can be attributed to a CPU when sampling. > > Ok. I believe that's different from all other uncore PMUs we support > (none of the drivers support sampling, certainly), so I'm not entirely > sure how/if we can make use of that sanely and reliably. Right; because not only do you need to know which CPU originated the event, the IRQ must also happen on that CPU. Simply knowing which CPU triggered it is not enough for sampling. > For the timebeing, I think this sampling needs to go, and the event_init > logic needs to reject sampling as with other uncore PMU drivers. Agreed. > One thing I forgot to mention in my earlier comments is that as an > uncore PMU you need to have task_ctx_nr = perf_invalid_context here > also. For good reasons, uncore PMUs (as is the case here) count strictly more than the effect of single CPUs (and thus also the current task). So attributing it back to a task is nonsense.
WARNING: multiple messages have this Message-ID (diff)
From: peterz@infradead.org (Peter Zijlstra) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 2/2] soc: qcom: add l2 cache perf events driver Date: Thu, 9 Jun 2016 21:41:36 +0200 [thread overview] Message-ID: <20160609194136.GZ30154@twins.programming.kicks-ass.net> (raw) In-Reply-To: <20160609155615.GE12201@leverpostej> On Thu, Jun 09, 2016 at 04:56:16PM +0100, Mark Rutland wrote: > > >> +static irqreturn_t l2_cache__handle_irq(int irq_num, void *data) > > >> +{ > > >> + struct hml2_pmu *slice = data; > > >> + u32 ovsr; > > >> + int idx; > > >> + struct pt_regs *regs; > > >> + > > >> + ovsr = hml2_pmu__getreset_ovsr(); > > >> + if (!hml2_pmu__has_overflowed(ovsr)) > > >> + return IRQ_NONE; > > >> + > > >> + regs = get_irq_regs(); > > >> + > > >> + for (idx = 0; idx < l2cache_pmu.num_counters; idx++) { > > >> + struct perf_event *event = slice->events[idx]; > > >> + struct hw_perf_event *hwc; > > >> + struct perf_sample_data data; > > >> + > > >> + if (!event) > > >> + continue; > > >> + > > >> + if (!hml2_pmu__counter_has_overflowed(ovsr, idx)) > > >> + continue; > > >> + > > >> + l2_cache__event_update_from_slice(event, slice); > > >> + hwc = &event->hw; > > >> + > > >> + if (is_sampling_event(event)) { > > >> + perf_sample_data_init(&data, 0, hwc->last_period); > > > > > > I don't think sampling makes sense, given this is an uncore PMU and the > > > events are triggered by other CPUs. > > > > There is origin filtering so events can be attributed to a CPU when sampling. > > Ok. I believe that's different from all other uncore PMUs we support > (none of the drivers support sampling, certainly), so I'm not entirely > sure how/if we can make use of that sanely and reliably. Right; because not only do you need to know which CPU originated the event, the IRQ must also happen on that CPU. Simply knowing which CPU triggered it is not enough for sampling. > For the timebeing, I think this sampling needs to go, and the event_init > logic needs to reject sampling as with other uncore PMU drivers. Agreed. > One thing I forgot to mention in my earlier comments is that as an > uncore PMU you need to have task_ctx_nr = perf_invalid_context here > also. For good reasons, uncore PMUs (as is the case here) count strictly more than the effect of single CPUs (and thus also the current task). So attributing it back to a task is nonsense.
next prev parent reply other threads:[~2016-06-09 19:41 UTC|newest] Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-06-03 21:03 [PATCH 0/2] qcom: add l2 cache perf events driver Neil Leeder 2016-06-03 21:03 ` Neil Leeder 2016-06-03 21:03 ` [PATCH 1/2] perf: allow add to change event state Neil Leeder 2016-06-03 21:03 ` Neil Leeder 2016-06-03 21:38 ` Peter Zijlstra 2016-06-03 21:38 ` Peter Zijlstra 2016-06-03 21:38 ` Peter Zijlstra 2016-06-03 21:03 ` [PATCH 2/2] soc: qcom: add l2 cache perf events driver Neil Leeder 2016-06-03 21:03 ` Neil Leeder 2016-06-06 9:51 ` Mark Rutland 2016-06-06 9:51 ` Mark Rutland 2016-06-08 15:16 ` Neil Leeder 2016-06-08 15:16 ` Neil Leeder 2016-06-08 15:16 ` Neil Leeder 2016-06-09 15:56 ` Mark Rutland 2016-06-09 15:56 ` Mark Rutland 2016-06-09 19:41 ` Peter Zijlstra [this message] 2016-06-09 19:41 ` Peter Zijlstra 2016-06-10 22:34 ` Neil Leeder 2016-06-10 22:34 ` Neil Leeder 2016-06-06 9:04 ` [PATCH 0/2] " Mark Rutland 2016-06-06 9:04 ` Mark Rutland 2016-06-08 15:21 ` Neil Leeder 2016-06-08 15:21 ` Neil Leeder 2016-06-08 15:21 ` Neil Leeder 2016-06-08 16:12 ` Mark Rutland 2016-06-08 16:12 ` Mark Rutland 2016-06-08 19:29 ` Neil Leeder 2016-06-08 19:29 ` Neil Leeder
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20160609194136.GZ30154@twins.programming.kicks-ass.net \ --to=peterz@infradead.org \ --cc=acme@kernel.org \ --cc=catalin.marinas@arm.com \ --cc=cov@codeaurora.org \ --cc=jcm@redhat.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=mingo@redhat.com \ --cc=mlangsdo@redhat.com \ --cc=msalter@redhat.com \ --cc=nleeder@codeaurora.org \ --cc=timur@codeaurora.org \ --cc=will.deacon@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.