From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 19444 invoked by alias); 11 Jan 2016 21:58:59 -0000 Mailing-List: contact systemtap-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: systemtap-owner@sourceware.org Received: (qmail 19374 invoked by uid 89); 11 Jan 2016 21:58:59 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-3.2 required=5.0 tests=AWL,BAYES_00,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=zhou, readers, remind, sk:systemt X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Mon, 11 Jan 2016 21:58:53 +0000 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (Postfix) with ESMTPS id 427FF8F4E5; Mon, 11 Jan 2016 21:58:52 +0000 (UTC) Received: from t540p.usersys.redhat.com (vpn-49-21.rdu2.redhat.com [10.10.49.21]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u0BLwnC9028606 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Mon, 11 Jan 2016 16:58:50 -0500 Subject: Re: [PATCH v2] Add new case for stress test To: Zhou Wenjian , systemtap@sourceware.org References: <1451889421-9209-1-git-send-email-zhouwj-fnst@cn.fujitsu.com> From: David Smith Message-ID: <56942599.9090401@redhat.com> Date: Mon, 11 Jan 2016 21:58:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <1451889421-9209-1-git-send-email-zhouwj-fnst@cn.fujitsu.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2016-q1/txt/msg00018.txt.bz2 On 01/04/2016 12:37 AM, Zhou Wenjian wrote: > * testsuite/lib/gen_load.exp: New test file > * testsuite/systemtap.stress/all_kernel_functions.exp: Use gen_load.exp > * testsuite/systemtap.stress/current.exp: Use gen_load.exp > * testsuite/systemtap.stress/parallel_exec.exp: New test case > --- > testsuite/lib/gen_load.exp | 19 ++++++++++ > .../systemtap.stress/all_kernel_functions.exp | 16 +-------- > testsuite/systemtap.stress/current.exp | 19 ++-------- > testsuite/systemtap.stress/parallel_exec.exp | 42 ++++++++++++++++++++++ > 4 files changed, 64 insertions(+), 32 deletions(-) > create mode 100644 testsuite/lib/gen_load.exp > create mode 100644 testsuite/systemtap.stress/parallel_exec.exp > > diff --git a/testsuite/lib/gen_load.exp b/testsuite/lib/gen_load.exp > new file mode 100644 > index 0000000..43b9df1 > --- /dev/null > +++ b/testsuite/lib/gen_load.exp > @@ -0,0 +1,19 @@ > +# genload is used by stress test > +proc gen_load {} { > + # if 'genload' from the ltp exists, use it to create a real load > + catch {exec which genload 2>/dev/null} genload > + if {![file executable $genload]} { > + set genload {/usr/local/ltp/testcases/bin/genload} > + } > + if [file executable $genload] { > + exec $genload -c 10 -i 10 -m 10 -t 10 > + # ^^^^^ run for 10 seconds > + # ^^^^^ 10 procs spinning on malloc > + # ^^^^^ 10 procs spinning on sync > + # ^^^^^ 10 procs spinning on sqrt > + } else { > + # sleep for a bit > + wait_n_secs 10 > + } > + return 0 > +} I know you didn't write the code in gen_load.exp, but I'd modify the comment before 'wait_n_secs' to remind readers that wait_n_secs runs 'stress' if available. > diff --git a/testsuite/systemtap.stress/all_kernel_functions.exp b/testsuite/systemtap.stress/all_kernel_functions.exp > index d7c2fbc..d546cfc 100644 > --- a/testsuite/systemtap.stress/all_kernel_functions.exp > +++ b/testsuite/systemtap.stress/all_kernel_functions.exp > @@ -1,18 +1,4 @@ > -proc genload {} { > - # if 'genload' from the ltp exists, use it to create a real load > - set genload {/usr/local/ltp/testcases/bin/genload} > - if [file executable $genload] { > - exec $genload -c 10 -i 10 -m 10 -t 10 > - # ^^^^^ run for 10 seconds > - # ^^^^^ 10 procs spinning on malloc > - # ^^^^^ 10 procs spinning on sync > - # ^^^^^ 10 procs spinning on sqrt > - } else { > - # sleep for a bit > - wait_n_secs 10 > - } > - return 0 > -} > +load_lib "gen_load.exp" OK, this change (calling 'load_lib') I don't get. That shouldn't be necessary. All the code in testsuite/lib should be available to all testcases with calling load_lib. Did you find this necessary? > proc probe_ok {probepoint} { > set cmd {exec stap -p2 -e} > diff --git a/testsuite/systemtap.stress/current.exp b/testsuite/systemtap.stress/current.exp > index 186baa3..83814b6 100644 > --- a/testsuite/systemtap.stress/current.exp > +++ b/testsuite/systemtap.stress/current.exp > @@ -2,23 +2,8 @@ > # function, install it, and get some output. > > set test "current" > - > -proc current_load {} { > - # if 'genload' from the ltp exists, use it to create a real load > - set genload {/usr/local/ltp/testcases/bin/genload} > - if [file executable $genload] { > - exec $genload -c 10 -i 10 -m 10 -t 10 > - # ^^^^^ run for 10 seconds > - # ^^^^^ 10 procs spinning on malloc > - # ^^^^^ 10 procs spinning on sync > - # ^^^^^ 10 procs spinning on sqrt > - } else { > - # sleep for a bit > - wait_n_secs 10 > - } > - return 0 > -} > +load_lib "gen_load.exp" Ditto comment about calling 'load_lib' here. > set output_string "(\\w+ = \\d+\r\n){5}(${all_pass_string}){2}(WARNING.*skipped.*)?" > > -stap_run $srcdir/$subdir/$test.stp current_load $output_string -g -w > +stap_run $srcdir/$subdir/$test.stp gen_load $output_string -g -w > diff --git a/testsuite/systemtap.stress/parallel_exec.exp b/testsuite/systemtap.stress/parallel_exec.exp > new file mode 100644 > index 0000000..9b6769e > --- /dev/null > +++ b/testsuite/systemtap.stress/parallel_exec.exp > @@ -0,0 +1,42 @@ > +#!/bin/expect -f > + > +set test "parallel execute" > + > +load_lib "gen_load.exp" > + > +set process_num 10 > + > +set script { > + probe syscall.open > + { > + if (pid() != target()) next > + log("open") > + } > + > + probe syscall.close > + { > + if (pid() != target()) next > + log("close") > + } > +} > + > +for { set i 0 } { $i < $process_num } { incr i } { > + exec stap -e "$script" -c "cat /etc/hosts > /dev/null" >>/tmp/parallel_exec & > +} > + > +if {[eval gen_load] == 0} then { > + pass "$test load generation" > +} else { > + fail "$test load generation" > +} I'm not sure I understand the above. As far as I can see, gen_load always returns a 0, so this test will always pass. In addition, I'd remove /tmp/parallel_exec before you start, otherwise you might be appending to a file from the previous run. You might think about giving your temporary file a unique name using mktemp. I think you are really hoping there that waiting 10 seconds will be enough time for all the systemtap scripts to finish. I'm not sure you can depend on that, especially on a slow system like an arm/arm64. One thing that would help here is to precompile your script (stap -p4 -e '$script") before your run of 10, so that all 10 can use the precompiled systemtap module from the cache. > +set num1 [exec stap -e "$script" -c "cat /etc/hosts > /dev/null" | grep \[open|close\] | wc -l] > +set num2 [exec grep \[open|close\] /tmp/parallel_exec | wc -l] I'm not sure I understand this logic, perhaps I'm missing something. I'd think we'd see num1 as 10 (since there are 10 invocations of stap). Why are you grepping for open|close for num1? I'd think we'd you'd want to see num2 as 20 (10 lines of 'open' and 10 lines of 'close'), but I'd guess that your regexp is going to find the stap invocation lines also. Why not grep for something like '^(open|close)$'? > + > +exec rm -f /tmp/parallel_exec > + > +if {$num1 * $process_num != $num2} { > + fail $test > +} else { > + pass $test > +} I'm lost on your math here. I'd think you'd want num1 as 10 (10 invocations of stap) and num2 as 20 (10 lines of 'open', 10 lines of 'close'), but num1 (10) * process_num (10) == 100, which isn't close to num2 (20). How do you see this working? -- David Smith dsmith@redhat.com Red Hat http://www.redhat.com 256.217.0141 (direct) 256.837.0057 (fax)