From: Cyril Hrubis <chrubis@suse.cz>
To: ltp@lists.linux.it
Subject: [LTP] [PATCH v2] syscalls/ioprio: Add test cases for I/O priority
Date: Thu, 18 Jul 2019 12:09:12 +0200 [thread overview]
Message-ID: <20190718100848.GA24931@rei.lan> (raw)
In-Reply-To: <20190718073941.13153-1-linus.walleij@linaro.org>
Hi!
> The ioprio_get/ioprio_set syscalls are used primarily by the
> userspace tool "ionice" to set priority of a process, user or
> process group toward the I/O (block layer) scheduler.
>
> This adds a simple iprio_get test and two more verbose tests
> for ioprio_set.
Generally looks fine, some comments below.
> Cc: Anders Roxell <anders.roxell@linaro.org>
> Cc: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Use the new test fixture "tst_test.h"
> - Fix up legalese in the Makefile to a simple SPDX tag.
> - Test from v2.6.13 when the syscall was introduced
> ---
> testcases/kernel/syscalls/ioprio/.gitignore | 3 +
> testcases/kernel/syscalls/ioprio/Makefile | 6 +
> testcases/kernel/syscalls/ioprio/ioprio.h | 44 ++++++
> .../kernel/syscalls/ioprio/ioprio_get01.c | 48 ++++++
> .../kernel/syscalls/ioprio/ioprio_set01.c | 98 ++++++++++++
> .../kernel/syscalls/ioprio/ioprio_set02.c | 142 ++++++++++++++++++
> 6 files changed, 341 insertions(+)
> create mode 100644 testcases/kernel/syscalls/ioprio/.gitignore
> create mode 100644 testcases/kernel/syscalls/ioprio/Makefile
> create mode 100644 testcases/kernel/syscalls/ioprio/ioprio.h
> create mode 100644 testcases/kernel/syscalls/ioprio/ioprio_get01.c
> create mode 100644 testcases/kernel/syscalls/ioprio/ioprio_set01.c
> create mode 100644 testcases/kernel/syscalls/ioprio/ioprio_set02.c
You are missing runtest entries here, see runtest/syscalls
> diff --git a/testcases/kernel/syscalls/ioprio/.gitignore b/testcases/kernel/syscalls/ioprio/.gitignore
> new file mode 100644
> index 000000000000..890081d8c625
> --- /dev/null
> +++ b/testcases/kernel/syscalls/ioprio/.gitignore
> @@ -0,0 +1,3 @@
> +ioprio_get01
> +ioprio_set01
> +ioprio_set02
> diff --git a/testcases/kernel/syscalls/ioprio/Makefile b/testcases/kernel/syscalls/ioprio/Makefile
> new file mode 100644
> index 000000000000..5ea7d67db123
> --- /dev/null
> +++ b/testcases/kernel/syscalls/ioprio/Makefile
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +
> +top_srcdir ?= ../../../..
> +
> +include $(top_srcdir)/include/mk/testcases.mk
> +include $(top_srcdir)/include/mk/generic_leaf_target.mk
> diff --git a/testcases/kernel/syscalls/ioprio/ioprio.h b/testcases/kernel/syscalls/ioprio/ioprio.h
> new file mode 100644
> index 000000000000..3a0f068a053a
> --- /dev/null
> +++ b/testcases/kernel/syscalls/ioprio/ioprio.h
> @@ -0,0 +1,44 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2019 Linus Walleij <linus.walleij@linaro.org>
> + */
> +
> +#ifndef LTP_IOPRIO_H
> +#define LTP_IOPRIO_H
> +
> +enum {
> + IOPRIO_CLASS_NONE = 0,
> + IOPRIO_CLASS_RT,
> + IOPRIO_CLASS_BE,
> + IOPRIO_CLASS_IDLE,
> +};
> +
> +enum {
> + IOPRIO_WHO_PROCESS = 1,
> + IOPRIO_WHO_PGRP,
> + IOPRIO_WHO_USER,
> +};
> +
> +#define IOPRIO_CLASS_SHIFT (13)
> +#define IOPRIO_PRIO_MASK ((1UL << IOPRIO_CLASS_SHIFT) - 1)
> +
> +#define IOPRIO_PRIO_CLASS(data) ((data) >> IOPRIO_CLASS_SHIFT)
> +#define IOPRIO_PRIO_LEVEL(data) ((data) & IOPRIO_PRIO_MASK)
> +#define IOPRIO_PRIO_VALUE(class, data) (((class) << IOPRIO_CLASS_SHIFT) | data)
> +
> +static const char *to_class_str[] = {
> + [IOPRIO_CLASS_NONE] = "NONE",
> + [IOPRIO_CLASS_RT] = "REALTIME",
> + [IOPRIO_CLASS_BE] = "BEST-EFFORT",
> + [IOPRIO_CLASS_IDLE] = "IDLE"
> +};
> +
> +/* Priority range from 0 (highest) to 7 (lowest) */
> +static inline int prio_in_range(int prio)
> +{
> + if ((prio < 0) || (prio > 7))
> + return 0;
> + return 1;
> +}
> +
> +#endif
> diff --git a/testcases/kernel/syscalls/ioprio/ioprio_get01.c b/testcases/kernel/syscalls/ioprio/ioprio_get01.c
> new file mode 100644
> index 000000000000..79807763a5f3
> --- /dev/null
> +++ b/testcases/kernel/syscalls/ioprio/ioprio_get01.c
> @@ -0,0 +1,48 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2019 Linaro Limited
> + */
We usually write a short comment paragraph about the test purpose here.
Given that this test is very basic it should be something as:
/*
* Basic ioprio_get() test. Gets the current process I/O priority and
* checks that the values are sane.
*/
> +#include <sys/types.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +#include <string.h>
> +#include <signal.h>
> +#include <unistd.h>
> +#include <sys/syscall.h>
> +
> +#include "tst_test.h"
> +#include "ioprio.h"
> +
> +static int ioprio_get(int which, int who)
> +{
> + return syscall(SYS_ioprio_get, which, who);
> +}
These wrappers should go to the ioprio.h header so that we don't have to
repeat then in each test.
> +static void run(void)
> +{
> + int prio, class;
> +
> + /* Get the I/O priority for the current process */
> + TEST(ioprio_get(IOPRIO_WHO_PROCESS, 0));
> +
> + if (TST_RET == -1) {
> + tst_res(TFAIL | TTERRNO, "ioprio_get failed");
> + return;
> + }
> +
> + class = IOPRIO_PRIO_CLASS(TST_RET);
> + prio = IOPRIO_PRIO_LEVEL(TST_RET);
> +
> + if (!prio_in_range(prio)) {
> + tst_res(TFAIL, "ioprio out of range (%d)", prio);
> + return;
> + }
Shouldn't we check that the class is valid as well here?
If we got garbage in the result we still have more than half of the bits
in the integer that will overflow the to_class_str array below.
> + tst_res(TPASS, "ioprio_get returned class %s prio %d",
> + to_class_str[class], prio);
> +}
> +
> +static struct tst_test test = {
> + .test_all = run,
> + .min_kver = "2.6.13",
> +};
> diff --git a/testcases/kernel/syscalls/ioprio/ioprio_set01.c b/testcases/kernel/syscalls/ioprio/ioprio_set01.c
> new file mode 100644
> index 000000000000..9f2c9f4b3d48
> --- /dev/null
> +++ b/testcases/kernel/syscalls/ioprio/ioprio_set01.c
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2019 Linus Walleij <linus.walleij@linaro.org>
> + */
Here as well, comment please.
> +#include <sys/types.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +#include <string.h>
> +#include <signal.h>
> +#include <unistd.h>
> +#include <sys/syscall.h>
> +
> +#include "tst_test.h"
> +#include "ioprio.h"
> +
> +static int ioprio_get(int which, int who)
> +{
> + return syscall(SYS_ioprio_get, which, who);
> +}
> +
> +static int ioprio_set(int which, int who, int ioprio)
> +{
> + return syscall(SYS_ioprio_set, which, who, ioprio);
> +}
> +
> +static void ioprio_check_setting(int class, int prio)
> +{
> + int res;
> + int newclass, newprio;
> +
> + res = ioprio_get(IOPRIO_WHO_PROCESS, 0);
> + if (res == -1) {
> + tst_res(TFAIL | TTERRNO,
> + "reading back prio failed");
> + return;
> + }
> +
> + newclass = IOPRIO_PRIO_CLASS(res);
> + newprio = IOPRIO_PRIO_LEVEL(res);
> + if (newclass != class)
> + tst_res(TFAIL,
> + "wrong class after setting, expected %s got %s",
> + to_class_str[class],
> + to_class_str[newclass]);
> + else if (newprio != prio)
> + tst_res(TFAIL,
> + "wrong prio after setting, expected %d got %d",
> + prio, newprio);
> + else
> + tst_res(TPASS, "ioprio_set new class %s, new prio %d",
> + to_class_str[newclass],
> + newprio);
> +}
> +
> +static void run(void)
> +{
> + int class, prio;
> +
> + /* Get the I/O priority for the current process */
> + TEST(ioprio_get(IOPRIO_WHO_PROCESS, 0));
> +
> + if (TST_RET == -1) {
> + tst_res(TFAIL | TTERRNO, "ioprio_get failed");
> + /* Try to continue anyway */
> + class = IOPRIO_CLASS_NONE;
> + prio = 4;
> + } else {
> + class = IOPRIO_PRIO_CLASS(TST_RET);
> + prio = IOPRIO_PRIO_LEVEL(TST_RET);
> + tst_res(TPASS, "ioprio_get returned class %s prio %d",
> + to_class_str[class], prio);
> + }
> +
> + /* Bump prio to what it was + 1 */
> + class = IOPRIO_CLASS_BE;
> + prio++;
Looking into a documentation the default io_nice for IOPRIO_CLASS_BE is
(cpu_nice + 20)/5 so the default is 4 and +1 and -1 is safe operation on
default settings. But I guess that a call to prio_in_range(prio+1) to
check the value wouldn't harm.
> + TEST(ioprio_set(IOPRIO_WHO_PROCESS, 0,
> + IOPRIO_PRIO_VALUE(class, prio)));
> + if (TST_RET == -1)
> + tst_res(TFAIL | TTERRNO, "ioprio_set failed");
> + else
> + ioprio_check_setting(class, prio);
> +
> + /* Bump prio down two notches */
> + prio -= 2;
Here as well.
> + TEST(ioprio_set(IOPRIO_WHO_PROCESS, 0,
> + IOPRIO_PRIO_VALUE(class, prio)));
> + if (TST_RET == -1)
> + tst_res(TFAIL | TTERRNO, "ioprio_set failed");
> + else
> + ioprio_check_setting(class, prio);
> +}
> +
> +static struct tst_test test = {
> + .test_all = run,
> + .min_kver = "2.6.13",
> +};
> diff --git a/testcases/kernel/syscalls/ioprio/ioprio_set02.c b/testcases/kernel/syscalls/ioprio/ioprio_set02.c
> new file mode 100644
> index 000000000000..7cec8538254d
> --- /dev/null
> +++ b/testcases/kernel/syscalls/ioprio/ioprio_set02.c
> @@ -0,0 +1,142 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2019 Linus Walleij <linus.walleij@linaro.org>
> + */
Here as well, comment please.
> +#include <sys/types.h>
> +#include <fcntl.h>
> +#include <errno.h>
> +#include <string.h>
> +#include <signal.h>
> +#include <unistd.h>
> +#include <sys/syscall.h>
> +
> +#include "tst_test.h"
> +#include "ioprio.h"
> +
> +static int ioprio_get(int which, int who)
> +{
> + return syscall(SYS_ioprio_get, which, who);
> +}
> +
> +static int ioprio_set(int which, int who, int ioprio)
> +{
> + return syscall(SYS_ioprio_set, which, who, ioprio);
> +}
> +
> +static void ioprio_check_setting(int class, int prio, int report)
> +{
> + int res;
> + int newclass, newprio;
> +
> + res = ioprio_get(IOPRIO_WHO_PROCESS, 0);
> + if (res == -1) {
> + tst_res(TFAIL | TTERRNO,
> + "reading back prio failed");
> + return;
> + }
> +
> + newclass = IOPRIO_PRIO_CLASS(res);
> + newprio = IOPRIO_PRIO_LEVEL(res);
> + if (newclass != class)
> + tst_res(TFAIL,
> + "wrong class after setting, expected %s got %s",
> + to_class_str[class],
> + to_class_str[newclass]);
> + else if (newprio != prio)
> + tst_res(TFAIL,
> + "wrong prio after setting, expected %d got %d",
> + prio, newprio);
> + else if (report)
> + tst_res(TPASS, "ioprio_set new class %s, new prio %d",
> + to_class_str[newclass],
> + newprio);
> +}
This is nearly the same as in the previous test, can we put this into
the hearder as well?
> +static void run(void)
> +{
> + int class, prio;
> + int fail_in_loop;
> +
> + /* Get the I/O priority for the current process */
> + TEST(ioprio_get(IOPRIO_WHO_PROCESS, 0));
> +
> + if (TST_RET == -1) {
> + tst_res(TFAIL | TTERRNO, "ioprio_get failed");
> + /* Try to continue anyway */
> + class = IOPRIO_CLASS_NONE;
> + prio = 4;
> + } else {
> + class = IOPRIO_PRIO_CLASS(TST_RET);
> + prio = IOPRIO_PRIO_LEVEL(TST_RET);
> + tst_res(TPASS, "ioprio_get returned class %s prio %d",
> + to_class_str[class], prio);
> + }
Why do we get the priority here? We set both the class and prio down
there.
> + /* Bump to best effort scheduling, all 8 priorities */
> + class = IOPRIO_CLASS_BE;
> +
> + fail_in_loop = 0;
> + for (prio = 0; prio < 8; prio++) {
> + TEST(ioprio_set(IOPRIO_WHO_PROCESS, 0,
> + IOPRIO_PRIO_VALUE(class, prio)));
> + if (TST_RET == -1) {
> + tst_res(TFAIL | TTERRNO, "ioprio_set IOPRIO_CLASS_BE prio %d failed", prio);
> + fail_in_loop = 1;
> + }
> + }
> + if (!fail_in_loop)
> + tst_res(TPASS, "tested all 8 prios in class %s",
> + to_class_str[class]);
Just btw, we don't have to have the fail_in_loop variable here
(fail_in_loop) == (prio < 8)
> + /* Test to fail with prio 8, first set prio 4 */
> + ioprio_set(IOPRIO_WHO_PROCESS, 0,
> + IOPRIO_PRIO_VALUE(class, 4));
> + TEST(ioprio_set(IOPRIO_WHO_PROCESS, 0,
> + IOPRIO_PRIO_VALUE(class, 8)));
> + if (TST_RET == -1) {
> + ioprio_check_setting(class, 4, 1);
> + prio = 5;
> + }
> + else
> + tst_res(TFAIL, "ioprio_set IOPRIO_CLASS_BE prio %d should not work", prio);
That's a GNU-style else and LTP follows LKML.
Also we usually tend to put all negative tests into a separate testcase
and also assert that the errno is correct.
> + /* Bump down to idle scheduling */
> + class = IOPRIO_CLASS_IDLE;
> +
> + fail_in_loop = 0;
> + for (prio = 0; prio < 8; prio++) {
> + TEST(ioprio_set(IOPRIO_WHO_PROCESS, 0,
> + IOPRIO_PRIO_VALUE(class, prio)));
> + if (TST_RET == -1) {
> + tst_res(TFAIL | TTERRNO, "ioprio_set IOPRIO_CLASS_IDLE failed");
> + fail_in_loop = 1;
> + }
> + }
> + if (!fail_in_loop)
> + tst_res(TPASS, "tested all 8 prios in class %s",
> + to_class_str[class]);
> +
> + /* Test NONE scheduling */
> + class = IOPRIO_CLASS_NONE;
> + TEST(ioprio_set(IOPRIO_WHO_PROCESS, 0,
> + IOPRIO_PRIO_VALUE(class, 0)));
> + if (TST_RET == -1)
> + tst_res(TFAIL | TTERRNO, "ioprio_set IOPRIO_CLASS_NONE failed");
> + else
> + ioprio_check_setting(class, 0, 1);
> +
> + /* Any other prio than 0 should not work with NONE */
> + class = IOPRIO_CLASS_NONE;
> + TEST(ioprio_set(IOPRIO_WHO_PROCESS, 0,
> + IOPRIO_PRIO_VALUE(class, 4)));
> + if (TST_RET == -1)
> + tst_res(TPASS, "tested illegal priority with class %s",
> + to_class_str[class]);
> + else
> + tst_res(TFAIL, "ioprio_set IOPRIO_CLASS_NONE should fail");
Here as well, we should assert the errno as well.
> +}
> +
> +static struct tst_test test = {
> + .test_all = run,
> + .min_kver = "2.6.13",
> +};
> --
> 2.21.0
>
>
> --
> Mailing list info: https://lists.linux.it/listinfo/ltp
--
Cyril Hrubis
chrubis@suse.cz
next prev parent reply other threads:[~2019-07-18 10:09 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-18 7:39 [LTP] [PATCH v2] syscalls/ioprio: Add test cases for I/O priority Linus Walleij
2019-07-18 10:09 ` Cyril Hrubis [this message]
2019-07-18 10:15 ` Cyril Hrubis
2019-07-22 9:04 ` Christian Amann
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190718100848.GA24931@rei.lan \
--to=chrubis@suse.cz \
--cc=ltp@lists.linux.it \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox