[PATCH] irq_work: Replace wait loop with rcuwait_wait_event

[PATCH] irq_work: Replace wait loop with rcuwait_wait_event

[Steven Davis]

The previous implementation of irq_work_sync used a busy-wait
loop with cpu_relax() to check the status of IRQ work. This
approach, while functional, could lead to inefficiencies in
CPU usage.

This commit replaces the busy-wait loop with the rcuwait_wait_event
mechanism. This change leverages the RCU wait mechanism to handle
waiting for IRQ work completion more efficiently, improving CPU
responsiveness and reducing unnecessary CPU usage.

Signed-off-by: Steven Davis <goldside000@outlook.com>
---
 kernel/irq_work.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/kernel/irq_work.c b/kernel/irq_work.c
index 2f4fb336dda1..2b092a1d07a9 100644
--- a/kernel/irq_work.c
+++ b/kernel/irq_work.c
@@ -295,8 +295,7 @@ void irq_work_sync(struct irq_work *work)
 		return;
 	}
 
-	while (irq_work_is_busy(work))
-		cpu_relax();
+	rcuwait_wait_event(&work->irqwait, !irq_work_is_busy(work), TASK_UNINTERRUPTIBLE);
 }
 EXPORT_SYMBOL_GPL(irq_work_sync);
 
-- 
2.39.5

Re: [PATCH] irq_work: Replace wait loop with rcuwait_wait_event

[Frederic Weisbecker]

Le Thu, Sep 19, 2024 at 11:43:26AM -0400, Steven Davis a écrit :
> The previous implementation of irq_work_sync used a busy-wait
> loop with cpu_relax() to check the status of IRQ work. This
> approach, while functional, could lead to inefficiencies in
> CPU usage.
> 
> This commit replaces the busy-wait loop with the rcuwait_wait_event
> mechanism. This change leverages the RCU wait mechanism to handle
> waiting for IRQ work completion more efficiently, improving CPU
> responsiveness and reducing unnecessary CPU usage.
> 
> Signed-off-by: Steven Davis <goldside000@outlook.com>
> ---
>  kernel/irq_work.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/kernel/irq_work.c b/kernel/irq_work.c
> index 2f4fb336dda1..2b092a1d07a9 100644
> --- a/kernel/irq_work.c
> +++ b/kernel/irq_work.c
> @@ -295,8 +295,7 @@ void irq_work_sync(struct irq_work *work)
>  		return;
>  	}
>  
> -	while (irq_work_is_busy(work))
> -		cpu_relax();
> +	rcuwait_wait_event(&work->irqwait, !irq_work_is_busy(work), TASK_UNINTERRUPTIBLE);

Dan Carpenter brought to my attention this a few weeks ago for another problem.:

perf_remove_from_context() <- disables preempt
__perf_event_exit_context() <- disables preempt
-> __perf_remove_from_context()
   -> perf_group_detach()
      -> perf_put_aux_event()
         -> put_event()
            -> _free_event()
               -> irq_work_sync()

Re: [PATCH] irq_work: Replace wait loop with rcuwait_wait_event

[Ankur Arora]

Frederic Weisbecker <frederic@kernel.org> writes:

> Le Thu, Sep 19, 2024 at 11:43:26AM -0400, Steven Davis a écrit :
>> The previous implementation of irq_work_sync used a busy-wait
>> loop with cpu_relax() to check the status of IRQ work. This
>> approach, while functional, could lead to inefficiencies in
>> CPU usage.
>>
>> This commit replaces the busy-wait loop with the rcuwait_wait_event
>> mechanism. This change leverages the RCU wait mechanism to handle
>> waiting for IRQ work completion more efficiently, improving CPU
>> responsiveness and reducing unnecessary CPU usage.
>>
>> Signed-off-by: Steven Davis <goldside000@outlook.com>
>> ---
>>  kernel/irq_work.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/kernel/irq_work.c b/kernel/irq_work.c
>> index 2f4fb336dda1..2b092a1d07a9 100644
>> --- a/kernel/irq_work.c
>> +++ b/kernel/irq_work.c
>> @@ -295,8 +295,7 @@ void irq_work_sync(struct irq_work *work)
>>  		return;
>>  	}
>>
>> -	while (irq_work_is_busy(work))
>> -		cpu_relax();
>> +	rcuwait_wait_event(&work->irqwait, !irq_work_is_busy(work), TASK_UNINTERRUPTIBLE);
>
> Dan Carpenter brought to my attention this a few weeks ago for another problem.:
>
> perf_remove_from_context() <- disables preempt
> __perf_event_exit_context() <- disables preempt
> -> __perf_remove_from_context()
>    -> perf_group_detach()
>       -> perf_put_aux_event()
>          -> put_event()
>             -> _free_event()
>                -> irq_work_sync()

irq_work_sync() is also annotated with might_sleep() (probably how Dan
saw it) so in principle the rcuwait_wait_event() isn't wrong there.


--
ankur

Re: [PATCH] irq_work: Replace wait loop with rcuwait_wait_event

[Steven Davis]

