linux-kernel - Re: [PATCH 3/3] wait: avoid spurious calls to prepare_to_wait_event() in ___wait

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250304141946.GM5880@noisy.programming.kicks-ass.net>
Date: Tue, 4 Mar 2025 15:19:46 +0100
From: Peter Zijlstra <peterz@...radead.org>
To: Mateusz Guzik <mjguzik@...il.com>
Cc: torvalds@...ux-foundation.org, oleg@...hat.com, brauner@...nel.org,
	mingo@...hat.com, rostedt@...dmis.org, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH 3/3] wait: avoid spurious calls to
 prepare_to_wait_event() in ___wait_event()

On Tue, Mar 04, 2025 at 12:04:09AM +0100, Mateusz Guzik wrote:
> In vast majority of cases the condition determining whether the thread
> can proceed is true after the first wake up.
> 
> However, even in that case the thread ends up calling into
> prepare_to_wait_event() again, suffering a spurious irq + lock trip.
> 
> Then it calls into finish_wait() to unlink itself.
> 
> Note that in case of a pending signal the work done by
> prepare_to_wait_event() gets ignored even without the change.
> 
> pre-check the condition after waking up instead.
> 
> Stats gathared during a kernel build:
> bpftrace -e 'kprobe:prepare_to_wait_event,kprobe:finish_wait \
> 		 { @[probe] = count(); }'
> 
> @[kprobe:finish_wait]: 392483
> @[kprobe:prepare_to_wait_event]: 778690
> 
> As in calls to prepare_to_wait_event() almost double calls to
> finish_wait(). This evens out with the patch.
> 
> Signed-off-by: Mateusz Guzik <mjguzik@...il.com>
> ---
> 
> One may worry about using "condition" twice. However, macros leading up
> to this one already do it, so it should be fine.
> 
> Also one may wonder about fences -- to my understanding going off and on
> CPU guarantees a full fence, so the now avoided lock trip changes
> nothing.

so it always helps if you provide actual numbers. Supposedly this makes
it go faster?

Also, how much bytes does it add to the image?

Anyway, no real objection, but it would be good to have better
substantiation etc.

>  include/linux/wait.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/wait.h b/include/linux/wait.h
> index 2bdc8f47963b..965a19809c7e 100644
> --- a/include/linux/wait.h
> +++ b/include/linux/wait.h
> @@ -316,6 +316,9 @@ extern void init_wait_entry(struct wait_queue_entry *wq_entry, int flags);
>  		}								\
>  										\
>  		cmd;								\
> +										\
> +		if (condition)							\
> +			break;							\
>  	}									\
>  	finish_wait(&wq_head, &__wq_entry);					\
>  __out:	__ret;									\
> -- 
> 2.43.0
>