Re: [PATCH v2] Add new case for stress test

[David Smith <dsmith at redhat dot com>]

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)