All of lore.kernel.org
 help / color / mirror / Atom feed
From: Peter Zijlstra <peterz@infradead.org>
To: Jiri Olsa <jolsa@redhat.com>
Cc: ak@linux.intel.com, Michael Ellerman <michaele@au1.ibm.com>,
	linux-kernel@vger.kernel.org, eranian@google.com,
	Paul Mackerras <paulus@samba.org>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	dev@codyps.com, Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>,
	linuxppc-dev@lists.ozlabs.org, Ingo Molnar <mingo@kernel.org>,
	Anshuman Khandual <khandual@linux.vnet.ibm.com>
Subject: Re: [PATCH v4 01/10] tools/perf: support parsing parameterized events
Date: Wed, 1 Oct 2014 17:59:57 +0200	[thread overview]
Message-ID: <20141001155957.GD2843@worktop.programming.kicks-ass.net> (raw)
In-Reply-To: <20141001090627.GB5168@krava.brq.redhat.com>

On Wed, Oct 01, 2014 at 11:06:27AM +0200, Jiri Olsa wrote:
> On Thu, Sep 25, 2014 at 07:25:20PM -0700, Sukadev Bhattiprolu wrote:
> > Jiri Olsa [jolsa@redhat.com] wrote:
> > | On Wed, Sep 24, 2014 at 12:27:15PM -0700, Sukadev Bhattiprolu wrote:
> > | > From: Cody P Schafer <cody@linux.vnet.ibm.com>
> > | > 
> > | > Enable event specification like:
> > | > 
> > | > 	pmu/event_name,param1=0x1,param2=0x4/
> > | > 
> > | > Assuming that
> > | > 
> > | > 	/sys/bus/event_source/devices/pmu/events/event_name
> > | > 
> > | > Contains something like
> > | > 
> > | > 	param2=foo,bar=1,param1=baz
> > | 
> > | hum, so what happened to the '?' ... AFAIU from out last discussion,
> > | you wanted to mark terms which are mandatory and user must provide
> > | values for them.. and I thought the decision was to have following
> > | alias record:
> > | 
> > |   $ cat /sys/bus/event_source/devices/pmu/events/event_name
> > |   param2=?,bar=1,param1=?
> > | 
> > | while perf would scream if any of param1/2 wasnt filled like for:
> > |   pmu/event_name,param1=0x1/
> > 
> > Sorry, I meant to make perf list consistent with sysfs.
> > 
> > Consider these two sysfs entries:
> > 
> > 	$ cat HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE 
> > 	domain=0x2,offset=0xe0,starting_index=core,lpar=0x0
> > 
> > 	$ cat HPM_0THRD_NON_IDLE_CCYC__VCPU_HOME_CORE 
> > 	domain=0x3,offset=0xe0,starting_index=vcpu,lpar=sibling_guest_id
> > 
> > In the first one, starting_index refers to a 'core' while in the second
> > it refers to a vcpu. This serves as a "hint" for the parameter's meaning.
> > 
> > By replacing both with 'starting_index=?' we lose that hint.
> > 
> > Should we fix both sysfs and 'perf list' to say
> > 
> > 	starting_index=?core 
> 
> Peter, Ingo,
> any opinions on this? Overall explanation is in here:
> http://marc.info/?l=linux-kernel&m=141158688307356&w=2

Consistency is good, and you indeed need to indicate it is a parameter.
I'm not entirely sure about ?core, but I suppose its easy to parse and
clear enough to read.

So the typical optional argument syntax would be like $arg or <type>
like. But overall I have no objection as long as you keep the lot
consistent and parsable.

WARNING: multiple messages have this Message-ID (diff)
From: Peter Zijlstra <peterz@infradead.org>
To: Jiri Olsa <jolsa@redhat.com>
Cc: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>,
	Arnaldo Carvalho de Melo <acme@kernel.org>,
	ak@linux.intel.com, eranian@google.com,
	Paul Mackerras <paulus@samba.org>,
	dev@codyps.com, Michael Ellerman <michaele@au1.ibm.com>,
	Anshuman Khandual <khandual@linux.vnet.ibm.com>,
	hbabu@us.ibm.com, linux-kernel@vger.kernel.org,
	linuxppc-dev@lists.ozlabs.org, Ingo Molnar <mingo@kernel.org>
Subject: Re: [PATCH v4 01/10] tools/perf: support parsing parameterized events
Date: Wed, 1 Oct 2014 17:59:57 +0200	[thread overview]
Message-ID: <20141001155957.GD2843@worktop.programming.kicks-ass.net> (raw)
In-Reply-To: <20141001090627.GB5168@krava.brq.redhat.com>

