[Fuego] [PATCH 3/3] backfire: Add a new test of the rt-tests

Bird, Timothy Tim.Bird at sony.com
Sat Jan 27 01:58:55 UTC 2018



> -----Original Message-----
> From: Hoang Van Tuyen on  Wednesday, January 24, 2018 5:44 PM
> The test (sendme program) uses the backfire driver to send a signal from
> driver to user. It calculates the time intervals to call the driver and
> to receive the signal from the driver.
> The backfire driver source code is very old and have several problems.
> We have two patches for the driver. The driver was tested on 3.16 and
> 4.14 kernel header version.

OK - this is pretty interesting.  This is our first test that uses a loadable kernel
module.

> Signed-off-by: Hoang Van Tuyen <tuyen.hoangvan at toshiba-tsdv.com>
> ---
>   ...dify-including-libraries-for-supporting-m.patch | 58 ++++++++++++++++
>   ...re-Fix-copying-data-to-and-from-userspace.patch | 81
> ++++++++++++++++++++++
>   engine/tests/Benchmark.backfire/chart_config.json  |  5 ++
>   engine/tests/Benchmark.backfire/criteria.json      | 26 +++++++
>   engine/tests/Benchmark.backfire/fuego_test.sh      | 36 ++++++++++
>   engine/tests/Benchmark.backfire/parser.py          | 23 ++++++
>   engine/tests/Benchmark.backfire/reference.json     | 26 +++++++
>   engine/tests/Benchmark.backfire/spec.json          | 14 ++++
>   8 files changed, 269 insertions(+)
>   create mode 100644
> engine/tests/Benchmark.backfire/0001-backfire-Modify-including-libraries-
> for-supporting-m.patch
>   create mode 100644
> engine/tests/Benchmark.backfire/0002-backfire-Fix-copying-data-to-and-
> from-userspace.patch
>   create mode 100644 engine/tests/Benchmark.backfire/chart_config.json
>   create mode 100644 engine/tests/Benchmark.backfire/criteria.json
>   create mode 100755 engine/tests/Benchmark.backfire/fuego_test.sh
>   create mode 100755 engine/tests/Benchmark.backfire/parser.py
>   create mode 100644 engine/tests/Benchmark.backfire/reference.json
>   create mode 100644 engine/tests/Benchmark.backfire/spec.json
> 
> diff --git
> a/engine/tests/Benchmark.backfire/0001-backfire-Modify-including-
> libraries-for-supporting-m.patch
> b/engine/tests/Benchmark.backfire/0001-backfire-Modify-including-
> libraries-for-supporting-m.patch
> new file mode 100644
> index 0000000..010d900
> --- /dev/null
> +++
> b/engine/tests/Benchmark.backfire/0001-backfire-Modify-including-
> libraries-for-supporting-m.patch
> @@ -0,0 +1,58 @@
> +From 6ff2b16636286167287c4774feeb47fbcc569c28 Mon Sep 17 00:00:00
> 2001
> +From: Hoang Van Tuyen <tuyen.hoangvan at toshiba-tsdv.com>
> +Date: Thu, 18 Jan 2018 11:25:46 +0700
> +Subject: [PATCH 1/2] backfire: Modify including libraries for supporting
> + modern kernels.
> +
> +The backfire driver is very old. The latest commit was from 2009.
> +Ex: It does not compile since 2.6.39-rc1 when SPIN_LOCK_UNLOCKED was
> +killed in d04fa5a3ba06.
> +The asm/system.h was removed in Linux 3.4, commit f05e798ad4c0.
> +This commit corrects using the libraries and tested on 4.14 and 3.16
> +kernel versions
> +
> +Signed-off-by: Hoang Van Tuyen <tuyen.hoangvan at toshiba-tsdv.com>
> +---
> + src/backfire/backfire.c | 12 +++++++++---
> + 1 file changed, 9 insertions(+), 3 deletions(-)
> +
> +diff --git a/src/backfire/backfire.c b/src/backfire/backfire.c
> +index aaf9c4a..e05f43d 100644
> +--- a/src/backfire/backfire.c
> ++++ b/src/backfire/backfire.c
> +@@ -20,23 +20,29 @@
> +  */
> +
> + #include <linux/module.h>
> ++#include <linux/version.h>
> +
> ++#if LINUX_VERSION_CODE < KERNEL_VERSION(4,11,0)
> + #include <linux/sched.h>
> ++#else
> ++#include <linux/sched/signal.h>
> ++#endif
> + #include <linux/cpumask.h>
> + #include <linux/time.h>
> +-#include <linux/smp_lock.h>
> + #include <linux/types.h>
> + #include <linux/errno.h>
> + #include <linux/miscdevice.h>
> + #include <linux/proc_fs.h>
> + #include <linux/spinlock.h>
> +
> +-#include <asm/uaccess.h>
> ++#include <linux/uaccess.h>
> ++#if LINUX_VERSION_CODE < KERNEL_VERSION(3,4,0)
> + #include <asm/system.h>
> ++#endif
> +
> + #define BACKFIRE_MINOR MISC_DYNAMIC_MINOR
> +
> +-static spinlock_t backfire_state_lock = SPIN_LOCK_UNLOCKED;
> ++static DEFINE_SPINLOCK(backfire_state_lock);
> + static int backfire_open_cnt; /* #times opened */
> + static int backfire_open_mode; /* special open modes */
> + static struct timeval sendtime; /* when the most recent signal was sent */
> +--
> +2.1.4
> +

