[PATCHSET 0/2] Optimize get_current_ioprio() a bit

[PATCHSET 0/2] Optimize get_current_ioprio() a bit

[Jens Axboe]

Hi,

Came across this one in some recent profiling, and it's actually
quite grim:

+    2.71%  io_uring  [kernel.vmlinux]  [k] __get_task_ioprio                       ▒

Just do the easy thing and move it into the header so we avoid
a function call per IO for this. Patch 2 is just a general cleanup.

-- 
Jens Axboe

[PATCH 1/2] block: move __get_task_ioprio() into header file

[Jens Axboe]

We call this once per IO, which can be millions of times per second.
Since nobody really uses io priorities, or at least it isn't very
common, this is all wasted time and can amount to as much as 3% of
the total kernel time.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/ioprio.c         | 26 --------------------------
 include/linux/ioprio.h | 25 ++++++++++++++++++++++++-
 2 files changed, 24 insertions(+), 27 deletions(-)

diff --git a/block/ioprio.c b/block/ioprio.c
index b5a942519a79..73301a261429 100644
--- a/block/ioprio.c
+++ b/block/ioprio.c
@@ -139,32 +139,6 @@ SYSCALL_DEFINE3(ioprio_set, int, which, int, who, int, ioprio)
 	return ret;
 }
 
-/*
- * If the task has set an I/O priority, use that. Otherwise, return
- * the default I/O priority.
- *
- * Expected to be called for current task or with task_lock() held to keep
- * io_context stable.
- */
-int __get_task_ioprio(struct task_struct *p)
-{
-	struct io_context *ioc = p->io_context;
-	int prio;
-
-	if (p != current)
-		lockdep_assert_held(&p->alloc_lock);
-	if (ioc)
-		prio = ioc->ioprio;
-	else
-		prio = IOPRIO_DEFAULT;
-
-	if (IOPRIO_PRIO_CLASS(prio) == IOPRIO_CLASS_NONE)
-		prio = IOPRIO_PRIO_VALUE(task_nice_ioclass(p),
-					 task_nice_ioprio(p));
-	return prio;
-}
-EXPORT_SYMBOL_GPL(__get_task_ioprio);
-
 static int get_task_ioprio(struct task_struct *p)
 {
 	int ret;
diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
index 7578d4f6a969..d6a9b5b7ed16 100644
--- a/include/linux/ioprio.h
+++ b/include/linux/ioprio.h
@@ -47,7 +47,30 @@ static inline int task_nice_ioclass(struct task_struct *task)
 }
 
 #ifdef CONFIG_BLOCK
-int __get_task_ioprio(struct task_struct *p);
+/*
+ * If the task has set an I/O priority, use that. Otherwise, return
+ * the default I/O priority.
+ *
+ * Expected to be called for current task or with task_lock() held to keep
+ * io_context stable.
+ */
+static inline int __get_task_ioprio(struct task_struct *p)
+{
+	struct io_context *ioc = p->io_context;
+	int prio;
+
+	if (p != current)
+		lockdep_assert_held(&p->alloc_lock);
+	if (ioc)
+		prio = ioc->ioprio;
+	else
+		prio = IOPRIO_DEFAULT;
+
+	if (IOPRIO_PRIO_CLASS(prio) == IOPRIO_CLASS_NONE)
+		prio = IOPRIO_PRIO_VALUE(task_nice_ioclass(p),
+					 task_nice_ioprio(p));
+	return prio;
+}
 #else
 static inline int __get_task_ioprio(struct task_struct *p)
 {
-- 
2.43.0

Re: [PATCH 1/2] block: move __get_task_ioprio() into header file

[Bart Van Assche]

On 1/8/24 10:59, Jens Axboe wrote:
> We call this once per IO, which can be millions of times per second.
> Since nobody really uses io priorities, or at least it isn't very
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

We have plans to set an I/O priority for most Android processes in the
near future. According to what Damien wrote on linux-block, there is
probably a significant number of hard disks for which I/O priority is
configured.

> common, this is all wasted time and can amount to as much as 3% of
> the total kernel time.

Anyway:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 1/2] block: move __get_task_ioprio() into header file

[Jens Axboe]

On 1/8/24 12:24 PM, Bart Van Assche wrote:
> On 1/8/24 10:59, Jens Axboe wrote:
>> We call this once per IO, which can be millions of times per second.
>> Since nobody really uses io priorities, or at least it isn't very
>         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> We have plans to set an I/O priority for most Android processes in the
> near future. According to what Damien wrote on linux-block, there is
> probably a significant number of hard disks for which I/O priority is
> configured.

The rates at which Android does IO means this won't really make any
difference, should probably have qualified the statement with "nobody
uses io priorities and does fast IO".

>> common, this is all wasted time and can amount to as much as 3% of
>> the total kernel time.
> 
> Anyway:
> 
> Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Thanks!

