linux-kernel - Re: [PATCH 03/11] rcu: clean up rcu_preempt_deferred_qs

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: <20191031135234.GQ20975@paulmck-ThinkPad-P72>
Date:   Thu, 31 Oct 2019 06:52:34 -0700
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Lai Jiangshan <laijs@...ux.alibaba.com>
Cc:     linux-kernel@...r.kernel.org,
        Josh Triplett <josh@...htriplett.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Joel Fernandes <joel@...lfernandes.org>, rcu@...r.kernel.org
Subject: Re: [PATCH 03/11] rcu: clean up rcu_preempt_deferred_qs_irqrestore()

On Thu, Oct 31, 2019 at 10:07:58AM +0000, Lai Jiangshan wrote:
> Remove several unneeded return.
> 
> It doesn't need to return earlier after every code block.
> The code protects itself and be safe to fall through because
> every code block has its own condition tests.
> 
> Signed-off-by: Lai Jiangshan <laijs@...ux.alibaba.com>
> ---
>  kernel/rcu/tree_plugin.h | 14 +-------------
>  1 file changed, 1 insertion(+), 13 deletions(-)
> 
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index 59ef10da1e39..82595db04eec 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -439,19 +439,10 @@ rcu_preempt_deferred_qs_irqrestore(struct task_struct *t, unsigned long flags)
>  	 * t->rcu_read_unlock_special cannot change.
>  	 */
>  	special = t->rcu_read_unlock_special;
> -	rdp = this_cpu_ptr(&rcu_data);
> -	if (!special.s && !rdp->exp_deferred_qs) {
> -		local_irq_restore(flags);
> -		return;
> -	}

The point of this check is the common case of this function being invoked
when both fields are zero, avoiding the below redundant store and all the
extra checks of subfields of special.

Or are you saying that current compilers figure all this out?

							Thanx, Paul

>  	t->rcu_read_unlock_special.b.deferred_qs = false;
>  	if (special.b.need_qs) {
>  		rcu_qs();
>  		t->rcu_read_unlock_special.b.need_qs = false;
> -		if (!t->rcu_read_unlock_special.s && !rdp->exp_deferred_qs) {
> -			local_irq_restore(flags);
> -			return;
> -		}
>  	}
>  
>  	/*
> @@ -460,12 +451,9 @@ rcu_preempt_deferred_qs_irqrestore(struct task_struct *t, unsigned long flags)
>  	 * tasks are handled when removing the task from the
>  	 * blocked-tasks list below.
>  	 */
> +	rdp = this_cpu_ptr(&rcu_data);
>  	if (rdp->exp_deferred_qs) {
>  		rcu_report_exp_rdp(rdp);
> -		if (!t->rcu_read_unlock_special.s) {
> -			local_irq_restore(flags);
> -			return;
> -		}
>  	}
>  
>  	/* Clean up if blocked during RCU read-side critical section. */
> -- 
> 2.20.1
>