From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752624AbbH1OvZ (ORCPT ); Fri, 28 Aug 2015 10:51:25 -0400 Received: from mail-wi0-f171.google.com ([209.85.212.171]:36790 "EHLO mail-wi0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751857AbbH1OvY (ORCPT ); Fri, 28 Aug 2015 10:51:24 -0400 Date: Fri, 28 Aug 2015 16:51:21 +0200 From: Michal Hocko To: Oleg Nesterov Cc: Peter Zijlstra , LKML , David Howells , Linus Torvalds Subject: Re: wake_up_process implied memory barrier clarification Message-ID: <20150828145121.GG5301@dhcp22.suse.cz> References: <20150827122727.GC27052@dhcp22.suse.cz> <20150827124334.GY16853@twins.programming.kicks-ass.net> <20150827131444.GE27052@dhcp22.suse.cz> <20150827182654.GA12191@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150827182654.GA12191@redhat.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu 27-08-15 20:26:54, Oleg Nesterov wrote: > On 08/27, Michal Hocko wrote: > > > > --- a/Documentation/memory-barriers.txt > > +++ b/Documentation/memory-barriers.txt > > @@ -2031,6 +2031,9 @@ something up. The barrier occurs before the task state is cleared, and so sits > > STORE current->state > > LOAD event_indicated > > > > +Please note that wake_up_process is an exception here because it implies > > +the write memory barrier unconditionally. > > + > > I simply can't understand (can't even parse) this part of memory-barriers.txt. Do you mean the added text or the example above it? > > --- a/kernel/sched/core.c > > +++ b/kernel/sched/core.c > > @@ -1967,8 +1967,7 @@ static void try_to_wake_up_local(struct task_struct *p) > > * > > * Return: 1 if the process was woken up, 0 if it was already running. > > * > > - * It may be assumed that this function implies a write memory barrier before > > - * changing the task state if and only if any tasks are woken up. > > + * It may be assumed that this function implies a write memory barrier. > > */ > > I won't argue, technically this is correct of course. And I agree that > the old comment is misleading. Well the reason I've noticed is the following race in the scsi code CPU0 CPU1 scsi_error_handler scsi_host_dev_release kthread_stop() while (!kthread_should_stop()) { set_bit(KTHREAD_SHOULD_STOP) wake_up_process() wait_for_completion() set_current_state(TASK_INTERRUPTIBLE) schedule() [...] } I have read the comment for wake_up_process and was wondering that moving set_current_state before kthread_should_stop wouldn't be enough because the the task at CPU0 might be TASK_RUNNIG and so wake_up_process wouldn't wake up it and the missing write barrier could lead to a missed KTHREAD_SHOULD_STOP. A look into ttwu made my worry void. > But the new comment looks as if it is fine to avoid wmb() if you do > wake_up_process(). Say, > > void w(void) > { > A = 1; > wake_up_process(something_unrelated); > // we know that it implies wmb(). > B = 1; > } > > void r(void) > { > int a, b; > > b = B; > rmb(); > a = A; > > BUG_ON(b && !a); > } > > Perhaps this part of the comment should be simply removed, the unconditional > wmb() in ttwu() is just implementation detail. And note that initially the > documented behaviour of smp_mb__before_spinlock() was only the STORE - LOAD > serialization. Then people noticed that it actually does wmb() and started > to rely on this fact. > > To me, this comment should just explain that this function implies a barrier > but only in a sense that you do not need another one after CONDITION = T and > before wake_up_process(). I have no objection against more precise wording here but what we have is just misleading. -- Michal Hocko SUSE Labs