| Message ID | 20230608005315.3703446-2-dlemoal@kernel.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Improve ioprio tests | expand |
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
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 !
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
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",