-- 
Jens Axboe

Re: [PATCH 1/2] block: move __get_task_ioprio() into header file

[Chaitanya Kulkarni]

On 1/8/24 10:59, Jens Axboe wrote:
> We call this once per IO, which can be millions of times per second.
> Since nobody really uses io priorities, or at least it isn't very
> common, this is all wasted time and can amount to as much as 3% of
> the total kernel time.
>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

[PATCH 2/2] block: make __get_task_ioprio() easier to read

[Jens Axboe]

We don't need to do any gymnastics if we don't have an io_context
assigned at all, so just return early with our default priority.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 include/linux/ioprio.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
index d6a9b5b7ed16..db1249cd9692 100644
--- a/include/linux/ioprio.h
+++ b/include/linux/ioprio.h
@@ -59,13 +59,13 @@ static inline int __get_task_ioprio(struct task_struct *p)
 	struct io_context *ioc = p->io_context;
 	int prio;
 
+	if (!ioc)
+		return IOPRIO_DEFAULT;
+
 	if (p != current)
 		lockdep_assert_held(&p->alloc_lock);
-	if (ioc)
-		prio = ioc->ioprio;
-	else
-		prio = IOPRIO_DEFAULT;
 
+	prio = ioc->ioprio;
 	if (IOPRIO_PRIO_CLASS(prio) == IOPRIO_CLASS_NONE)
 		prio = IOPRIO_PRIO_VALUE(task_nice_ioclass(p),
 					 task_nice_ioprio(p));
-- 
2.43.0

Re: [PATCH 2/2] block: make __get_task_ioprio() easier to read

[Bart Van Assche]

On 1/8/24 10:59, Jens Axboe wrote:
> We don't need to do any gymnastics if we don't have an io_context
> assigned at all, so just return early with our default priority.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>   include/linux/ioprio.h | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
> index d6a9b5b7ed16..db1249cd9692 100644
> --- a/include/linux/ioprio.h
> +++ b/include/linux/ioprio.h
> @@ -59,13 +59,13 @@ static inline int __get_task_ioprio(struct task_struct *p)
>   	struct io_context *ioc = p->io_context;
>   	int prio;
>   
> +	if (!ioc)
> +		return IOPRIO_DEFAULT;
> +
>   	if (p != current)
>   		lockdep_assert_held(&p->alloc_lock);
> -	if (ioc)
> -		prio = ioc->ioprio;
> -	else
> -		prio = IOPRIO_DEFAULT;
>   
> +	prio = ioc->ioprio;
>   	if (IOPRIO_PRIO_CLASS(prio) == IOPRIO_CLASS_NONE)
>   		prio = IOPRIO_PRIO_VALUE(task_nice_ioclass(p),
>   					 task_nice_ioprio(p));

Shouldn't it be mentioned in the subject that this patch is a performance
optimization? Anyway:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 2/2] block: make __get_task_ioprio() easier to read

[Jens Axboe]

On 1/8/24 12:22 PM, Bart Van Assche wrote:
> On 1/8/24 10:59, Jens Axboe wrote:
>> We don't need to do any gymnastics if we don't have an io_context
>> assigned at all, so just return early with our default priority.
>>
>> Signed-off-by: Jens Axboe <axboe@kernel.dk>
>> ---
>>   include/linux/ioprio.h | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h
>> index d6a9b5b7ed16..db1249cd9692 100644
>> --- a/include/linux/ioprio.h
>> +++ b/include/linux/ioprio.h
>> @@ -59,13 +59,13 @@ static inline int __get_task_ioprio(struct task_struct *p)
>>       struct io_context *ioc = p->io_context;
>>       int prio;
>>   +    if (!ioc)
>> +        return IOPRIO_DEFAULT;
>> +
>>       if (p != current)
>>           lockdep_assert_held(&p->alloc_lock);
>> -    if (ioc)
>> -        prio = ioc->ioprio;
>> -    else
>> -        prio = IOPRIO_DEFAULT;
>>   +    prio = ioc->ioprio;
>>       if (IOPRIO_PRIO_CLASS(prio) == IOPRIO_CLASS_NONE)
>>           prio = IOPRIO_PRIO_VALUE(task_nice_ioclass(p),
>>                        task_nice_ioprio(p));
> 
> Shouldn't it be mentioned in the subject that this patch is a performance
> optimization? Anyway:

I doubt this matters for performance really, it's more of a readability
thing for me.

> Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Thanks!

-- 
Jens Axboe

Re: [PATCH 2/2] block: make __get_task_ioprio() easier to read

[Chaitanya Kulkarni]

On 1/8/24 10:59, Jens Axboe wrote:
> We don't need to do any gymnastics if we don't have an io_context
> assigned at all, so just return early with our default priority.
>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck