On Mon, 2010-05-03 at 10:03 -0700, Arun Sharma wrote:
> Add perf_events based counting/sampling profilers to autotest
> 
> Counting profiler:
> 
> Counts cycles, instructions and computes CPI
> 
> Sampling profiler:
> 
> Depends on a patch that enables sampling per cpu and reporting via 
> 
> --sort cpu,...
> 
> which will be mailed upstream soon.

Hi Arun, thanks for your work on those profilers, it's high time we
start exploring perf for profiling purposes. I verified that the cpistat
utility that you've contributed on this patch follows different coding
style rules than the rest of autotest, maybe because the script already
exists on its own right on another project/repository. Is that the case?
I pointed out the places where the code does not comply with the defined
autotest coding style:

http://autotest.kernel.org/browser/trunk/CODING_STYLE

> Signed-off-by: Arun Sharma <[email protected]>
> 
> --- /dev/null 2009-12-17 12:29:38.000000000 -0800
> +++ autotest/client/profilers/cpistat/control 2010-04-30 16:50:45.000000000 
> -0700
> @@ -0,0 +1,3 @@
> +job.profilers.add('cpistat')
> +job.run_test('sleeptest', seconds=10)
> +job.profilers.delete('cpistat')
> --- /dev/null 2009-12-17 12:29:38.000000000 -0800
> +++ autotest/client/profilers/cpistat/cpistat 2010-04-30 16:50:45.000000000 
> -0700
> @@ -0,0 +1,103 @@
> +#!/usr/bin/env python

^ The usual shebang for autotest scripts is
#!/usr/bin/python

> +# Copyright (c) 2010 Google, Inc.
> +# Contributed by Arun Sharma <[email protected]>
> +#
> +# Permission is hereby granted, free of charge, to any person obtaining a
> +# copy of this software and associated documentation files (the "Software"),
> +# to deal in the Software without restriction, including without limitation
> +# the rights to use, copy, modify, merge, publish, distribute, sublicense,
> +# and/or sell copies of the Software, and to permit persons to whom the
> +# Software is furnished to do so, subject to the following conditions:
> +#
> +# The above copyright notice and this permission notice shall be included
> +# in all copies or substantial portions of the Software.
> +#
> +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> +# THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> +# OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> +# ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> +# OTHER DEALINGS IN THE SOFTWARE.

^ I assume this is a BSD-like style copyright clause right? Do you have
any reservations about making this code GPL v2, as per autotest license
(just curiosity really)? Also, the whole block of information could be
put on a docstring rather than on comments. 

> +#
> +# Run as: ./cpistat -c cpulist -e eventlist
> +#
> +# Depends on libpfm4: http://perfmon2.sf.net/
> +#
> +# git://perfmon2.git.sourceforge.net/gitroot/perfmon2/libpfm4
> +
> +import sys
> +import os
> +import optparse
> +import time
> +import struct
> +import perfmon

^ Autotest coding style specifies that imports are made a little
differently than this, please check the coding style documetn, session
"Import Modules". 

> +if __name__ == '__main__':
> +    parser = optparse.OptionParser()
> +    parser.add_option("-e", "--events", help="Events to use",
> +                       action="store", dest="events")
> +    parser.add_option("-c", "--cpulist", help="CPUs to monitor",
> +                       action="store", dest="cpulist")
> +    
> parser.set_defaults(events="PERF_COUNT_HW_CPU_CYCLES,PERF_COUNT_HW_INSTRUCTIONS")
> +    (options, args) = parser.parse_args()
> +
> +    show_per_cpu = False
> +    if not options.cpulist:
> +        ncpus = os.sysconf('SC_NPROCESSORS_ONLN')
> +        cpus = range(0, ncpus)
> +    else:
> +        cpus = options.cpulist.split(',')
> +        cpus = [ int(c) for c in cpus ]
> +        show_per_cpu = True
> +
> +    if options.events:
> +        events = options.events.split(",")
> +    else:
> +        raise "You need to specify events to monitor"

^ 2 things here:

 1 - We don't do string exceptions anymore
 2 - The style for raising also is done differently

Please check the coding style document, session "Exceptions". I'd
suggest to raise a ValueError exception, like:

raise ValueError("You need to specify events to monitor")

