[LTP] [PATCH v2 0/2] Improve ioprio tests

[LTP] [PATCH v2 0/2] Improve ioprio tests

[Damien Le Moal]

The ioprio syscall tests rely on ltp internal definitions of the
IOPRIO_XXX() macro defining priority classes and levels. With changes
to the ioprio API to support command duration limits, these internal
definitions are incomplete, causing test case ioprio_set03 to fail.

Avoid this issue by having the iprio tests use the kernel header file
definitions if that header exists. This enables additional checks in
the header file [1] which restores the expected results with test
ioprio_set03.

[1] https://lore.kernel.org/linux-block/20230530061307.525644-1-dlemoal@kernel.org/

Note: a review of this patch on the kernel block mailing list would be
very appreciated.

Changes from v1:
 - Added Linus's review tag
 - Resend with being subscribed to ltp list this time :)

Damien Le Moal (2):
  ioprio: use ioprio.h kernel header if it exists
  ioprio: Use IOPRIO_PRIO_NUM to check prio range

 configure.ac                                  |  1 +
 testcases/kernel/syscalls/ioprio/ioprio.h     | 33 ++++++++++++++-----
 .../kernel/syscalls/ioprio/ioprio_set03.c     |  2 +-
 3 files changed, 26 insertions(+), 10 deletions(-)

-- 
2.40.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v2 1/2] ioprio: use ioprio.h kernel header if it exists

[Damien Le Moal]

For the ioprio system call test cases, avoid blindly defining the
IOPRIO_XXX macro internally and instead use the kernel user API header
file if it exists. Given that the definitions in this header file have
changed over time, make sure to test for the existence of the macro
IOPRIO_PRIO_LEVEL macro and define it if it does not exist. Similarly,
use IOPRIO_NR_LEVELS to define IOPRIO_PRIO_NUM if that macro exists.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 configure.ac                              |  1 +
 testcases/kernel/syscalls/ioprio/ioprio.h | 29 +++++++++++++++++------
 2 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/configure.ac b/configure.ac
