perf: allow events to be specified as an iterable

perf: allow events to be specified as an iterable

Limiting the events list to be a comma-separated string doesn't work
for the more complex syntaxes perf now supports for things like kprobes;
allow the list to be given as an iterable (while still supporting a
string) and pass each using a separate -e argument

Change-Id: I18cea36095cb4a9f93c3b9da9a8a77cb66da0686
Signed-off-by: Scott James Remnant <keybuk@chromium.org>;

BUG=chromiumos:13826
TEST=Yes

[ericli, 2011-04-07 23:19:57]

Hi,

Can you send these two patches to autotest@test.kernel.org and CC me? I 'd
like to get these two changes upstream first and then rebase them back.

Yes, I will help you to commit them in upstream.

Eric

On Thu, Apr 7, 2011 at 3:11 PM, <keybuk@chromium.org> wrote:

> Reviewers: ericli, Benson Leung,
>
> Description:
> perf: allow events to be specified as an iterable
>
> Limiting the events list to be a comma-separated string doesn't work
> for the more complex syntaxes perf now supports for things like kprobes;
> allow the list to be given as an iterable (while still supporting a
> string) and pass each using a separate -e argument
>
> Change-Id: I18cea36095cb4a9f93c3b9da9a8a77cb66da0686
> Signed-off-by: Scott James Remnant <keybuk@chromium.org>
>
> BUG=chromiumos:13826
> TEST=Yes
>
> Please review this at http://codereview.chromium.org/6816032/
>
> SVN Base: ssh://git@gitrw.chromium.org:9222/autotest.git@master
>
> Affected files:
>  M client/profilers/perf/perf.py
>
>
> Index: client/profilers/perf/perf.py
> diff --git a/client/profilers/perf/perf.py b/client/profilers/perf/perf.py
> index
>
211d562bdaea058341afad4fcbda3b65ec0e3664..2bd2da4fb6a8800ad8ec56c789b938f873b33429
> 100644
> --- a/client/profilers/perf/perf.py
> +++ b/client/profilers/perf/perf.py
> @@ -13,8 +13,12 @@ from autotest_lib.client.bin import profiler, os_dep,
> utils
>  class perf(profiler.profiler):
>     version = 1
>
> -    def initialize(self, events="cycles,instructions"):
> -        self.events = events
> +    def initialize(self, events=["cycles","instructions"]):
> +        if type(events) == str:
> +            self.events = [events]
> +        else:
> +            self.events = events
> +        self.trace = trace
>         self.perf_bin = os_dep.command('perf')
>         perf_help = utils.run('%s report help' % self.perf_bin,
>                               ignore_status=True).stderr
> @@ -31,8 +35,10 @@ class perf(profiler.profiler):
>
>     def start(self, test):
>         self.logfile = os.path.join(test.profdir, "perf")
> -        cmd = ("%s record -a -o %s -e %s" %
> -               (self.perf_bin, self.logfile, self.events))
> +        cmd = ("%s record -a -o %s" %
> +               (self.perf_bin, self.logfile))
> +        for event in self.events:
> +            cmd += " -e %s" % event
>         self._process = subprocess.Popen(cmd, shell=True,
>                                          stderr=subprocess.STDOUT)
>
>
>
>


-- 
Eric Li
李咏竹
Google Kirkland

[lmr_redhat.com, 2011-04-07 23:56:14]

On Thu, 2011-04-07 at 16:19 -0700, Eric Li(李咏竹) wrote:
> Hi,
> 
> 
> Can you send these two patches to autotest@test.kernel.org and CC me?
> I 'd like to get these two changes upstream first and then rebase them
> back.

I just looked at the patch, looks good to me.

It would be a bit easier if they were sent directly to the mailing list,
it makes easier for my patch check scripts.

Cheers,

Lucas

