On Thu, May 27, 2010 at 10:54 AM, Arun Sharma <[email protected]> wrote: > > On Tue, May 18, 2010 at 10:49 AM, Lucas Meneghel Rodrigues > <[email protected]> wrote: > > Hi Lucas, > > > > > 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: > > Thanks for looking into this. The patch to perf that adds --sort cpu > option isn't upstream yet. Once that's committed, I'll fixup the style > issues and resend. GPLv2 is fine.
Incremental patch to address your review comments attached. -Arun
Address review comments Primarily deals with coding style, licensing and consistency issues. Signed-off-by: Arun Sharma <[email protected]> --- autotest/client/profilers/cpistat/cpistat 2010-06-22 13:17:13.000000000 -0700 +++ autotest/client/profilers/cpistat/cpistat 2010-06-22 13:17:13.000000000 -0700 @@ -1,47 +1,28 @@ -#!/usr/bin/env 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. -# -# -# 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 +#!/usr/bin/python + +""" +python-libpfm4 provides python bindings to the libpfm4 +library and the perf_event kernel subsystem. This +script builds on them to provide a *stat like interface +to CPU performance counters. + +Run as: ./cpistat -c cpulist -e eventlist + +Depends on libpfm4: http://perfmon2.sf.net/ + +git://perfmon2.git.sourceforge.net/gitroot/perfmon2/libpfm4 +""" + +import sys, os, optparse, time, struct, perfmon 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") + 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 @@ -54,9 +35,9 @@ show_per_cpu = True if options.events: - events = options.events.split(",") + events = options.events.split(',') else: - raise "You need to specify events to monitor" + raise ValueError('You need to specify events to monitor') s = perfmon.SystemWideSession(cpus, events) @@ -86,18 +67,18 @@ e = events[i] sum[e] = 0 for c in cpus: - count = struct.unpack("L", s.read(c, i))[0] + 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]) + 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"] + 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) + print ('cycles: %12lu, instructions: %12lu, CPI: %2.4f' + % (cycles, instructions, CPI)) sys.stdout.flush() time.sleep(interval) iters = iters - 1 --- autotest/client/profilers/cpistat/cpistat.py 2010-06-22 13:17:13.000000000 -0700 +++ autotest/client/profilers/cpistat/cpistat.py 2010-06-22 13:17:13.000000000 -0700 @@ -19,7 +19,7 @@ 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, \ + p = subprocess.Popen(cmd, stdout=logfile, stderr=subprocess.STDOUT) self.pid = p.pid --- autotest/client/profilers/perf/perf.py 2010-06-22 13:17:13.000000000 -0700 +++ autotest/client/profilers/perf/perf.py 2010-06-22 13:17:13.000000000 -0700 @@ -2,7 +2,7 @@ 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 @@ -17,8 +17,8 @@ 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) + cmd = ("/usr/bin/perf record -a -o %s -e %s" % + (self.logfile, self.events)) self._process = subprocess.Popen(cmd, shell=True, stderr=subprocess.STDOUT)
_______________________________________________ Autotest mailing list [email protected] http://test.kernel.org/cgi-bin/mailman/listinfo/autotest
