On Wednesday 18 September 2013 00:48:45 Marek Olšák wrote: > On Tue, Sep 17, 2013 at 6:43 PM, Dylan Baker <baker.dyla...@gmail.com> wrote: > > On Monday 16 September 2013 20:08:35 Marek Olšák wrote: > >> The Radeon driver writes GPU page faults to dmesg and we need to know > >> which > >> test caused them. > >> > >> If there is any change in dmesg during a test run, the test result is > >> changed as follows: > >> * pass -> dmesg-warn > >> * warn -> dmesg-warn > >> * fail -> dmesg-fail > >> Dmesg is captured before and after the test and the difference between > >> the > >> two is stored in the test summary. > >> > >> The piglit-run.py parameter which enables this behavior is --dmesg. It's > >> also recommended to use -c0. > >> --- > >> > >> framework/core.py | 5 ++-- > >> framework/exectest.py | 64 > >> > >> +++++++++++++++++++++++++++++++++++++++++----- > > > > framework/shader_test.py | > > > >> 4 +-- > >> framework/summary.py | 26 ++++++++++++++----- > >> piglit-run.py | 6 ++++- > >> templates/index.css | 6 ++++- > >> templates/test_result.mako | 7 ++++- > >> 7 files changed, 98 insertions(+), 20 deletions(-) > >> > >> diff --git a/framework/core.py b/framework/core.py > >> index 150a70c..9efae63 100644 > >> --- a/framework/core.py > >> +++ b/framework/core.py > >> > >> @@ -364,13 +364,14 @@ class TestrunResult: > >> class Environment: > >> def __init__(self, concurrent=True, execute=True, include_filter=[], > >> > >> - exclude_filter=[], valgrind=False): > >> > >> + exclude_filter=[], valgrind=False, dmesg=False): > >> self.concurrent = concurrent > >> self.execute = execute > >> self.filter = [] > >> self.exclude_filter = [] > >> self.exclude_tests = set() > >> self.valgrind = valgrind > >> > >> + self.dmesg = dmesg > >> > >> """ > >> The filter lists that are read in should be a list of string > >> > >> objects, @@ -448,7 +449,7 @@ class Test: > >> try: > >> status("running") > >> time_start = time.time() > >> > >> - result = self.run(env.valgrind) > >> + result = self.run(env.valgrind, env.dmesg)'' > > > > Instead of passing env.valgrind and env.dmesg, let's just pass env as a > > single argument here and sort it out in the Test.run method. > > Will do. > > >> time_end = time.time() > >> > >> if 'time' not in result: > >> result['time'] = time_end - time_start > >> > >> diff --git a/framework/exectest.py b/framework/exectest.py > >> index 6ee550c..2e9f0a5 100644 > >> --- a/framework/exectest.py > >> +++ b/framework/exectest.py > >> > >> @@ -35,8 +35,48 @@ if 'PIGLIT_PLATFORM' in os.environ: > >> else: > >> PIGLIT_PLATFORM = '' > >> > >> - > >> -# ExecTest: A shared base class for tests that simply run an executable. > >> +def read_dmesg(): > >> + try: > >> + proc = subprocess.Popen('dmesg', > >> + stdout=subprocess.PIPE, > >> + universal_newlines=True) > >> + out, err = proc.communicate() > >> + except OSError as e: > >> + return None > >> + return out > >> + > >> +def get_last_dmesg_timestamp(text): > >> + for i in reversed(range(0, len(text)-1)): > >> + if text[i] == '\n': > >> + line = text[i+1:] > >> + for right_bracket, char in enumerate(line): > >> + if char == ']': > >> + return line[:right_bracket+1] > >> + return None > >> + return None > >> + > >> +def get_dmesg_diff(old, new): > >> + ts = get_last_dmesg_timestamp(old) > >> + if ts == None: > >> + return '' > >> + > >> + start = 0 > >> + end = len(new)-1 > >> + > >> + while True: > >> + pos = new.find(ts, start, end) > >> + if pos == -1: > >> + break > >> + start = pos+len(ts) > >> + > >> + pos = new.find('\n', start, end) > >> + if (pos == -1): > >> + return '' > >> + start = pos+1 > >> + return new[start:end] > > > > I have a couple of problems with this section: > > 1) Please leave some comments, I'm having trouble following what you're > > trying to do here > > 2) This feels really hacky. I'd really like to see a more elegent solution > > to reading dmesg. What about a class that reads dmesg in the constructor, > > and has a method that either returns false or new entries to dmesg since > > it was called. > > It's dumb structured programming. I'm not a python guru, so that's > what I came up with. I'll try to add a class for it. >
While I think a class would be more elegant, I'd settle for some comments and polish. Looking at this I'm certain it can be optimized and polished as-is, but without a good understanding of what's going in and what's supposed to come out I can't help. > >> + > >> + > >> +# ExecTest: A shared base class for tests that simply runs an > >> executable. > >> > >> class ExecTest(Test): > >> def __init__(self, command): > >> Test.__init__(self) > >> > >> @@ -49,11 +89,11 @@ class ExecTest(Test): > >> self.skip_test = self.check_for_skip_scenario(command) > >> > >> - def interpretResult(self, out, returncode, results): > >> > >> + def interpretResult(self, out, returncode, results, dmesg): > >> raise NotImplementedError > >> return out > >> > >> - def run(self, valgrind): > >> > >> + def run(self, valgrind, dmesg): > >> """ > >> Run a test. The return value will be a dictionary with keys > >> including 'result', 'info', 'returncode' and 'command'. > >> > >> @@ -69,6 +109,7 @@ class ExecTest(Test): > >> if self.command is not None: > >> command = self.command > >> > >> + dmesg_diff = '' > >> > >> if valgrind: > >> command[:0] = ['valgrind', '--quiet', > >> '--error-exitcode=1', > >> > >> @@ -81,8 +122,13 @@ class ExecTest(Test): > >> err = "" > >> returncode = None > >> > >> else: > >> + old_dmesg = '' > >> + if dmesg: > >> + old_dmesg = read_dmesg() > >> > >> (out, err, returncode) = \ > >> > >> self.get_command_result(command, fullenv) > >> > >> + if dmesg: > >> + dmesg_diff = get_dmesg_diff(old_dmesg, > >> read_dmesg()) > >> > >> # https://bugzilla.gnome.org/show_bug.cgi?id=680214 is > >> # affecting many developers. If we catch it > >> > >> @@ -117,7 +163,7 @@ class ExecTest(Test): > >> results['result'] = 'skip' > >> > >> else: > >> results['result'] = 'fail' > >> > >> - out = self.interpretResult(out, returncode, results) > >> + out = self.interpretResult(out, returncode, results, > >> dmesg_diff) > >> > >> crash_codes = [ > >> > >> # Unix: terminated by a signal > >> > >> @@ -161,6 +207,7 @@ class ExecTest(Test): > >> err, out) > >> > >> results['returncode'] = returncode > >> results['command'] = ' '.join(self.command) > >> > >> + results['dmesg'] = dmesg_diff > >> > >> self.handleErr(results, err) > >> > >> @@ -217,11 +264,16 @@ class PlainExecTest(ExecTest): > >> # Prepend testBinDir to the path. > >> self.command[0] = testBinDir + self.command[0] > >> > >> - def interpretResult(self, out, returncode, results): > >> > >> + def interpretResult(self, out, returncode, results, dmesg): > >> outlines = out.split('\n') > >> outpiglit = map(lambda s: s[7:], > >> > >> filter(lambda s: s.startswith('PIGLIT:'), > >> > >> outlines)) > >> > >> + if dmesg != '': > >> + outpiglit = map(lambda s: s.replace("'pass'", > >> "'dmesg-warn'"), > >> outpiglit) + outpiglit = map(lambda s: s.replace("'warn'", > >> "'dmesg-warn'"), outpiglit) + outpiglit = map(lambda s: > >> s.replace("'fail'", "'dmesg-fail'"), outpiglit) + > >> > >> if len(outpiglit) > 0: > >> try: > >> for piglit in outpiglit: > >> diff --git a/framework/shader_test.py b/framework/shader_test.py > >> index b80af24..c91c22e 100755 > >> --- a/framework/shader_test.py > >> +++ b/framework/shader_test.py > >> > >> @@ -243,7 +243,7 @@ class ShaderTest(PlainExecTest): > >> self.__command = [runner] + self.__shader_runner_args > >> return self.__command > >> > >> - def run(self, valgrind=False): > >> > >> + def run(self, valgrind=False, dmesg=False): > >> """ Parse the test file's [require] block to determine which > >> executable is needed to run the test. Then run the executable on > >> > >> the test file.""" > >> > >> @@ -259,7 +259,7 @@ class ShaderTest(PlainExecTest): > >> # parsing the test file discovered an error. > >> return self.__result > >> > >> - return PlainExecTest.run(self, valgrind) > >> + return PlainExecTest.run(self, valgrind, dmesg) > >> > >> def _usage_error(): > > This is going to break every test class that you haven't updated that > > descends from ExecTest, so the IGTTest class in tests/igt.tests, GTFTest > > in gtf.tests and es3conform.tests, and OGLCTest in oglconform.tests. > > Those will need to be fixed > > Will fix. > > >> diff --git a/framework/summary.py b/framework/summary.py > >> index 1cdbab7..5ce327c 100644 > >> --- a/framework/summary.py > >> +++ b/framework/summary.py > >> > >> @@ -325,10 +325,14 @@ class Summary: > >> return 2 > >> > >> elif status == 'warn': > >> return 3 > >> > >> - elif status == 'fail': > >> > >> + elif status == 'dmesg-warn': > >> return 4 > >> > >> - elif status == 'crash': > >> > >> + elif status == 'fail': > >> return 5 > >> > >> + elif status == 'dmesg-fail': > >> + return 6 > >> + elif status == 'crash': > >> + return 7 > >> > >> openGroup('fake') > >> openGroup('all') > >> > >> @@ -421,12 +425,16 @@ class Summary: > >> return 1 > >> > >> elif status == 'warn': > >> return 2 > >> > >> - elif status == 'fail': > >> > >> + elif status == 'dmesg-warn': > >> return 3 > >> > >> - elif status == 'skip': > >> > >> + elif status == 'fail': > >> return 4 > >> > >> - elif status == 'crash': > >> > >> + elif status == 'dmesg-fail': > >> return 5 > >> > >> + elif status == 'skip': > >> + return 6 > >> + elif status == 'crash': > >> + return 7 > >> > >> elif status == 'special': > >> return 0 > > > > Is there really a reason to treat dmesg-<status> and <status> seperately? > > a > > dmesg-fail is a fail right, so why not jus thave something like: > > > > elif status in ['fail', 'dmesg-fail']: > > return 3 > > > > This will make the necissary changes much less invasive, and make the code > > simpler. > > Will do. > > >> @@ -450,7 +458,7 @@ class Summary: > >> # If the result contains a value other than 1 (pass) > >> or > >> > >> 4 # (skip) it is a problem. Skips are not problems becasuse # they have > >> Their own page. > >> - if [i for e in [2, 3, 5] for i in status if e is i]: > >> + if [i for e in [2, 3, 4, 5, 7] for i in status if e > >> is > >> i]: self.tests['problems'].append(test) > >> > >> if 'skipped' in lists: > >> @@ -480,7 +488,8 @@ class Summary: > >> Private: Find the total number of pass, fail, crash, skip, and > >> warn > >> > >> in the *last* set of results stored in self.results. > >> > >> """ > >> > >> - self.totals = {'pass': 0, 'fail': 0, 'crash': 0, 'skip': 0, > >> 'warn': 0} + self.totals = {'pass': 0, 'fail': 0, 'crash': 0, > >> 'skip': 0, 'warn': 0, + 'dmesg-warn': 0, > >> 'dmesg-fail': 0}>> > >> for test in self.results[-1].tests.values(): > >> self.totals[test['result']] += 1 > >> > >> @@ -547,6 +556,7 @@ class Summary: > >> info=value.get('info', 'None'), > >> traceback=value.get('traceback', 'None'), > >> command=value.get('command', 'None'), > >> > >> + dmesg=value.get('dmesg', 'None'), > >> > >> css=path.relpath(resultCss, tPath), > >> index=path.relpath(index, tPath))) > >> > >> file.close() > >> > >> @@ -625,6 +635,8 @@ class Summary: > >> print " crash: %d" % self.totals['crash'] > >> print " skip: %d" % self.totals['skip'] > >> print " warn: %d" % self.totals['warn'] > >> > >> + print " dmesg-warn: %d" % self.totals['dmesg-warn'] > >> + print " dmesg-fail: %d" % self.totals['dmesg-fail'] > >> > >> if self.tests['changes']: > >> print " changes: %d" % len(self.tests['changes']) > >> print " fixes: %d" % len(self.tests['fixes']) > >> > >> diff --git a/piglit-run.py b/piglit-run.py > >> index 7945b21..169c621 100755 > >> --- a/piglit-run.py > >> +++ b/piglit-run.py > >> > >> @@ -72,6 +72,9 @@ def main(): > >> parser.add_argument("--valgrind", > >> > >> action="store_true", > >> help="Run tests in valgrind's memcheck") > >> > >> + parser.add_argument("--dmesg", > >> + action="store_true", > >> + help="Capture a difference in dmesg before and > >> after each test") parser.add_argument("testProfile", > >> > >> metavar="<Path to test profile>", > >> help="Path to testfile to run") > >> > >> @@ -108,7 +111,8 @@ def main(): > >> exclude_filter=args.exclude_tests, > >> include_filter=args.include_tests, > >> execute=args.execute, > >> > >> - valgrind=args.valgrind) > >> + valgrind=args.valgrind, > >> + dmesg=args.dmesg) > >> > >> # Change working directory to the root of the piglit directory > >> piglit_dir = path.dirname(path.realpath(sys.argv[0])) > >> > >> diff --git a/templates/index.css b/templates/index.css > >> index 875333f..577370c 100644 > >> --- a/templates/index.css > >> +++ b/templates/index.css > >> @@ -36,7 +36,7 @@ td:first-child > div { > >> > >> background-color: #c8c838 > >> > >> } > >> > >> -td.skip, td.warn, td.fail, td.pass, td.trap, td.abort, td.crash { > >> +td.skip, td.warn, td.fail, td.pass, td.trap, td.abort, td.crash, > >> td.dmesg-warn, td.dmesg-fail { text-align: right; > >> > >> } > >> > >> @@ -59,9 +59,13 @@ tr:nth-child(even) td.skip { background-color: > >> #a0a0a0; > >> } > >> > >> tr:nth-child(odd) td.warn { background-color: #ff9020; } > >> tr:nth-child(even) td.warn { background-color: #ef8010; } > >> > >> +tr:nth-child(odd) td.dmesg-warn { background-color: #ff9020; } > >> +tr:nth-child(even) td.dmesg-warn { background-color: #ef8010; } > >> > >> tr:nth-child(odd) td.fail { background-color: #ff2020; } > >> tr:nth-child(even) td.fail { background-color: #e00505; } > >> > >> +tr:nth-child(odd) td.dmesg-fail { background-color: #ff2020; } > >> +tr:nth-child(even) td.dmesg-fail { background-color: #e00505; } > >> > >> tr:nth-child(odd) td.trap { background-color: #111111; } > >> tr:nth-child(even) td.trap { background-color: #000000; } > >> > >> diff --git a/templates/test_result.mako b/templates/test_result.mako > >> index 410dbb4..b23fb8e 100644 > >> --- a/templates/test_result.mako > >> +++ b/templates/test_result.mako > >> @@ -46,7 +46,12 @@ > >> > >> <pre>${traceback}</pre> > >> > >> </td> > >> > >> </tr> > >> > >> - > >> + <tr> > >> + <td>dmesg</td> > >> + <td> > >> + <pre>${dmesg}</pre> > >> + </td> > >> + </tr> > >> > >> </table> > >> <p><a href="${index}">Back to summary</a></p> > >> > >> </body> > > > > I have some additional reservations about this patch: > > First, how do you know that the change in dmesg is actually relevant? for > > example my dmesg is full of entries becasue I have the misfortune of > > having > > and ALPS touchpad and it just spams useless information to dmesg. I dont' > > see anything that's actually checking to make sure that the entries are > > relevant. > We could change the piglit parameter to --dmesg=regexp, but as I said > earlier, it's not a priority for me right now. I'm not asking you add this but: I'd rather see the regex baked into the module in the same way the regex for ignoring output errors is. > > > Second this patch feels unpolished in many places, and has the felling of: > > 'I got it working, I'm done' in others. > > I kind of answered this one above. > > > Finally, I'm also rather concerned that this would be unreliable with > > concurrency, since there's no way to know which of the threads running at > > the same time triggered an entry in dmesg. If that is the case, turning > > dmesg on should either forcably turn concurrency off, or raise an error > > if it doesn't. > I decided to leave this to the user. It's obvious that concurrency can > cause some false negatives and allowing it isn't really foolproof, but > I didn't want to disallow it completely. I might actually use both > concurrency and dmesg logging if the concurrency weren't broken with > python 2.7.4. > > Marek
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Piglit mailing list Piglit@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/piglit