From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752763AbcEISv5 (ORCPT ); Mon, 9 May 2016 14:51:57 -0400 Received: from mx2.suse.de ([195.135.220.15]:57266 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751867AbcEISvz (ORCPT ); Mon, 9 May 2016 14:51:55 -0400 Date: Mon, 9 May 2016 11:51:18 -0700 From: Davidlohr Bueso To: Peter Zijlstra 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: <20160509185118.GC29630@linux-uzut.site> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20160509161129.GC3408@twins.programming.kicks-ass.net> User-Agent: Mutt/1.6.0 (2016-04-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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... Thanks, Davidlohr