LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: "Suzuki K. Poulose" <Suzuki.Poulose@arm.com>
To: Peter Zijlstra <peterz@infradead.org>,
	Mark Rutland <Mark.Rutland@arm.com>
Cc: Will Deacon <Will.Deacon@arm.com>,
	"linux@arm.linux.org.uk" <linux@arm.linux.org.uk>,
	"acme@kernel.org" <acme@kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	Punit Agrawal <Punit.Agrawal@arm.com>,
	Pawel Moll <Pawel.Moll@arm.com>
Subject: Re: [PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs
Date: Tue, 10 Mar 2015 14:05:23 +0000	[thread overview]
Message-ID: <54FEFA23.7050307@arm.com> (raw)
In-Reply-To: <20150310130041.GC11574@worktop.ger.corp.intel.com>

On 10/03/15 13:00, Peter Zijlstra wrote:
> On Tue, Mar 10, 2015 at 01:53:51PM +0100, Peter Zijlstra wrote:
>>> It would be nicer if we could prevent this in the core so we're not
>>> reliant on every PMU driver doing the same verification. My initial
>>> thought was that seemed like unnecessary duplication of the ctx checking
>>> above, but if we're going to end up shoving it into several drivers
>>> anyway perhaps it's the lesser evil.
>>
>> Again, agreed, that would be better and less error prone. But I'm not
>> entirely sure how to go about doing it :/ I'll have to go think about
>> that; and conferences are not the best place for that.
>>
>> Suggestions on that are welcome of course ;)
>
> So the problem is that event_init() is what will return the pmu, so we
> cannot make decisions on it until after that returns.
>
> Maybe we can pull out the validate step into its own funciton;
> pmu->validate() or whatnot, to be called slightly later.
>
I think we could still solve this problem by deferring the 'context'
validation to the core. The PMUs could validate the group, within its
context. i.e, if it can accommodate its events as a group, during 
event_init.  The problem we face now, is encountering an event from a 
different PMU, which we could leave it to the core as we do already.

i.e the fix could look like (and similarly for other cases):

diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
index 557e128..b3af19b 100644
--- a/arch/arm/kernel/perf_event.c
+++ b/arch/arm/kernel/perf_event.c
@@ -259,20 +259,28 @@ out:
  }

  static int
-validate_event(struct pmu_hw_events *hw_events,
-	       struct perf_event *event)
+validate_event(struct pmu *pmu, struct pmu_hw_events *hw_events,
+			       struct perf_event *event)
  {
-	struct arm_pmu *armpmu = to_arm_pmu(event->pmu);
+	struct arm_pmu *armpmu;

  	if (is_software_event(event))
  		return 1;

+	/*
+	 * We are only worried if we can accommodate the events
+	 * from this pmu in this group.
+	 */
+	if (event->pmu != pmu)
+		return 1;
+
  	if (event->state < PERF_EVENT_STATE_OFF)
  		return 1;

  	if (event->state == PERF_EVENT_STATE_OFF && !event->attr.enable_on_exec)
  		return 1;

+	armpmu = to_arm_pmu(event->pmu);
  	return armpmu->get_event_idx(hw_events, event) >= 0;
  }

@@ -288,15 +296,15 @@ validate_group(struct perf_event *event)
  	 */
  	memset(&fake_pmu.used_mask, 0, sizeof(fake_pmu.used_mask));

-	if (!validate_event(&fake_pmu, leader))
+	if (!validate_event(event->pmu, &fake_pmu, leader))
  		return -EINVAL;

  	list_for_each_entry(sibling, &leader->sibling_list, group_entry) {
-		if (!validate_event(&fake_pmu, sibling))
+		if (!validate_event(event->pmu, &fake_pmu, sibling))
  			return -EINVAL;
  	}

-	if (!validate_event(&fake_pmu, event))
+	if (!validate_event(event->pmu, &fake_pmu, event))
  		return -EINVAL;

  	return 0;

Thoughts ?

Suzuki


  parent reply	other threads:[~2015-03-10 14:05 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-03-09 12:46 [PATCH 0/3] [4.0] arm/arm64: Do not group hardware events from different PMUs Suzuki K. Poulose
2015-03-09 12:46 ` [PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs Suzuki K. Poulose
2015-03-10 11:27   ` Peter Zijlstra
2015-03-10 12:00     ` Suzuki K. Poulose
2015-03-10 12:05     ` Mark Rutland
2015-03-10 12:53       ` Peter Zijlstra
2015-03-10 13:00         ` Peter Zijlstra
2015-03-10 13:57           ` Mark Rutland
2015-03-10 14:05           ` Suzuki K. Poulose [this message]
2015-03-10 15:09             ` Mark Rutland
2015-03-10 15:36         ` Mark Rutland
2015-03-10 15:44           ` Peter Zijlstra
2015-03-09 12:46 ` [PATCH 2/3] arm64/pmu: Reject groups spanning multiple HW PMUs Suzuki K. Poulose
2015-03-09 12:46 ` [PATCH 3/3] arm-cci: " Suzuki K. Poulose
  -- strict thread matches above, loose matches on Subject: below --
2015-03-09 12:43 [PATCH 0/3] [4.0] arm/arm64: Do not group hardware events from different PMUs a
2015-03-09 12:43 ` [PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs a

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=54FEFA23.7050307@arm.com \
    --to=suzuki.poulose@arm.com \
    --cc=Mark.Rutland@arm.com \
    --cc=Pawel.Moll@arm.com \
    --cc=Punit.Agrawal@arm.com \
    --cc=Will.Deacon@arm.com \
    --cc=acme@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@arm.linux.org.uk \
    --cc=peterz@infradead.org \
    --subject='Re: [PATCH 1/3] arm/pmu: Reject groups spanning multiple hardware PMUs' \
    /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: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).