> 
> Yes, I will help you to commit them in upstream.
> 
> 
> Eric
> 
> On Thu, Apr 7, 2011 at 3:11 PM, <keybuk@chromium.org> wrote:
>         Reviewers: ericli, Benson Leung,
>         
>         Description:
>         perf: allow events to be specified as an iterable
>         
>         Limiting the events list to be a comma-separated string
>         doesn't work
>         for the more complex syntaxes perf now supports for things
>         like kprobes;
>         allow the list to be given as an iterable (while still
>         supporting a
>         string) and pass each using a separate -e argument
>         
>         Change-Id: I18cea36095cb4a9f93c3b9da9a8a77cb66da0686
>         Signed-off-by: Scott James Remnant <keybuk@chromium.org>
>         
>         BUG=chromiumos:13826
>         TEST=Yes
>         
>         Please review this at http://codereview.chromium.org/6816032/
>         
>         SVN Base:
>         ssh://git@gitrw.chromium.org:9222/autotest.git@master
>         
>         Affected files:
>          M client/profilers/perf/perf.py
>         
>         
>         Index: client/profilers/perf/perf.py
>         diff --git a/client/profilers/perf/perf.py
>         b/client/profilers/perf/perf.py
>         index
>        
211d562bdaea058341afad4fcbda3b65ec0e3664..2bd2da4fb6a8800ad8ec56c789b938f873b33429
100644
>         --- a/client/profilers/perf/perf.py
>         +++ b/client/profilers/perf/perf.py
>         @@ -13,8 +13,12 @@ from autotest_lib.client.bin import
>         profiler, os_dep, utils
>          class perf(profiler.profiler):
>             version = 1
>         
>         -    def initialize(self, events="cycles,instructions"):
>         -        self.events = events
>         +    def initialize(self, events=["cycles","instructions"]):
>         +        if type(events) == str:
>         +            self.events = [events]
>         +        else:
>         +            self.events = events
>         +        self.trace = trace
>                 self.perf_bin = os_dep.command('perf')
>                 perf_help = utils.run('%s report help' %
>         self.perf_bin,
>                                       ignore_status=True).stderr
>         @@ -31,8 +35,10 @@ class perf(profiler.profiler):
>         
>             def start(self, test):
>                 self.logfile = os.path.join(test.profdir, "perf")
>         -        cmd = ("%s record -a -o %s -e %s" %
>         -               (self.perf_bin, self.logfile, self.events))
>         +        cmd = ("%s record -a -o %s" %
>         +               (self.perf_bin, self.logfile))
>         +        for event in self.events:
>         +            cmd += " -e %s" % event
>                 self._process = subprocess.Popen(cmd, shell=True,
>         
>          stderr=subprocess.STDOUT)
>         
>         
>         
> 
> 
> 
> -- 
> Eric Li
> 李咏竹
> Google Kirkland
> 
> 
> 
> _______________________________________________
> Autotest mailing list
> Autotest@test.kernel.org
> http://test.kernel.org/cgi-bin/mailman/listinfo/autotest

[lmr, 2011-04-08 00:36:46]

Small comment made on your patch set.

http://codereview.chromium.org/6816032/diff/3001/client/profilers/perf/perf.py
File client/profilers/perf/perf.py (right):

http://codereview.chromium.org/6816032/diff/3001/client/profilers/perf/perf.p...
client/profilers/perf/perf.py:18: self.events = [events]
If we are going to support the old way of passing events, one would pass a
string such as:

"cycles,instructions"

So in case it is a string, better split it using commas as a separator. I am
assuming the idea here is to support the old interface, right?

[lmr_redhat.com, 2011-04-08 00:37:49]

On Thu, 2011-04-07 at 20:56 -0300, Lucas Meneghel Rodrigues wrote:
> On Thu, 2011-04-07 at 16:19 -0700, Eric Li(李咏竹) wrote:
> > Hi,
> > 
> > 
> > Can you send these two patches to autotest@test.kernel.org and CC me?
> > I 'd like to get these two changes upstream first and then rebase them
> > back.
> 
> I just looked at the patch, looks good to me.
> 
> It would be a bit easier if they were sent directly to the mailing list,
> it makes easier for my patch check scripts.

I was going to apply it when I noticed a tiny, tiny thing. Replied with
comments on codereview, hope you guys got it.

