Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 7 Jul 2020 10:20:05 +0100
From:   Qais Yousef <qais.yousef@arm.com>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Dave Jones <davej@codemonkey.org.uk>,
        Mel Gorman <mgorman@techsingularity.net>,
        Linux Kernel <linux-kernel@vger.kernel.org>, mingo@kernel.org,
        Linus Torvalds <torvalds@linux-foundation.org>,
        paul.gortmaker@windriver.com, valentin.schneider@arm.com
Subject: Re: weird loadavg on idle machine post 5.7
Message-ID: <20200707092004.jviycpvzb4l5ytiz@e107158-lin.cambridge.arm.com>
References: <20200702171548.GA11813@codemonkey.org.uk>
 <20200702213627.GF3183@techsingularity.net>
 <20200703090226.GV4800@hirez.programming.kicks-ass.net>
 <20200703104033.GK117543@hirez.programming.kicks-ass.net>
 <20200703205153.GA19901@codemonkey.org.uk>
 <20200706145952.GB597537@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20200706145952.GB597537@hirez.programming.kicks-ass.net>
User-Agent: NeoMutt/20171215
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 07/06/20 16:59, Peter Zijlstra wrote:

[...]

> @@ -4104,12 +4108,19 @@ static void __sched notrace __schedule(bool preempt)
>  	local_irq_disable();
>  	rcu_note_context_switch(preempt);
>  
> +	prev_state = prev->state;
> +
>  	/*
> -	 * Make sure that signal_pending_state()->signal_pending() below
> -	 * can't be reordered with __set_current_state(TASK_INTERRUPTIBLE)
> -	 * done by the caller to avoid the race with signal_wake_up().
> +	 * __set_current_state(@state)
> +	 * schedule()				signal_wake_up()
> +	 *   prev_state = p->state		  set_tsk_thread_flag(p, TIF_SIGPENDING)
> +	 *					  wake_up_state()
> +	 *   LOCK rq->lock			    LOCK p->pi_state
> +	 *   smp_mb__after_spinlock()		    smp_mb__after_spinlock()
> +	 *     if (signal_pending_state()	    if (p->state & @state)
> +	 *
>  	 *
> -	 * The membarrier system call requires a full memory barrier
> +	 * Also, the membarrier system call requires a full memory barrier
>  	 * after coming from user-space, before storing to rq->curr.
>  	 */
>  	rq_lock(rq, &rf);
> @@ -4120,10 +4131,30 @@ static void __sched notrace __schedule(bool preempt)
>  	update_rq_clock(rq);
>  
>  	switch_count = &prev->nivcsw;
> -	if (!preempt && prev->state) {
> -		if (signal_pending_state(prev->state, prev)) {
> +	/*
> +	 * We must re-load p->state in case ttwu_runnable() changed it
> +	 * before we acquired rq->lock.
> +	 */
> +	if (!preempt && prev_state && prev_state == prev->state) {

I think the compiler won't optimize `prev_state == prev->state` out because of
the smp_mb__after_spinlock() which implies a compiler barrier. Still not sure
if it's worth making prev->state accesses a READ_ONCE()?

Thanks

--
Qais Yousef