index 548288310..e4aa2cadf 100644
--- a/configure.ac
+++ b/configure.ac
@@ -56,6 +56,7 @@ AC_CHECK_HEADERS_ONCE([ \
     linux/if_ether.h \
     linux/if_packet.h \
     linux/io_uring.h \
+    linux/ioprio.h \
     linux/keyctl.h \
     linux/mempolicy.h \
     linux/module.h \
diff --git a/testcases/kernel/syscalls/ioprio/ioprio.h b/testcases/kernel/syscalls/ioprio/ioprio.h
index c74380475..6ca134a54 100644
--- a/testcases/kernel/syscalls/ioprio/ioprio.h
+++ b/testcases/kernel/syscalls/ioprio/ioprio.h
@@ -6,6 +6,12 @@
 #ifndef LTP_IOPRIO_H
 #define LTP_IOPRIO_H
 
+#ifdef HAVE_LINUX_IOPRIO_H
+
+# include <linux/ioprio.h>
+
+#else
+
 enum {
 	IOPRIO_CLASS_NONE = 0,
 	IOPRIO_CLASS_RT,
@@ -19,15 +25,24 @@ enum {
 	IOPRIO_WHO_USER,
 };
 
-/* The I/O scheduler classes have 8 priorities 0..7 except for the IDLE class */
-#define IOPRIO_PRIO_NUM		8
+# 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_VALUE(class, data)	(((class) << IOPRIO_CLASS_SHIFT) | data)
 
-#define IOPRIO_CLASS_SHIFT	(13)
-#define IOPRIO_PRIO_MASK	((1UL << IOPRIO_CLASS_SHIFT) - 1)
+#endif
+
+/* The RT and BE I/O priority classes have 8 priority levels 0..7 */
+#ifdef IOPRIO_NR_LEVELS
+# define IOPRIO_PRIO_NUM		IOPRIO_NR_LEVELS
+#else
+# define IOPRIO_PRIO_NUM		8
+#endif
 
-#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)
+#ifndef IOPRIO_PRIO_LEVEL
+# define IOPRIO_PRIO_LEVEL(data)	((data) & IOPRIO_PRIO_MASK)
+#endif
 
 static const char * const to_class_str[] = {
 	[IOPRIO_CLASS_NONE] = "NONE",
-- 
2.40.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

[LTP] [PATCH v2 2/2] ioprio: Use IOPRIO_PRIO_NUM to check prio range

[Damien Le Moal]

Use the macro IOPRIO_PRIO_NUM in prio_in_range() to check the upper
bound of the valid range for priority levels. Similarly, in the test
case ioprio_set03, use this macro to check for failures when the user
attempts using a priority level out of range.

Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
---
 testcases/kernel/syscalls/ioprio/ioprio.h       | 4 ++--
 testcases/kernel/syscalls/ioprio/ioprio_set03.c | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/testcases/kernel/syscalls/ioprio/ioprio.h b/testcases/kernel/syscalls/ioprio/ioprio.h
index 6ca134a54..c2115bf20 100644
--- a/testcases/kernel/syscalls/ioprio/ioprio.h
+++ b/testcases/kernel/syscalls/ioprio/ioprio.h
@@ -61,10 +61,10 @@ static inline int sys_ioprio_set(int which, int who, int ioprio)
 	return tst_syscall(__NR_ioprio_set, which, who, ioprio);
 }
 
-/* Priority range from 0 (highest) to 7 (lowest) */
+/* Priority range from 0 (highest) to IOPRIO_PRIO_NUM (lowest) */
 static inline int prio_in_range(int prio)
 {
-	if ((prio < 0) || (prio > 7))
+	if ((prio < 0) || (prio >= IOPRIO_PRIO_NUM))
 		return 0;
 	return 1;
 }
diff --git a/testcases/kernel/syscalls/ioprio/ioprio_set03.c b/testcases/kernel/syscalls/ioprio/ioprio_set03.c
index b2c962a6f..d6b44df85 100644
--- a/testcases/kernel/syscalls/ioprio/ioprio_set03.c
+++ b/testcases/kernel/syscalls/ioprio/ioprio_set03.c
@@ -27,7 +27,7 @@ static void run(void)
 	sys_ioprio_set(IOPRIO_WHO_PROCESS, 0,
 		       IOPRIO_PRIO_VALUE(class, 4));
 	TEST(sys_ioprio_set(IOPRIO_WHO_PROCESS, 0,
-			    IOPRIO_PRIO_VALUE(class, 8)));
+			    IOPRIO_PRIO_VALUE(class, IOPRIO_PRIO_NUM)));
 	if (TST_RET == -1) {
 		ioprio_check_setting(class, 4, 1);
 		if (errno == EINVAL)
-- 
2.40.1


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/2] ioprio: use ioprio.h kernel header if it exists

[Petr Vorel]

Hi Damien,

thanks for this effort!

> For the ioprio system call test cases, avoid blindly defining the
> IOPRIO_XXX macro internally and instead use the kernel user API header
> file if it exists. Given that the definitions in this header file have
> changed over time, make sure to test for the existence of the macro
> IOPRIO_PRIO_LEVEL macro and define it if it does not exist. Similarly,
> use IOPRIO_NR_LEVELS to define IOPRIO_PRIO_NUM if that macro exists.

> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

If I'm correct, the only change in v2 is added Linus Walleij's RBT.
nit: it'd be better if he sent it himself.

> ---
>  configure.ac                              |  1 +
>  testcases/kernel/syscalls/ioprio/ioprio.h | 29 +++++++++++++++++------
>  2 files changed, 23 insertions(+), 7 deletions(-)

> diff --git a/configure.ac b/configure.ac
> index 548288310..e4aa2cadf 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -56,6 +56,7 @@ AC_CHECK_HEADERS_ONCE([ \
>      linux/if_ether.h \
>      linux/if_packet.h \
>      linux/io_uring.h \
> +    linux/ioprio.h \
>      linux/keyctl.h \
>      linux/mempolicy.h \
>      linux/module.h \
> diff --git a/testcases/kernel/syscalls/ioprio/ioprio.h b/testcases/kernel/syscalls/ioprio/ioprio.h
> index c74380475..6ca134a54 100644
> --- a/testcases/kernel/syscalls/ioprio/ioprio.h
> +++ b/testcases/kernel/syscalls/ioprio/ioprio.h
FYI the header should be moved to include/lapi/,
but that can be done as a separate effort afterwards (by us LTP developers).

> @@ -6,6 +6,12 @@
>  #ifndef LTP_IOPRIO_H
>  #define LTP_IOPRIO_H

There needs to be
#include "config.h"

(Otherwise header will be never included.)

With this change
Reviewed-by: Petr Vorel <pvorel@suse.cz>

> +#ifdef HAVE_LINUX_IOPRIO_H
> +
> +# include <linux/ioprio.h>
> +
> +#else

NOTE: yes, we have policy to include kernel (or libc) headers in LTP LAPI
headers [1], but we usually instead of #else part always check for constants like
this:

#ifdef HAVE_LINUX_IOPRIO_H
# include <linux/ioprio.h>
#endif

#ifndef IOPRIO_CLASS_SHIFT
# define IOPRIO_CLASS_SHIFT	(13)
#endif
...
(Exactly the same way you do for e.g. IOPRIO_NR_LEVELS or IOPRIO_PRIO_LEVEL.)

I'm ok with this variant, because it's simpler (the header was added in
v5.15-rc1) and we can check for enum. But once some constant or enum gets
changed we'd need to handle this.

Kind regards,
Petr

[1] https://github.com/linux-test-project/ltp/wiki/C-Test-API#lapi-headers

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/2] ioprio: use ioprio.h kernel header if it exists

[Damien Le Moal]

On 6/20/23 19:14, Petr Vorel wrote:
> Hi Damien,
> 
> thanks for this effort!
> 
>> For the ioprio system call test cases, avoid blindly defining the
>> IOPRIO_XXX macro internally and instead use the kernel user API header
>> file if it exists. Given that the definitions in this header file have
>> changed over time, make sure to test for the existence of the macro
>> IOPRIO_PRIO_LEVEL macro and define it if it does not exist. Similarly,
>> use IOPRIO_NR_LEVELS to define IOPRIO_PRIO_NUM if that macro exists.
> 
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> If I'm correct, the only change in v2 is added Linus Walleij's RBT.
> nit: it'd be better if he sent it himself.

Yes, that was the only difference + I sent that v2 after subscribing to the ltp
list as I did get the notice about my messages being held up when I sent V1.

Thanks !

-- 
Damien Le Moal
Western Digital Research


-- 
Mailing list info: https://lists.linux.it/listinfo/ltp

Re: [LTP] [PATCH v2 1/2] ioprio: use ioprio.h kernel header if it exists

[Linus Walleij]

On Tue, Jun 20, 2023 at 12:14 PM Petr Vorel <pvorel@suse.cz> wrote:

> If I'm correct, the only change in v2 is added Linus Walleij's RBT.
> nit: it'd be better if he sent it himself.

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp