Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 4 Mar 2020 10:56:47 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     Qian Cai <cai@lca.pw>, fweisbec@gmail.com, mingo@kernel.org,
        elver@google.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] tick/sched: fix data races at tick_do_timer_cpu
Message-ID: <20200304095647.GL2596@hirez.programming.kicks-ass.net>
References: <20200225030808.1207-1-cai@lca.pw>
 <87tv34laqq.fsf@nanos.tec.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87tv34laqq.fsf@nanos.tec.linutronix.de>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Mar 04, 2020 at 10:39:41AM +0100, Thomas Gleixner wrote:
> Qian,
> 
> Qian Cai <cai@lca.pw> writes:
> > tick_do_timer_cpu could be accessed concurrently where both plain writes
> > and plain reads are not protected by a lock. Thus, it could result in
> > data races. Fix them by adding pairs of READ|WRITE_ONCE(). The data
> > races were reported by KCSAN,
> 
> They are reported, but are they actually a real problem?
> 
> This completely lacks analysis why these 8 places need the
> READ/WRITE_ONCE() treatment at all and if so why the other 14 places
> accessing tick_do_timer_cpu are safe without it.
> 
> I definitely appreciate the work done with KCSAN, but just making the
> tool shut up does not cut it. 

Worse:

+	if (cpu != READ_ONCE(tick_do_timer_cpu) &&
+	    (READ_ONCE(tick_do_timer_cpu) != TICK_DO_TIMER_NONE ||

Doing that read twice is just utterly stupid. And the patch is full of
that :/

Please stop this non-thinking 'fix' generation!