From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752425AbcEIS77 (ORCPT ); Mon, 9 May 2016 14:59:59 -0400 Received: from merlin.infradead.org ([205.233.59.134]:57569 "EHLO merlin.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751275AbcEIS76 (ORCPT ); Mon, 9 May 2016 14:59:58 -0400 Date: Mon, 9 May 2016 20:59:45 +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: <20160509185945.GD3408@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> <20160509161129.GC3408@twins.programming.kicks-ass.net> <20160509185118.GC29630@linux-uzut.site> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160509185118.GC29630@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 11:51:18AM -0700, Davidlohr Bueso wrote: > On Mon, 09 May 2016, Peter Zijlstra wrote: > > >>>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? > > Ah indeed, you are most certainly correct. For some reason I was always > considering schedule() in the picture. Hmm I'll have to think about this > some more, but given the small chance of a waiter actually seeing the nil > task at the first iteration I'm wondering if we could just invert the code > and call schedule() before the task check. Saving the refcounts will serve > _all_ reader waiters otoh, but this would obviously need numbers... So with where you're going -- using wake_q, it naturally goes away if you do: wake_q_add(&wake_q, tsk); smp_store_release(&waiter->task, NULL); Because the wake_q already takes a task ref, and we'll not actually issue the wakeup until after the waiter->task store.