| Message ID | 20210802092157.1260445-3-damien.lemoal@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | IO priority fixes and improvements | expand |
On 8/2/21 2:21 AM, Damien Le Moal wrote: > diff --git a/include/uapi/linux/ioprio.h b/include/uapi/linux/ioprio.h > index 77b17e08b0da..27dc7fb0ba12 100644 > --- a/include/uapi/linux/ioprio.h > +++ b/include/uapi/linux/ioprio.h > @@ -6,10 +6,12 @@ > * Gives us 8 prio classes with 13-bits of data for each class > */ > #define IOPRIO_CLASS_SHIFT (13) > +#define IOPRIO_CLASS_MASK (0x07) > #define IOPRIO_PRIO_MASK ((1UL << IOPRIO_CLASS_SHIFT) - 1) > > -#define IOPRIO_PRIO_CLASS(mask) ((mask) >> IOPRIO_CLASS_SHIFT) > -#define IOPRIO_PRIO_DATA(mask) ((mask) & IOPRIO_PRIO_MASK) > +#define IOPRIO_PRIO_CLASS(val) \ > + (((val) >> IOPRIO_CLASS_SHIFT) & IOPRIO_CLASS_MASK) > +#define IOPRIO_PRIO_DATA(val) ((val) & IOPRIO_PRIO_MASK) > #define IOPRIO_PRIO_VALUE(class, data) (((class) << IOPRIO_CLASS_SHIFT) | data) > > /* > @@ -23,9 +25,16 @@ enum { > IOPRIO_CLASS_RT, > IOPRIO_CLASS_BE, > IOPRIO_CLASS_IDLE, > + > + IOPRIO_CLASS_MAX, > }; > > -#define ioprio_valid(mask) (IOPRIO_PRIO_CLASS((mask)) != IOPRIO_CLASS_NONE) > +static inline bool ioprio_valid(unsigned short ioprio) > +{ > + unsigned short class = IOPRIO_PRIO_CLASS(ioprio); > + > + return class > IOPRIO_CLASS_NONE && class < IOPRIO_CLASS_MAX; > +} > > /* > * 8 best effort priority levels are supported Are there any plans to use ioprio_valid() in user space applications? If not, should this function perhaps be defined in a kernel-only header? Thanks, Bart.
On 2021/08/03 0:58, Bart Van Assche wrote: > On 8/2/21 2:21 AM, Damien Le Moal wrote: >> diff --git a/include/uapi/linux/ioprio.h b/include/uapi/linux/ioprio.h >> index 77b17e08b0da..27dc7fb0ba12 100644 >> --- a/include/uapi/linux/ioprio.h >> +++ b/include/uapi/linux/ioprio.h >> @@ -6,10 +6,12 @@ >> * Gives us 8 prio classes with 13-bits of data for each class >> */ >> #define IOPRIO_CLASS_SHIFT (13) >> +#define IOPRIO_CLASS_MASK (0x07) >> #define IOPRIO_PRIO_MASK ((1UL << IOPRIO_CLASS_SHIFT) - 1) >> >> -#define IOPRIO_PRIO_CLASS(mask) ((mask) >> IOPRIO_CLASS_SHIFT) >> -#define IOPRIO_PRIO_DATA(mask) ((mask) & IOPRIO_PRIO_MASK) >> +#define IOPRIO_PRIO_CLASS(val) \ >> + (((val) >> IOPRIO_CLASS_SHIFT) & IOPRIO_CLASS_MASK) >> +#define IOPRIO_PRIO_DATA(val) ((val) & IOPRIO_PRIO_MASK) >> #define IOPRIO_PRIO_VALUE(class, data) (((class) << IOPRIO_CLASS_SHIFT) | data) >> >> /* >> @@ -23,9 +25,16 @@ enum { >> IOPRIO_CLASS_RT, >> IOPRIO_CLASS_BE, >> IOPRIO_CLASS_IDLE, >> + >> + IOPRIO_CLASS_MAX, >> }; >> >> -#define ioprio_valid(mask) (IOPRIO_PRIO_CLASS((mask)) != IOPRIO_CLASS_NONE) >> +static inline bool ioprio_valid(unsigned short ioprio) >> +{ >> + unsigned short class = IOPRIO_PRIO_CLASS(ioprio); >> + >> + return class > IOPRIO_CLASS_NONE && class < IOPRIO_CLASS_MAX; >> +} >> >> /* >> * 8 best effort priority levels are supported > > Are there any plans to use ioprio_valid() in user space applications? If > not, should this function perhaps be defined in a kernel-only header? Good point. I wondered the same. I think it may be better to leave that one in include/linux/ioprio.h instead of moving it to the uapi header. Jens, Thoughts ? > > Thanks, > > Bart. > > >
diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h index ef9ad4fb245f..997641211cca 100644 --- a/include/linux/ioprio.h +++ b/include/linux/ioprio.h @@ -25,10 +25,9 @@ static inline int task_nice_ioclass(struct task_struct *task) { if (task->policy == SCHED_IDLE) return IOPRIO_CLASS_IDLE; - else if (task_is_realtime(task)) + if (task_is_realtime(task)) return IOPRIO_CLASS_RT; - else - return IOPRIO_CLASS_BE; + return IOPRIO_CLASS_BE; } /* diff --git a/include/uapi/linux/ioprio.h b/include/uapi/linux/ioprio.h index 77b17e08b0da..27dc7fb0ba12 100644 --- a/include/uapi/linux/ioprio.h +++ b/include/uapi/linux/ioprio.h @@ -6,10 +6,12 @@ * Gives us 8 prio classes with 13-bits of data for each class */ #define IOPRIO_CLASS_SHIFT (13) +#define IOPRIO_CLASS_MASK (0x07) #define IOPRIO_PRIO_MASK ((1UL << IOPRIO_CLASS_SHIFT) - 1) -#define IOPRIO_PRIO_CLASS(mask) ((mask) >> IOPRIO_CLASS_SHIFT) -#define IOPRIO_PRIO_DATA(mask) ((mask) & IOPRIO_PRIO_MASK) +#define IOPRIO_PRIO_CLASS(val) \ + (((val) >> IOPRIO_CLASS_SHIFT) & IOPRIO_CLASS_MASK) +#define IOPRIO_PRIO_DATA(val) ((val) & IOPRIO_PRIO_MASK) #define IOPRIO_PRIO_VALUE(class, data) (((class) << IOPRIO_CLASS_SHIFT) | data) /* @@ -23,9 +25,16 @@ enum { IOPRIO_CLASS_RT, IOPRIO_CLASS_BE, IOPRIO_CLASS_IDLE, + + IOPRIO_CLASS_MAX, }; -#define ioprio_valid(mask) (IOPRIO_PRIO_CLASS((mask)) != IOPRIO_CLASS_NONE) +static inline bool ioprio_valid(unsigned short ioprio) +{ + unsigned short class = IOPRIO_PRIO_CLASS(ioprio); + + return class > IOPRIO_CLASS_NONE && class < IOPRIO_CLASS_MAX; +} /* * 8 best effort priority levels are supported
An iocb aio_reqprio field is 16 bits only, but often handled as an int in the block layer. E.g. ioprio_check_cap() takes an int as argument. However, with such int casting function calls, the upper 16 bits of the argument may be left uninitialized by the compiler, resulting in invalid values for the IOPRIO_PRIO_CLASS() macro (garbage upper bits) and in an error return for functions such as ioprio_check_cap(). Fix this by masking the result of the shift by IOPRIO_CLASS_SHIFT bits in the IOPRIO_PRIO_CLASS() macro. The new macro IOPRIO_CLASS_MASK defines the 3-bits mask for the priority class. While at it, cleanup the following: * Update the mention of CFQ in the comment describing priority classes and mention BFQ and mq-deadline. * Change the argument name of the IOPRIO_PRIO_CLASS() and IOPRIO_PRIO_DATA() macros from "mask" to "val" to reflect the fact that an IO priority value should be passed rather than a mask. * Change the ioprio_valid() macro into an inline function, adding a check on the maximum value of the class of a priority as defined by the IOPRIO_CLASS_MAX enum value. * Remove the unnecessary "else" after the return statements in task_nice_ioclass(). Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- include/linux/ioprio.h | 5 ++--- include/uapi/linux/ioprio.h | 15 ++++++++++++--- 2 files changed, 14 insertions(+), 6 deletions(-)