linux-kernel - Re: [patch V2 7/9] softirq: Replace barrier() with cpu_relax() in tasklet_unlock

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20201207113958.GJ3040@hirez.programming.kicks-ass.net>
Date:   Mon, 7 Dec 2020 12:39:58 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Frederic Weisbecker <frederic@...nel.org>,
        Paul McKenney <paulmck@...nel.org>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Subject: Re: [patch V2 7/9] softirq: Replace barrier() with cpu_relax() in
 tasklet_unlock_wait()

On Fri, Dec 04, 2020 at 06:01:58PM +0100, Thomas Gleixner wrote:
> From: Thomas Gleixner <tglx@...utronix.de>
> 
> A barrier() in a tight loop which waits for something to happen on a remote
> CPU is a pointless exercise. Replace it with cpu_relax() which allows HT
> siblings to make progress.
> 
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> ---
>  include/linux/interrupt.h |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -668,7 +668,8 @@ static inline void tasklet_unlock(struct
>  
>  static inline void tasklet_unlock_wait(struct tasklet_struct *t)
>  {
> -	while (test_bit(TASKLET_STATE_RUN, &(t)->state)) { barrier(); }
> +	while (test_bit(TASKLET_STATE_RUN, &(t)->state))
> +		cpu_relax();
>  }

Wouldn't it be nicer to stick a completion in tasklet_struct ? Or at the
very least use wait_var_event() or something?


diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index ee8299eb1f52..7818085ac003 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -663,12 +663,14 @@ static inline int tasklet_trylock(struct tasklet_struct *t)
 static inline void tasklet_unlock(struct tasklet_struct *t)
 {
 	smp_mb__before_atomic();
-	clear_bit(TASKLET_STATE_RUN, &(t)->state);
+	clear_bit(TASKLET_STATE_RUN, &t->state);
+	smp_mb__after_atomic();
+	wake_up_var(&t->state);
 }
 
 static inline void tasklet_unlock_wait(struct tasklet_struct *t)
 {
-	while (test_bit(TASKLET_STATE_RUN, &(t)->state)) { barrier(); }
+	wait_var_event(&t->state, !test_bit(TASKLET_STATE_RUN, &t->state));
 }
 #else
 #define tasklet_trylock(t) 1