From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751673AbcEIQLi (ORCPT ); Mon, 9 May 2016 12:11:38 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:48626 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750916AbcEIQLh (ORCPT ); Mon, 9 May 2016 12:11:37 -0400 Date: Mon, 9 May 2016 18:11:29 +0200 From: Peter Zijlstra To: Davidlohr Bueso Cc: mingo@kernel.org, tglx@linutronix.de, Waiman.Long@hpe.com, jason.low2@hp.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/4] locking/rwsem: Drop superfluous waiter refcount Message-ID: <20160509161129.GC3408@twins.programming.kicks-ass.net> References: <1462769770-29363-1-git-send-email-dave@stgolabs.net> <1462769770-29363-3-git-send-email-dave@stgolabs.net> <20160509073933.GC3430@twins.programming.kicks-ass.net> <20160509155607.GB29630@linux-uzut.site> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160509155607.GB29630@linux-uzut.site> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, May 09, 2016 at 08:56:07AM -0700, Davidlohr Bueso wrote: > On Mon, 09 May 2016, Peter Zijlstra wrote: > > >On Sun, May 08, 2016 at 09:56:08PM -0700, Davidlohr Bueso wrote: > >>Read waiters are currently reference counted from the time it enters > >>the slowpath until the lock is released and the waiter is awoken. This > >>is fragile and superfluous considering everything occurs within down_read() > >>without returning to the caller, and the very nature of the primitive does > >>not suggest that the task can disappear from underneath us. In addition, > >>spurious wakeups can make the whole refcount useless as get_task_struct() > >>is only called when setting up the waiter. > > > >So I think you're wrong here; imagine this: > > > > > > rwsem_down_read_failed() rwsem_wake() > > get_task_struct(); > > raw_spin_lock_irq(&wait_lock); > > list_add_tail(&waiter.list, &wait_list); > > raw_spin_unlock_irq(&wait_lock); > > raw_spin_lock_irqsave(&wait_lock) > > __rwsem_do_wake() > > while (true) { > > set_task_state(tsk, TASK_UNINTERRUPTIBLE); > > waiter->task = NULL > > if (!waiter.task) // true > > break; > > > > __set_task_state(tsk, TASK_RUNNING); > > > > do_exit(); > > wake_up_process(tsk); /* BOOM */ > > I may be missing something, but rwsem_down_read_failed() will not return until > after the wakeup is done by the rwsem_wake() thread. The above never gets to schedule(), and even if it did, a spurious wakeup could've happened, no? > So racing with do_exit() isn't > going to occur because the task is still blocked at that point. This is even more > so with delaying the wakeup. Similarly, we don't do this for writers either, which > could also suffer from similar scenarios. The write side is different; it serializes on wait_lock. See how it takes wait_lock again, after blocking, and removes itself from the wait_list. Readers do not do this, they rely on the waker to remove them, and therefore suffer this problem.