> Cheers,
> 
> Lucas
> 
> > 
> > Yes, I will help you to commit them in upstream.
> > 
> > 
> > Eric
> > 
> > On Thu, Apr 7, 2011 at 3:11 PM, <keybuk@chromium.org> wrote:
> >         Reviewers: ericli, Benson Leung,
> >         
> >         Description:
> >         perf: allow events to be specified as an iterable
> >         
> >         Limiting the events list to be a comma-separated string
> >         doesn't work
> >         for the more complex syntaxes perf now supports for things
> >         like kprobes;
> >         allow the list to be given as an iterable (while still
> >         supporting a
> >         string) and pass each using a separate -e argument
> >         
> >         Change-Id: I18cea36095cb4a9f93c3b9da9a8a77cb66da0686
> >         Signed-off-by: Scott James Remnant <keybuk@chromium.org>
> >         
> >         BUG=chromiumos:13826
> >         TEST=Yes
> >         
> >         Please review this at http://codereview.chromium.org/6816032/
> >         
> >         SVN Base:
> >         ssh://git@gitrw.chromium.org:9222/autotest.git@master
> >         
> >         Affected files:
> >          M client/profilers/perf/perf.py
> >         
> >         
> >         Index: client/profilers/perf/perf.py
> >         diff --git a/client/profilers/perf/perf.py
> >         b/client/profilers/perf/perf.py
> >         index
> >        
211d562bdaea058341afad4fcbda3b65ec0e3664..2bd2da4fb6a8800ad8ec56c789b938f873b33429
100644
> >         --- a/client/profilers/perf/perf.py
> >         +++ b/client/profilers/perf/perf.py
> >         @@ -13,8 +13,12 @@ from autotest_lib.client.bin import
> >         profiler, os_dep, utils
> >          class perf(profiler.profiler):
> >             version = 1
> >         
> >         -    def initialize(self, events="cycles,instructions"):
> >         -        self.events = events
> >         +    def initialize(self, events=["cycles","instructions"]):
> >         +        if type(events) == str:
> >         +            self.events = [events]
> >         +        else:
> >         +            self.events = events
> >         +        self.trace = trace
> >                 self.perf_bin = os_dep.command('perf')
> >                 perf_help = utils.run('%s report help' %
> >         self.perf_bin,
> >                                       ignore_status=True).stderr
> >         @@ -31,8 +35,10 @@ class perf(profiler.profiler):
> >         
> >             def start(self, test):
> >                 self.logfile = os.path.join(test.profdir, "perf")
> >         -        cmd = ("%s record -a -o %s -e %s" %
> >         -               (self.perf_bin, self.logfile, self.events))
> >         +        cmd = ("%s record -a -o %s" %
> >         +               (self.perf_bin, self.logfile))
> >         +        for event in self.events:
> >         +            cmd += " -e %s" % event
> >                 self._process = subprocess.Popen(cmd, shell=True,
> >         
> >          stderr=subprocess.STDOUT)
> >         
> >         
> >         
> > 
> > 
> > 
> > -- 
> > Eric Li
> > 李咏竹
> > Google Kirkland
> > 
> > 
> > 
> > _______________________________________________
> > Autotest mailing list
> > Autotest@test.kernel.org
> > http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
> 
> 
> _______________________________________________
> Autotest mailing list
> Autotest@test.kernel.org
> http://test.kernel.org/cgi-bin/mailman/listinfo/autotest

[keybuk, 2011-04-12 17:27:05]

On Thu, Apr 7, 2011 at 5:36 PM, <lookkas@gmail.com> wrote:

> Small comment made on your patch set.
>
>
>
> http://codereview.chromium.org/6816032/diff/3001/client/profilers/perf/perf.py
> File client/profilers/perf/perf.py (right):
>
>
>
http://codereview.chromium.org/6816032/diff/3001/client/profilers/perf/perf.p...
> client/profilers/perf/perf.py:18: self.events = [events]
> If we are going to support the old way of passing events, one would pass
> a string such as:
>
> "cycles,instructions"
>
> So in case it is a string, better split it using commas as a separator.
> I am assuming the idea here is to support the old interface, right?
>
>
> http://codereview.chromium.org/6816032/
>

Well, the old way just passed the entire string, commas and all, to perf. So
I was aiming for direct compatibility

There may be some subtle differences between how perf parses that string and
the result of events.split(",")

Scott

[ericli, 2011-04-21 20:43:35]

This has been pushed into upstream and merged back to our master.

This CL should be abandoned now.