On Thu, 19 Sep 2024 at 20:10:42 -0700, Ankur Arora wrote:
> Frederic Weisbecker <frederic@kernel.org> writes:
>
>> Le Thu, Sep 19, 2024 at 11:43:26AM -0400, Steven Davis a écrit :
>>> The previous implementation of irq_work_sync used a busy-wait
>>> loop with cpu_relax() to check the status of IRQ work. This
>>> approach, while functional, could lead to inefficiencies in
>>> CPU usage.
>>>
>>> This commit replaces the busy-wait loop with the rcuwait_wait_event
>>> mechanism. This change leverages the RCU wait mechanism to handle
>>> waiting for IRQ work completion more efficiently, improving CPU
>>> responsiveness and reducing unnecessary CPU usage.
>>>
>>> Signed-off-by: Steven Davis <goldside000@outlook.com>
>>> ---
>>>  kernel/irq_work.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/kernel/irq_work.c b/kernel/irq_work.c
>>> index 2f4fb336dda1..2b092a1d07a9 100644
>>> --- a/kernel/irq_work.c
>>> +++ b/kernel/irq_work.c
>>> @@ -295,8 +295,7 @@ void irq_work_sync(struct irq_work *work)
>>>  		return;
>>>  	}
>>>
>>> -	while (irq_work_is_busy(work))
>>> -		cpu_relax();
>>> +	rcuwait_wait_event(&work->irqwait, !irq_work_is_busy(work), TASK_UNINTERRUPTIBLE);
>>
>> Dan Carpenter brought to my attention this a few weeks ago for another problem.:
>>
>> perf_remove_from_context() <- disables preempt
>> __perf_event_exit_context() <- disables preempt
>> -> __perf_remove_from_context()
>>    -> perf_group_detach()
>>       -> perf_put_aux_event()
>>          -> put_event()
>>             -> _free_event()
>>                -> irq_work_sync()
>
> irq_work_sync() is also annotated with might_sleep() (probably how Dan
> saw it) so in principle the rcuwait_wait_event() isn't wrong there.

The might_sleep() annotation does seem to indicate a preempt context. My main 
goal with this patch is to increase performance and responsiveness, and I believe
that rcu_wait_event() will do that. Let me know of any further improvements or 
drawbacks to this approach, though.

	Steven 

> --
> ankur

Re: [PATCH] irq_work: Replace wait loop with rcuwait_wait_event

[Ankur Arora]

Steven Davis <goldside000@outlook.com> writes:

> On Thu, 19 Sep 2024 at 20:10:42 -0700, Ankur Arora wrote:
>> Frederic Weisbecker <frederic@kernel.org> writes:
>>
>>> Le Thu, Sep 19, 2024 at 11:43:26AM -0400, Steven Davis a écrit :
>>>> The previous implementation of irq_work_sync used a busy-wait
>>>> loop with cpu_relax() to check the status of IRQ work. This
>>>> approach, while functional, could lead to inefficiencies in
>>>> CPU usage.
>>>>
>>>> This commit replaces the busy-wait loop with the rcuwait_wait_event
>>>> mechanism. This change leverages the RCU wait mechanism to handle
>>>> waiting for IRQ work completion more efficiently, improving CPU
>>>> responsiveness and reducing unnecessary CPU usage.
>>>>
>>>> Signed-off-by: Steven Davis <goldside000@outlook.com>
>>>> ---
>>>>  kernel/irq_work.c | 3 +--
>>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/kernel/irq_work.c b/kernel/irq_work.c
>>>> index 2f4fb336dda1..2b092a1d07a9 100644
>>>> --- a/kernel/irq_work.c
>>>> +++ b/kernel/irq_work.c
>>>> @@ -295,8 +295,7 @@ void irq_work_sync(struct irq_work *work)
>>>>  		return;
>>>>  	}
>>>>
>>>> -	while (irq_work_is_busy(work))
>>>> -		cpu_relax();
>>>> +	rcuwait_wait_event(&work->irqwait, !irq_work_is_busy(work), TASK_UNINTERRUPTIBLE);
>>>
>>> Dan Carpenter brought to my attention this a few weeks ago for another problem.:
>>>
>>> perf_remove_from_context() <- disables preempt
>>> __perf_event_exit_context() <- disables preempt
>>> -> __perf_remove_from_context()
>>>    -> perf_group_detach()
>>>       -> perf_put_aux_event()
>>>          -> put_event()
>>>             -> _free_event()
>>>                -> irq_work_sync()
>>
>> irq_work_sync() is also annotated with might_sleep() (probably how Dan
>> saw it) so in principle the rcuwait_wait_event() isn't wrong there.
>
> The might_sleep() annotation does seem to indicate a preempt context.

Yeah, and that is one of the problems with might_sleep(), cond_resched()
etc. They can get out of sync with the surrounding code or code paths.

In this case, this could might mean that maybe the annotation is wrong
and should be removed. So, it might be worth looking at the calling
paths a bit closer to see if calling schedule() is really safe in all
of these contexts.

