[Fuego] [PATCH 1/6] Functional.pi_tests: fix error in intel up2 board

Li, Xiaoming lixm.fnst at cn.fujitsu.com
Thu Jul 19 06:56:33 UTC 2018


Hi Tim:

Thx for your review.

 I will  pay more attention on  patch's readability , coding style, and  portability.

And I will  make a optimization and  submit it later.

Patches bellows are the same.


Best regards
Li

-----Original Message-----
From: Tim.Bird at sony.com [mailto:Tim.Bird at sony.com] 
Sent: Thursday, July 19, 2018 8:23 AM
To: Li, Xiaoming/李 霄鸣; fuego at lists.linuxfoundation.org
Subject: RE: [Fuego] [PATCH 1/6] Functional.pi_tests: fix error in intel up2 board

Comments and questions below.

> -----Original Message-----
> From: Li Xiaoming
> 
> Details: modify the variable kernel.sched_rt_runtime_us

I would like more description in the commit message about why this patch is setting this variable.

Specifically, why does this variable need to be set?  Why does this patch only set the variable for x86_64?  Do other architectures not have the same problem with whatever issue this is solving?

> 
> Signed-off-by: Li Xiaoming <lixm.fnst at cn.fujitsu.com>
> ---
>  engine/tests/Functional.pi_tests/fuego_test.sh | 5 +++++
>  engine/tests/Functional.pi_tests/spec.json     | 2 +-
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/engine/tests/Functional.pi_tests/fuego_test.sh
> b/engine/tests/Functional.pi_tests/fuego_test.sh
> index 8df76d3..023d83f 100755
> --- a/engine/tests/Functional.pi_tests/fuego_test.sh
> +++ b/engine/tests/Functional.pi_tests/fuego_test.sh
> @@ -15,7 +15,10 @@ function test_deploy {
>      put pi_stress  $BOARD_TESTDIR/fuego.$TESTDIR/  }
> 
> +TMPFILE="$LOGDIR/sched_rt_runtime_us"
> +TMPFILE2="$BOARD_TESTDIR/fuego.$TESTDIR/sched_rt_runtime_us"
It might be easier to follow the code if we named these something like LOG_TMPFILE and BOARD_TMPFILE

>  function test_run {
> +    if [ $ARCHITECTURE == "x86_64" ]; then report "cat
Why does this only apply to x86_64?  Is this just an example of you being conservative and not wanting to affect other architectures, or is there some reason this change is not appropriate for other architectures.  In general we should try to avoid cpu-specific operations, if possible.

> /proc/sys/kernel/sched_rt_runtime_us > $TMPFILE2; sysctl -w 
> kernel.sched_rt_runtime_us=-1"; get $TMPFILE2 $TMPFILE; fi;

This is hard to read because it is all on one line.  It might read easier using shell continuation lines, like so:
if [ $ARCHITECTURE == "x86_64" ]; then
   report "cat /proc/sys/kernel/sched_rt_runtime_us >$BOARD_TMPFILE ; \
        sysctl -s kernel.schedule_rt_runtime_us=-1" ;
  get $BOARD_TMPFILE $LOG_TMPFILE;
fi

Also, are you sure we need to do this using the "report" function?
Is it important that the output from sysctl be captures in the test log?
It seems like it would be better to use 'cmd' here.

Also, why are you saving the value to the log directory (in $LOG_TMPFILE)?
Are you trying to preserve the original value for information' sake?

>      report "cd $BOARD_TESTDIR/fuego.$TESTDIR; ./pi_stress 
> $FUNCTIONAL_PI_TESTS_PARAMS"
>  }
> 
> @@ -29,5 +32,7 @@ function test_processing {
>      log_compare "$TESTDIR" "1" "^Total inversion performed" "p"
>      if [ $? -ne 0 ]; then RETURN_VALUE=1; fi
> 
> +    if [ $ARCHITECTURE == "x86_64" ]; then rt_runtime=$(cat 
> + $TMPFILE);
> report "sysctl -w kernel.sched_rt_runtime_us=$rt_runtime"; fi;
> +
This is a cleanup operation that should be performed in test_run.
It should be moved to the end of that routine, immediately after the execution of pi_stress.  The goal should be to leave the machine in an altered state for as short a time as possible.

>      return $RETURN_VALUE
>  }
> diff --git a/engine/tests/Functional.pi_tests/spec.json
> b/engine/tests/Functional.pi_tests/spec.json
> index 96f51a4..263096d 100644
> --- a/engine/tests/Functional.pi_tests/spec.json
> +++ b/engine/tests/Functional.pi_tests/spec.json
> @@ -16,7 +16,7 @@
>          "latest": {
>              "PER_JOB_BUILD": "true",
>              "gitrepo": "https://git.kernel.org/pub/scm/utils/rt-tests/rt-tests.git",
> -            "gitref": "unstable/devel/v1.1.1",
> +            "gitref": "unstable/devel/latest",
>              "PARAMS": "--duration=60"
>          }
>      }
> --
> 2.7.4

I appreciate the patch, but would like some clarification on my questions before applying it.

Thanks,
 -- Tim







More information about the Fuego mailing list