On Wed, Oct 01, 2014 at 11:06:27AM +0200, Jiri Olsa wrote:
> On Thu, Sep 25, 2014 at 07:25:20PM -0700, Sukadev Bhattiprolu wrote:
> > Jiri Olsa [jolsa@redhat.com] wrote:
> > | On Wed, Sep 24, 2014 at 12:27:15PM -0700, Sukadev Bhattiprolu wrote:
> > | > From: Cody P Schafer <cody@linux.vnet.ibm.com>
> > | > 
> > | > Enable event specification like:
> > | > 
> > | > 	pmu/event_name,param1=0x1,param2=0x4/
> > | > 
> > | > Assuming that
> > | > 
> > | > 	/sys/bus/event_source/devices/pmu/events/event_name
> > | > 
> > | > Contains something like
> > | > 
> > | > 	param2=foo,bar=1,param1=baz
> > | 
> > | hum, so what happened to the '?' ... AFAIU from out last discussion,
> > | you wanted to mark terms which are mandatory and user must provide
> > | values for them.. and I thought the decision was to have following
> > | alias record:
> > | 
> > |   $ cat /sys/bus/event_source/devices/pmu/events/event_name
> > |   param2=?,bar=1,param1=?
> > | 
> > | while perf would scream if any of param1/2 wasnt filled like for:
> > |   pmu/event_name,param1=0x1/
> > 
> > Sorry, I meant to make perf list consistent with sysfs.
> > 
> > Consider these two sysfs entries:
> > 
> > 	$ cat HPM_0THRD_NON_IDLE_CCYC__PHYS_CORE 
> > 	domain=0x2,offset=0xe0,starting_index=core,lpar=0x0
> > 
> > 	$ cat HPM_0THRD_NON_IDLE_CCYC__VCPU_HOME_CORE 
> > 	domain=0x3,offset=0xe0,starting_index=vcpu,lpar=sibling_guest_id
> > 
> > In the first one, starting_index refers to a 'core' while in the second
> > it refers to a vcpu. This serves as a "hint" for the parameter's meaning.
> > 
> > By replacing both with 'starting_index=?' we lose that hint.
> > 
> > Should we fix both sysfs and 'perf list' to say
> > 
> > 	starting_index=?core 
> 
> Peter, Ingo,
> any opinions on this? Overall explanation is in here:
> http://marc.info/?l=linux-kernel&m=141158688307356&w=2

Consistency is good, and you indeed need to indicate it is a parameter.
I'm not entirely sure about ?core, but I suppose its easy to parse and
clear enough to read.

So the typical optional argument syntax would be like $arg or <type>
like. But overall I have no objection as long as you keep the lot
consistent and parsable.

  reply	other threads:[~2014-10-01 16:00 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-09-24 19:27 [PATCH v4 00/10] Add support for parameterized events from sysfs Sukadev Bhattiprolu
2014-09-24 19:27 ` Sukadev Bhattiprolu
2014-09-24 19:27 ` [PATCH v4 01/10] tools/perf: support parsing parameterized events Sukadev Bhattiprolu
2014-09-24 19:27   ` Sukadev Bhattiprolu
2014-09-25  8:58   ` Jiri Olsa
2014-09-25  8:58     ` Jiri Olsa
2014-09-26  2:25     ` Sukadev Bhattiprolu
2014-09-26  2:25       ` Sukadev Bhattiprolu
2014-10-01  9:06       ` Jiri Olsa
2014-10-01  9:06         ` Jiri Olsa
2014-10-01 15:59         ` Peter Zijlstra [this message]
2014-10-01 15:59           ` Peter Zijlstra
2014-09-24 19:27 ` [PATCH v4 02/10] tools/perf: extend format_alias() to include event parameters Sukadev Bhattiprolu
2014-09-24 19:27   ` Sukadev Bhattiprolu
2014-09-24 19:27 ` [PATCH v4 03/10] perf: provide sysfs_show for struct perf_pmu_events_attr Sukadev Bhattiprolu
2014-09-24 19:27   ` Sukadev Bhattiprolu
2014-09-24 19:27 ` [PATCH v4 04/10] powerpc/perf/hv-24x7: parse catalog and populate sysfs with events Sukadev Bhattiprolu
2014-09-24 19:27   ` Sukadev Bhattiprolu
2014-09-24 19:27 ` [PATCH v4 05/10] perf: add PMU_EVENT_ATTR_STRING() helper Sukadev Bhattiprolu
2014-09-24 19:27   ` Sukadev Bhattiprolu
2014-09-24 19:27 ` [PATCH v4 06/10] powerpc/perf/{hv-gpci, hv-common}: generate requests with counters annotated Sukadev Bhattiprolu
2014-09-24 19:27   ` [PATCH v4 06/10] powerpc/perf/{hv-gpci,hv-common}: " Sukadev Bhattiprolu
2014-09-24 19:27 ` [PATCH v4 07/10] powerpc/perf/hv-gpci: add the remaining gpci requests Sukadev Bhattiprolu
2014-09-24 19:27   ` Sukadev Bhattiprolu
2014-09-24 19:27 ` [PATCH v4 08/10] perf Documentation: add event parameters Sukadev Bhattiprolu
2014-09-24 19:27   ` Sukadev Bhattiprolu
2014-09-24 19:27 ` [PATCH v4 09/10] tools/perf: Document parameterized and symbolic events Sukadev Bhattiprolu
2014-09-24 19:27   ` Sukadev Bhattiprolu
2014-09-24 19:27 ` [PATCH v4 10/10] powerpc/perf/hv-24x7: Document sysfs event description entries Sukadev Bhattiprolu
2014-09-24 19:27   ` Sukadev Bhattiprolu
2014-09-30 21:03   ` Cody P Schafer
2014-09-30 21:03     ` Cody P Schafer

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=20141001155957.GD2843@worktop.programming.kicks-ass.net \
    --to=peterz@infradead.org \
    --cc=acme@kernel.org \
    --cc=ak@linux.intel.com \
    --cc=dev@codyps.com \
    --cc=eranian@google.com \
    --cc=jolsa@redhat.com \
    --cc=khandual@linux.vnet.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=michaele@au1.ibm.com \
    --cc=mingo@kernel.org \
    --cc=paulus@samba.org \
    --cc=sukadev@linux.vnet.ibm.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: link
Be 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.