And, if all the other contexts are safe, then it would be a good idea
to fix the perf_remove_from_context() since with this patch, we might
end up scheduling in spinlock context.

--
ankur

Re: [PATCH] irq_work: Replace wait loop with rcuwait_wait_event

[Frederic Weisbecker]

Le Sun, Sep 22, 2024 at 11:35:11PM -0700, Ankur Arora a écrit :
> 
> Steven Davis <goldside000@outlook.com> writes:
> 
> > On Thu, 19 Sep 2024 at 20:10:42 -0700, Ankur Arora wrote:
> >> Frederic Weisbecker <frederic@kernel.org> writes:
> >>
> >>> Le Thu, Sep 19, 2024 at 11:43:26AM -0400, Steven Davis a écrit :
> >>>> The previous implementation of irq_work_sync used a busy-wait
> >>>> loop with cpu_relax() to check the status of IRQ work. This
> >>>> approach, while functional, could lead to inefficiencies in
> >>>> CPU usage.
> >>>>
> >>>> This commit replaces the busy-wait loop with the rcuwait_wait_event
> >>>> mechanism. This change leverages the RCU wait mechanism to handle
> >>>> waiting for IRQ work completion more efficiently, improving CPU
> >>>> responsiveness and reducing unnecessary CPU usage.
> >>>>
> >>>> Signed-off-by: Steven Davis <goldside000@outlook.com>
> >>>> ---
> >>>>  kernel/irq_work.c | 3 +--
> >>>>  1 file changed, 1 insertion(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/kernel/irq_work.c b/kernel/irq_work.c
> >>>> index 2f4fb336dda1..2b092a1d07a9 100644
> >>>> --- a/kernel/irq_work.c
> >>>> +++ b/kernel/irq_work.c
> >>>> @@ -295,8 +295,7 @@ void irq_work_sync(struct irq_work *work)
> >>>>  		return;
> >>>>  	}
> >>>>
> >>>> -	while (irq_work_is_busy(work))
> >>>> -		cpu_relax();
> >>>> +	rcuwait_wait_event(&work->irqwait, !irq_work_is_busy(work), TASK_UNINTERRUPTIBLE);
> >>>
> >>> Dan Carpenter brought to my attention this a few weeks ago for another problem.:
> >>>
> >>> perf_remove_from_context() <- disables preempt
> >>> __perf_event_exit_context() <- disables preempt
> >>> -> __perf_remove_from_context()
> >>>    -> perf_group_detach()
> >>>       -> perf_put_aux_event()
> >>>          -> put_event()
> >>>             -> _free_event()
> >>>                -> irq_work_sync()
> >>
> >> irq_work_sync() is also annotated with might_sleep() (probably how Dan
> >> saw it) so in principle the rcuwait_wait_event() isn't wrong there.
> >
> > The might_sleep() annotation does seem to indicate a preempt context.
> 
> Yeah, and that is one of the problems with might_sleep(), cond_resched()
> etc. They can get out of sync with the surrounding code or code paths.
> 
> In this case, this could might mean that maybe the annotation is wrong
> and should be removed. So, it might be worth looking at the calling
> paths a bit closer to see if calling schedule() is really safe in all
> of these contexts.
> 
> And, if all the other contexts are safe, then it would be a good idea
> to fix the perf_remove_from_context() since with this patch, we might
> end up scheduling in spinlock context.

After a second look, I think this path is fine. If an aux event is
being unlinked, this means that it hasn't been removed from the context
yet. So it can't be the last reference to it causing a call to _free_event().

So it _looks_ good after all.

Thanks.

Re: [PATCH] irq_work: Replace wait loop with rcuwait_wait_event

[Frederic Weisbecker]

Le Thu, Sep 19, 2024 at 11:43:26AM -0400, Steven Davis a écrit :
> The previous implementation of irq_work_sync used a busy-wait
> loop with cpu_relax() to check the status of IRQ work. This
> approach, while functional, could lead to inefficiencies in
> CPU usage.
> 
> This commit replaces the busy-wait loop with the rcuwait_wait_event
> mechanism. This change leverages the RCU wait mechanism to handle
> waiting for IRQ work completion more efficiently, improving CPU
> responsiveness and reducing unnecessary CPU usage.
> 
> Signed-off-by: Steven Davis <goldside000@outlook.com>
> ---
>  kernel/irq_work.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/kernel/irq_work.c b/kernel/irq_work.c
> index 2f4fb336dda1..2b092a1d07a9 100644
> --- a/kernel/irq_work.c
> +++ b/kernel/irq_work.c
> @@ -295,8 +295,7 @@ void irq_work_sync(struct irq_work *work)
>  		return;
>  	}
>  
> -	while (irq_work_is_busy(work))
> -		cpu_relax();
> +	rcuwait_wait_event(&work->irqwait, !irq_work_is_busy(work), TASK_UNINTERRUPTIBLE);

So you can remove the CONFIG_PREEMPT_RT special case in this function, right?

Thanks.

>  }
>  EXPORT_SYMBOL_GPL(irq_work_sync);
>  
> -- 
> 2.39.5
>