git complained about this trailing newline.  Please remove it.

> diff --git
> a/engine/tests/Benchmark.backfire/0002-backfire-Fix-copying-data-to-and-
> from-userspace.patch
> b/engine/tests/Benchmark.backfire/0002-backfire-Fix-copying-data-to-and-
> from-userspace.patch
> new file mode 100644
> index 0000000..03aa196
> --- /dev/null
> +++
> b/engine/tests/Benchmark.backfire/0002-backfire-Fix-copying-data-to-and-
> from-userspace.patch
> @@ -0,0 +1,81 @@
> +From f8950a126420e3240233473cf5e235486aa3dc56 Mon Sep 17 00:00:00
> 2001
> +From: Hoang Van Tuyen <tuyen.hoangvan at toshiba-tsdv.com>
> +Date: Thu, 18 Jan 2018 13:49:30 +0700
> +Subject: [PATCH 2/2] backfire: Fix copying data to and from userspace.
> +
> +Use copy_from_user and copy_to_user functions for copying between
> +kernel and user space. This commit is based on a patch from Michele
> Dionisio:
> +https://www.spinics.net/lists/linux-rt-users/msg17403.html
> +
> +Signed-off-by: Hoang Van Tuyen <tuyen.hoangvan at toshiba-tsdv.com>
> +---
> + src/backfire/backfire.c | 30 +++++++++++++++++++++++++++---
> + 1 file changed, 27 insertions(+), 3 deletions(-)
> +
> +diff --git a/src/backfire/backfire.c b/src/backfire/backfire.c
> +index e05f43d..3e8dc67 100644
> +--- a/src/backfire/backfire.c
> ++++ b/src/backfire/backfire.c
> +@@ -48,6 +48,7 @@ static int backfire_open_mode; /* special open modes
> */
> + static struct timeval sendtime; /* when the most recent signal was sent */
> + #define BACKFIRE_WRITE 1 /* opened for writing (exclusive) */
> + #define BACKFIRE_EXCL 2 /* opened with O_EXCL */
> ++#define MAX_SIZE_DATA_RW 512
> +
> + /*
> +  * These are the file operation function for user access to /dev/backfire
> +@@ -55,8 +56,17 @@ static struct timeval sendtime; /* when the most
> recent signal was sent */
> + static ssize_t
> + backfire_read(struct file *file, char *buf, size_t count, loff_t *ppos)
> + {
> +-    return snprintf(buf, count, "%d,%d\n", (int) sendtime.tv_sec,
> ++    char kbuf[MAX_SIZE_DATA_RW];
> ++    ssize_t res;
> ++    if (count > MAX_SIZE_DATA_RW) {
> ++        count = MAX_SIZE_DATA_RW;
> ++    }
> ++    res = snprintf(kbuf, count, "%d,%d\n", (int) sendtime.tv_sec,
> +        (int) sendtime.tv_usec);
> ++    if (copy_to_user(buf, kbuf, res+1)){
> ++        return -EFAULT;
> ++    }
> ++    return res;
> + }
> +
> + static ssize_t
> +@@ -64,16 +74,30 @@ backfire_write(struct file *file, const char *buf,
> size_t count, loff_t *ppos)
> + {
> +    int signo;
> +    struct pid *pid;
> ++    char kbuf[MAX_SIZE_DATA_RW+1];
> ++    ssize_t retval = 0;
> ++
> ++    if (count == 0) {
> ++        return 0;
> ++    }
> ++    if (count > MAX_SIZE_DATA_RW) {
> ++        count = MAX_SIZE_DATA_RW;
> ++    }
> ++    if (copy_from_user(kbuf, buf, count)) {
> ++        return -EFAULT;
> ++    }
> ++    kbuf[count] = '\0';
> +
> +-    if (sscanf(buf, "%d", &signo) >= 1) {
> ++    if (sscanf(kbuf, "%d", &signo) >= 1) {
> +        if (signo > 0 && signo < 32) {
> +            pid = get_pid(task_pid(current));
> +            do_gettimeofday(&sendtime);
> +            kill_pid(pid, signo, 1);
> ++            retval = strlen(kbuf);
> +        } else
> +            printk(KERN_ERR "Invalid signal no. %d\n", signo);
> +    }
> +-    return strlen(buf);
> ++    return retval;
> + }
> +
> + static int
> +--
> +2.1.4
> +

Please remove this trailing newline also.

> diff --git a/engine/tests/Benchmark.backfire/chart_config.json
> b/engine/tests/Benchmark.backfire/chart_config.json
> new file mode 100644
> index 0000000..3cbaef2
> --- /dev/null
> +++ b/engine/tests/Benchmark.backfire/chart_config.json
> @@ -0,0 +1,5 @@
> +{
> +    "chart_type": "measure_plot",
> +    "measures": ["default.intervals.max_interval",
> +        "default.intervals.avg_interval"]
> +}
> diff --git a/engine/tests/Benchmark.backfire/criteria.json
> b/engine/tests/Benchmark.backfire/criteria.json
> new file mode 100644
> index 0000000..c6d3395
> --- /dev/null
> +++ b/engine/tests/Benchmark.backfire/criteria.json
> @@ -0,0 +1,26 @@
> +{
> +    "schema_version":"1.0",
> +    "criteria":[
> +        {
> +            "tguid":"default.intervals.max_interval",
> +            "reference":{
> +                "value":100,
> +                "operator":"le"
> +            }
> +        },
> +        {
> +            "tguid":"default.intervals.min_interval",
> +            "reference":{
> +                "value":100,
> +                "operator":"le"
> +            }
> +        },
> +        {
> +            "tguid":"default.intervals.avg_interval",
> +            "reference":{
> +                "value":100,
> +                "operator":"le"
> +            }
> +        }
> +    ]
> +}
> diff --git a/engine/tests/Benchmark.backfire/fuego_test.sh
> b/engine/tests/Benchmark.backfire/fuego_test.sh
> new file mode 100755
> index 0000000..37f7403
> --- /dev/null
> +++ b/engine/tests/Benchmark.backfire/fuego_test.sh
> @@ -0,0 +1,39 @@
> +tarball=../rt-tests/rt-tests-v1.1.1.tar.gz
> +
> +NEED_ROOT=1
> +
> +function test_pre_check {
> +    assert_define BENCHMARK_SENDME_PARAMS
> +    if [ ! "$ARCHITECTURE" = "x86_64" ]; then
> +        abort_job "Currently, This test is just available on x86_64
> architecture"

Is there something about the backfire module that is architecture-specific?
I know that your other Dockerfile patch only adds support for the
Linux-headers for the host architecture (presumably x86_64. 
But if an SDK for some other platform provides kernel header files,
is there any problem with compiling and loading the backfire module?

Also, you should check for 'insmod' here.  Most platforms will have
it, but some might not.  That is a dependency that we should capture
in test_pre_check.

> +    fi
> +}
> +
> +function test_build {
> +    patch -p1 -N -s <
> $TEST_HOME/../rt-tests/0001-Add-scheduling-policies-for-old-kernels.patch
> +    make NUMA=0 sendme
> +
> +    # Build the backfire driver
> +    patch -p1 -N -s <
> $TEST_HOME/0001-backfire-Modify-including-libraries-for-supporting-
> m.patch
> +    patch -p1 -N -s <
> $TEST_HOME/0002-backfire-Fix-copying-data-to-and-from-userspace.patch
> +    cd ./src/backfire/ || abort_job "./src/backfire directory does not
> exist"
> +    make
> +    [ -f backfire.ko ] || abort_job "Cannot build backfire driver"
> +    cd -
> +}
> +
> +function test_deploy {
> +    put sendme  $BOARD_TESTDIR/fuego.$TESTDIR/
> +    put ./src/backfire/backfire.ko $BOARD_TESTDIR/fuego.$TESTDIR/
> +}
> +
> +function test_run {
> +    # sendme does not support a option for printing a summary only on exit.
> +    # So, We get the three lines at the end of the command's output as a
> +    # summary of the report.
> +    report "cd $BOARD_TESTDIR/fuego.$TESTDIR; insmod ./backfire.ko;
> ./sendme $BENCHMARK_SENDME_PARAMS | tail -3"

I mentioned doing a pre-test check for insmod.  I'm not sure about tail.
Hmmm.  It looks like a few other tests use tail.  But they all use
the -n syntax.  ... Checking busybox, it does not support the
syntax 'tail  -<num_lines>', but rather requires 'tail -n <num_lines>'.
Please use 'tail -n 3' here.
(and don't worry about checking for tail.  I'm going to add it to
the list of commands required on the board).

Daniel - do you agree with this, or should we not have Fuego require 'tail',
and have individual tests check for it before using it?  It's a pretty standard command,
and I don't know of many busybox instances that don't have it compiled in.

On the other hand, where we're using it could probably be replaced with
host-side processing.

Let me know what you think.

> +}
> +
> +function test_cleanup {
> +    cmd "rmmod backfire &> /dev/null"
> +}
Nice.  good cleanup. 

However we should also check for rmmod in test_pre_check.

> diff --git a/engine/tests/Benchmark.backfire/parser.py
> b/engine/tests/Benchmark.backfire/parser.py
> new file mode 100755
> index 0000000..e8416f1
> --- /dev/null
> +++ b/engine/tests/Benchmark.backfire/parser.py
> @@ -0,0 +1,23 @@
> +#!/usr/bin/python
> +import os, re, sys
> +sys.path.insert(0, os.environ['FUEGO_CORE'] + '/engine/scripts/parser')
> +import common as plib
> +
> +regex_string = ".* Min\s+(\d+).*, Avg\s+(\d+), Max\s+(\d+)"
> +measurements = {}
> +matches = plib.parse_log(regex_string)
> +
> +if matches:
> +    min_intervals = []
> +    avg_intervals = []
> +    max_intervals = []
> +    for thread in matches:
> +        min_intervals.append(float(thread[0]))
> +        avg_intervals.append(float(thread[1]))
> +        max_intervals.append(float(thread[2]))
> +    measurements['default.intervals'] = [
> +        {"name": "max_interval", "measure" : max(max_intervals)},
> +        {"name": "min_interval", "measure" : min(min_intervals)},
> +        {"name": "avg_interval", "measure" :
> sum(avg_intervals)/len(avg_intervals)}]
> +
> +sys.exit(plib.process(measurements))
> diff --git a/engine/tests/Benchmark.backfire/reference.json
> b/engine/tests/Benchmark.backfire/reference.json
> new file mode 100644
> index 0000000..d1dd0bc
> --- /dev/null
> +++ b/engine/tests/Benchmark.backfire/reference.json
> @@ -0,0 +1,26 @@
> +{
> +    "test_sets":[
> +        {
> +            "name":"default",
> +            "test_cases":[
> +                {
> +                    "name":"intervals",
> +                    "measurements":[
> +                        {
> +                            "name":"max_interval",
> +                            "unit":"us"
> +                        },
> +                        {
> +                            "name":"min_interval",
> +                            "unit":"us"
> +                        },
> +                        {
> +                            "name":"avg_interval",
> +                            "unit":"us"
> +                        }
> +                    ]
> +                }
> +            ]
> +        }
> +    ]
> +}
> diff --git a/engine/tests/Benchmark.backfire/spec.json
> b/engine/tests/Benchmark.backfire/spec.json
> new file mode 100644
> index 0000000..3103935
> --- /dev/null
> +++ b/engine/tests/Benchmark.backfire/spec.json
> @@ -0,0 +1,14 @@
> +{
> +    "testName": "Benchmark.sendme",
> +    "specs": {
> +        "default": {
> +            "PARAMS": "-a -p99 -l100"
> +        },
> +        "latest": {
> +            "PER_JOB_BUILD": "true",
> +            "gitrepo":
> "https://git.kernel.org/pub/scm/utils/rt-tests/rt-tests.git",
> +            "gitref": "unstable/devel/v1.1.1",
> +            "PARAMS": "-a -p99 -l100"
> +        }
> +    }

If we only ever use one set of PARAMS, do we need to include them here?
Are you planning on adding additional specs for this test, with other parameter
values?

> +}
> --
> 2.1.4

Please revise as indicated, and see my comments on the Dockerfile patch.
We may need to discuss some things before this is accepted.
 -- Tim



More information about the Fuego mailing list