From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 30093 invoked by alias); 12 Jan 2016 01:43:26 -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 30080 invoked by uid 89); 12 Jan 2016 01:43:25 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.9 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD autolearn=no version=3.3.2 spammy=Hx-spam-relays-external:sk:heian.c, H*RU:sk:heian.c, H*F:D*cn.fujitsu.com, HX-HELO:sk:heian.c X-HELO: heian.cn.fujitsu.com Received: from cn.fujitsu.com (HELO heian.cn.fujitsu.com) (59.151.112.132) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 12 Jan 2016 01:43:24 +0000 Received: from unknown (HELO cn.fujitsu.com) ([10.167.33.5]) by heian.cn.fujitsu.com with ESMTP; 12 Jan 2016 09:43:15 +0800 Received: from G08CNEXCHPEKD02.g08.fujitsu.local (unknown [10.167.33.83]) by cn.fujitsu.com (Postfix) with ESMTP id F20BE45DEE1B; Tue, 12 Jan 2016 09:42:54 +0800 (CST) Received: from localhost.localdomain (10.167.226.48) by G08CNEXCHPEKD02.g08.fujitsu.local (10.167.33.89) with Microsoft SMTP Server id 14.3.181.6; Tue, 12 Jan 2016 09:42:54 +0800 Message-ID: <569459D0.5050203@cn.fujitsu.com> Date: Tue, 12 Jan 2016 01:43:00 -0000 From: =?UTF-8?B?Ilpob3UsIFdlbmppYW4v5ZGo5paH5YmRIg==?= User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: David Smith , Subject: Re: [PATCH v2] Add new case for stress test References: <1451889421-9209-1-git-send-email-zhouwj-fnst@cn.fujitsu.com> <56942599.9090401@redhat.com> In-Reply-To: <56942599.9090401@redhat.com> Content-Type: text/plain; charset="UTF-8"; format=flowed Content-Transfer-Encoding: 7bit X-yoursite-MailScanner-ID: F20BE45DEE1B.AE1F6 X-yoursite-MailScanner: Found to be clean X-yoursite-MailScanner-From: zhouwj-fnst@cn.fujitsu.com X-IsSubscribed: yes X-SW-Source: 2016-q1/txt/msg00019.txt.bz2 On 01/12/2016 05:58 AM, David Smith wrote: > 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? > load_lib should be called either here or in systemtap/testsuite/config/unix.exp. Otherwise it can't be found. Since the gen_load.exp is only used by the stress test, I didn't choose calling it in systemtap/testsuite/config/unix.exp. >> 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. > Perhaps it will always pass. But it is the same of code in stap_run.exp: if {$LOAD_GEN_FUNCTION != ""} then { #run the interesting test here if {[eval $LOAD_GEN_FUNCTION] == 0} then { pass "$TEST_NAME load generation" } else { fail "$TEST_NAME load generation" } } > 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 see. > 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. > Previously, I think it is not important whether systemtap will finish in 10 seconds or not, for starting 10 systemtap processes in 10 seconds is enough. But now, I'm confused. I not sure if it is necessary to test the parallel compiling. Maybe compiling and executing should be test separately. How do you think? >> +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? > I'm afraid you have misunderstood it. num1 is the number of [open|close] in the output by 1 systemtap process. num2 is the number of [open|close] in the output by 10 systemtap processes. 10 processes' outputs are all in one file. So if there is no problem in parallel executing, num2 should equal num1 * 10. -- Thanks Zhou