From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759269AbcHaDll (ORCPT ); Tue, 30 Aug 2016 23:41:41 -0400 Received: from mail-pf0-f196.google.com ([209.85.192.196]:35689 "EHLO mail-pf0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759173AbcHaDlj (ORCPT ); Tue, 30 Aug 2016 23:41:39 -0400 Subject: Re: [RFC][PATCH] Fix a race between rwsem and the scheduler To: Peter Zijlstra References: <4050f2ce-1aee-d2aa-39e3-36e995b56252@gmail.com> <20160830121937.GQ10138@twins.programming.kicks-ass.net> Cc: LKML , Oleg Nesterov , Benjamin Herrenschmidt , Nicholas Piggin , Alexey Kardashevskiy From: Balbir Singh Message-ID: <1e2e1a49-db2f-bdae-53b2-0bda225be472@gmail.com> Date: Wed, 31 Aug 2016 13:41:33 +1000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.0 MIME-Version: 1.0 In-Reply-To: <20160830121937.GQ10138@twins.programming.kicks-ass.net> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 30/08/16 22:19, Peter Zijlstra wrote: > On Tue, Aug 30, 2016 at 06:49:37PM +1000, Balbir Singh wrote: >> >> >> The origin of the issue I've seen seems to be related to >> rwsem spin lock stealing. Basically I see the system deadlock'd in the >> following state > > As Nick says (good to see you're back Nick!), this is unrelated to > rwsems. > > This is true for pretty much every blocking wait loop out there, they > all do: > > for (;;) { > current->state = UNINTERRUPTIBLE; > smp_mb(); > if (cond) > break; > schedule(); > } > current->state = RUNNING; > > Which, if the wakeup is spurious, is just the pattern you need. Yes True! My bad Alexey had seen the same basic pattern, I should have been clearer in my commit log. Should I resend the patch? > >> +++ b/kernel/sched/core.c >> @@ -2016,6 +2016,17 @@ try_to_wake_up(struct task_struct *p, unsigned int state, int wake_flags) >> success = 1; /* we're going to change ->state */ >> cpu = task_cpu(p); >> >> + /* >> + * Ensure we see on_rq and p_state consistently >> + * >> + * For example in __rwsem_down_write_failed(), we have >> + * [S] ->on_rq = 1 [L] ->state >> + * MB RMB > > There isn't an MB there. The best I can do is UNLOCK+LOCK, which, thanks > to PPC, is _not_ MB. It is however sufficient for this case. > The MB comes from the __switch_to() in schedule(). Ben mentioned it in a different thread. >> + * [S] ->state = TASK_UNINTERRUPTIBLE [L] ->on_rq >> + * In the absence of the RMB p->on_rq can be observed to be 0 >> + * and we end up spinning indefinitely in while (p->on_cpu) >> + */ > > > /* > * Ensure we load p->on_rq _after_ p->state, otherwise it would > * be possible to, falsely, observe p->on_rq == 0 and get stuck > * in smp_cond_load_acquire() below. > * > * sched_ttwu_pending() try_to_wake_up() > * [S] p->on_rq = 1; [L] P->state > * UNLOCK rq->lock > * > * schedule() RMB > * LOCK rq->lock > * UNLOCK rq->lock > * > * [task p] > * [S] p->state = UNINTERRUPTIBLE [L] p->on_rq > * > * Pairs with the UNLOCK+LOCK on rq->lock from the > * last wakeup of our task and the schedule that got our task > * current. > */ > >> + smp_rmb(); >> if (p->on_rq && ttwu_remote(p, wake_flags)) >> goto stat; >> > > > Now, this has been present for a fair while, I suspect ever since we > reworked the wakeup path to not use rq->lock twice. Curious you only now > hit it. > Yes, I just hit it a a week or two back and I needed to collect data to explain why p->on_rq got to 0. Hitting it requires extreme stress -- for me I needed a system with large threads and less memory running stress-ng. Reproducing the problem takes an unpredictable amount of time. Balbir Singh.