[Fuego] [PATCH] fix_Benchmark_cyclictest_with_env

[Tim.Bird at sony.com]

> -----Original Message-----
> From: Daniel Sangorrin
> 
> Hello Li,
> 
> > -----Original Message-----
> > From: Li, Xiaoming
> >
> >
> > Details: When running on intel-up2, report an error of  "Unable to change
> > scheduling policy! either run as root or join realtime group".
> > As systemd defaults put ssh connections into a non-RT cgroup,  ssh in and
> running
> > FIFO/RR tasks won't work.
This is useful information.  Thanks very much for researching this.

I have a few questions.  Does Fuego use a non-root user account to
execute tests on your intel-up2 board?

Do you know if boards that don't use systemd put ssh connections into a
non-RT cgroup?

> > To fix this, add "sysctl -w kernel.sched_rt_runtime_us=-1".   It will  move
> shell
> > into the default cpu:/ group which does permit priority scheduling.
This is strange.  Is it the kernel itself that is changing the cgroup in response to this flag
being set?  (It must be - I wouldn't expect sysctl to do anything that complicated).

> 
> That is a bit weird. There should be another way.
> The purpose of sched_rt_runtime_us is to set what percentage of cpu is left
> for non-rt tasks in case an rt-task hogs the cpu. It has nothing to do with
> permissions hummm.

I agree with Daniel here.  That flag can potentially upset the realtime configuration
of groups throughout the whole system.  Is cyclictest assumed to be an invasive
test? - that is, one that can potentially disrupt the overall realtime configuration
of the sytem?  I haven't used it a lot, but I assume that by default cyclictest
assumes that it has the full RT scheduling bandwidth of the system.
In that regard, your fix makes sense.

> # Also you are not checking if the target board is actually using systemd.
> # kernel.sched_rt_runtime_us=-1 is actually a good setting for testing
> (maybe not production) the rt performance with cyclictest
> What OS are you using?
> 
> Thanks,
> Daniel
> 
> > It looks like even the console is restricted. We have only tested it  on x86-64
> and
> > arm-64. This may only occurs on x86-64,  arm_64 has no such restriction.
> >
> > Signed-off-by: Li Xiaoming <lixm.fnst at cn.fujitsu.com>
> > ---
> >  engine/tests/Benchmark.cyclictest/fuego_test.sh | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/engine/tests/Benchmark.cyclictest/fuego_test.sh
> > b/engine/tests/Benchmark.cyclictest/fuego_test.sh
> > index 319f361..edf37c8 100755
> > --- a/engine/tests/Benchmark.cyclictest/fuego_test.sh
> > +++ b/engine/tests/Benchmark.cyclictest/fuego_test.sh
> > @@ -16,5 +16,16 @@ function test_deploy {  }
> >
> >  function test_run {
> > +    LOG_TMPFILE="$LOGDIR/sched_rt_runtime_us"
> > +
> >
> BOARD_TMPFILE="$BOARD_TESTDIR/fuego.$TESTDIR/sched_rt_runtime_us
> "
> > +    if [ $ARCHITECTURE == "x86_64" ]; then
> > +        cmd "cat /proc/sys/kernel/sched_rt_runtime_us > $BOARD_TMPFILE;
> \
> > +            sysctl -w kernel.sched_rt_runtime_us=-1";
> > +        get $BOARD_TMPFILE $LOG_TMPFILE;
> > +    fi
> >      report "cd $BOARD_TESTDIR/fuego.$TESTDIR; ./cyclictest
> > $BENCHMARK_CYCLICTEST_PARAMS"
> > +    if [ $ARCHITECTURE == "x86_64" ]; then
> > +        cmd "sysctl -w kernel.sched_rt_runtime_us=$(cat $LOG_TMPFILE)";
> > +    fi
> > +
> >  }
> > --
> > 2.7.4

The structure of the fix is OK, based on the desire to save the value
of sched_rt_runtime_us, set it to -1 during the test, and then restore it.

But this has nothing to do with cgroups.  My preference would be to
use a mechanism that detected that a process was running
in an non-RT cgroup, warn the user about that, and then
switch to the cpu:/ group automatically, using some explicit group manipulation,
rather than using the side effect of setting sched_rt_runtime_us to switch the cgroup.

I would rather control the change in the patch (setting sched_rt_runtime_us to -1)
with a test parameter (specified in the spec file or board file) that indicates to allow cyclictest to
have unlimited RT runtime scheduling.

That is, something like this:
In the spec:
  "unlimited_rt_runtime":"true"
or board file:
BENCHMARK_CYCLICTEST_UNLIMITED_RUNTIME="true"

and in fuegotest.sh:
if [ "$BENCHMARK_CYCLICTEST_UNLIMITED_RUNTIME" = "true" ] ; then ...

If this is something that is needed by lots of different RT tests, we may want
to define a new board variable, that is independent of this specific test (Benchmark.cyclictest).

Let me know what you think.
 -- Tim