> +    s = perfmon.SystemWideSession(cpus, events)
> +
> +    s.start()
> +    # Measuring loop
> +    interval = 1
> +    iters = -1
> +    infinite = True
> +    if len(args) == 2:
> +        interval = int(args[0])
> +        iters = int(args[1])
> +        infinite = False
> +
> +    delta = {}
> +    last = {}
> +    sum = {}
> +    for e in events:
> +        delta[e] = {}
> +        last[e] = {}
> +        sum[e] = {}
> +        for c in cpus:
> +            delta[e][c] = 0
> +            last[e][c] = 0
> +
> +    while infinite or iters:
> +        for i in range(0, len(events)):
> +          e = events[i]
> +          sum[e] = 0
> +          for c in cpus:
> +              count = struct.unpack("L", s.read(c, i))[0]
> +              delta[e][c] = count - last[e][c]
> +              last[e][c] = count
> +              if show_per_cpu:
> +                  print """CPU%d: %s\t%lu""" % (c, e, delta[e][c])
> +              sum[e] += delta[e][c]
> +
> +        cycles = sum["PERF_COUNT_HW_CPU_CYCLES"]
> +        instructions = sum["PERF_COUNT_HW_INSTRUCTIONS"]
> +        CPI = cycles * 1.0/instructions
> +        print "cycles: %12lu, instructions: %12lu, CPI: %2.4f" \
> +               % (cycles, instructions, CPI)
> +        sys.stdout.flush()
> +        time.sleep(interval)
> +        iters = iters - 1
> --- /dev/null 2009-12-17 12:29:38.000000000 -0800
> +++ autotest/client/profilers/cpistat/cpistat.py      2010-04-30 
> 16:50:45.000000000 -0700
> @@ -0,0 +1,28 @@
> +"""
> +Uses perf_events to count cycles and instructions
> +
> +Defaults options:
> +job.profilers.add('cpistat', interval=1)
> +"""
> +import time, os, subprocess
> +from autotest_lib.client.bin import profiler
> +
> +class cpistat(profiler.profiler):
> +    version = 1
> +
> +    def initialize(self, interval = 1):
> +        self.interval = interval
> +
> +
> +    def start(self, test):
> +        cmd = os.path.join(self.bindir, 'site_cpistat')
> +        if not os.path.exists(cmd):
> +           cmd = os.path.join(self.bindir, 'cpistat')
> +        logfile = open(os.path.join(test.profdir, "cpistat"), 'w')
> +        p = subprocess.Popen(cmd, stdout=logfile, \

The backslash is unnecessary                        ^

> +                             stderr=subprocess.STDOUT)
> +        self.pid = p.pid
> +
> +
> +    def stop(self, test):
> +        os.kill(self.pid, 15)
> --- /dev/null 2009-12-17 12:29:38.000000000 -0800
> +++ autotest/client/profilers/perf/control    2010-04-30 16:50:45.000000000 
> -0700
> @@ -0,0 +1,3 @@
> +job.profilers.add('perf')
> +job.run_test('sleeptest', seconds=10)
> +job.profilers.delete('perf')
> --- /dev/null 2009-12-17 12:29:38.000000000 -0800
> +++ autotest/client/profilers/perf/perf.py    2010-04-30 16:50:45.000000000 
> -0700
> @@ -0,0 +1,42 @@
> +"""
> +perf is a tool included in the linux kernel tree that
> +supports functionality similar to oprofile and more.
> +
> +More Info: http://lwn.net/Articles/310260/
> +"""

^ @see: http://lwn.net/Articles/310260/

> +import time, os, subprocess, signal
> +from autotest_lib.client.bin import profiler
> +
> +class perf(profiler.profiler):
> +    version = 1
> +
> +    def initialize(self, events="cycles,instructions"):
> +        self.events = events
> +
> +
> +    def start(self, test):
> +        self.logfile = os.path.join(test.profdir, "perf")
> +        cmd = "/usr/bin/perf record -a -o %s -e %s" % \
> +              (self.logfile, self.events)

^ With parenthesis we can use implicit line continuation, which looks
better and is generally recommended.

+        cmd = ("/usr/bin/perf record -a -o %s -e %s" %
+               (self.logfile, self.events))

> +        self._process = subprocess.Popen(cmd, shell=True, 
> stderr=subprocess.STDOUT)
> +
> +
> +    def stop(self, test):
> +        os.kill(self._process.pid, signal.SIGINT)
> +        self._process.wait()
> +
> +
> +    def report(self, test):
> +        self.reportfile_comm = os.path.join(test.profdir, 'perf.comm')
> +        cmd = "/usr/bin/perf report -i %s --sort comm,dso" % self.logfile
> +        outfile = open(self.reportfile_comm, 'w')
> +        p1 = subprocess.Popen(cmd, shell=True, stdout=outfile,
> +                              stderr=subprocess.STDOUT)
> +        p1.wait()
> +        self.reportfile_cpu = os.path.join(test.profdir, 'perf.cpu')
> +        cmd = "/usr/bin/perf report -i %s --sort cpu,dso" % self.logfile
> +        outfile = open(self.reportfile_cpu, 'w')
> +        p2 = subprocess.Popen(cmd, shell=True, stdout=outfile,
> +                              stderr=subprocess.STDOUT)
> +        p2.wait()
> _______________________________________________
> Autotest mailing list
> [email protected]
> http://test.kernel.org/cgi-bin/mailman/listinfo/autotest


_______________________________________________
Autotest mailing list
[email protected]
http://test.kernel.org/cgi-bin/mailman/listinfo/autotest

Reply via email to