From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933761AbdC3RNP (ORCPT ); Thu, 30 Mar 2017 13:13:15 -0400 Received: from mx2.suse.de ([195.135.220.15]:43098 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933206AbdC3RNO (ORCPT ); Thu, 30 Mar 2017 13:13:14 -0400 MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Date: Thu, 30 Mar 2017 10:13:12 -0700 From: Davidlohr Bueso To: Laurent Dufour Cc: Davidlohr Bueso , mingo@kernel.org, peterz@infradead.org, akpm@linux-foundation.org, jack@suse.cz, kirill.shutemov@linux.intel.com, mhocko@suse.com, mgorman@techsingularity.net, linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/5] locking: Introduce range reader/writer lock Organization: SUSE Labs In-Reply-To: <1901e378-c18a-3cca-a68a-044867078433@linux.vnet.ibm.com> References: <1488863010-13028-1-git-send-email-dave@stgolabs.net> <1488863010-13028-2-git-send-email-dave@stgolabs.net> <1901e378-c18a-3cca-a68a-044867078433@linux.vnet.ibm.com> Message-ID: User-Agent: Roundcube Webmail Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 2017-03-30 07:56, Laurent Dufour wrote: > On 07/03/2017 06:03, Davidlohr Bueso wrote: >> +static inline int wait_for_ranges(struct range_rwlock_tree *tree, >> + struct range_rwlock *lock, long state) >> +{ >> + int ret = 0; >> + >> + while (true) { >> + set_current_state(state); >> + >> + if (unlikely(signal_pending_state(state, current))) { >> + unsigned long flags; >> + >> + ret = -EINTR; >> + /* >> + * We're not taking the lock after all, cleanup >> + * after ourselves. >> + */ >> + spin_lock_irqsave(&tree->lock, flags); >> + lock->reader = false; >> + __range_tree_remove(tree, lock); >> + spin_unlock_irqrestore(&tree->lock, flags); >> + break; >> + } >> + >> + /* do we need to go to sleep? */ >> + if (!lock->blocking_ranges) >> + break; > > Hi Davidlohr, > > While building a kernel on top of a patched kernel using full range > lock > in the place of mmap_sem, I found that fork() sometimes failed > returning > ENOMEM. > It happens that if fork() get called at the time signal is sent to the > calling process, the call to range_write_lock_interruptible() is > failing > even if there is no contention on the lock. This is because we check > for > the signal pending before checking for the lock contention in > wait_for_ranges(). > > The loop here should rather be : > > while (true) { > /* do we need to go to sleep? */ > if (!lock->blocking_ranges) > break; > Thanks, this makes sense, and is actually the standard way of waiting in most locks. > if (unlikely(signal_pending_state(state, current))) { > unsigned long flags; > > ret = -EINTR; > /* > * We're not taking the lock after all, cleanup > * after ourselves. > */ > spin_lock_irqsave(&tree->lock, flags); > lock->reader = false; > __range_tree_remove(tree, lock); > spin_unlock_irqrestore(&tree->lock, flags); > break; > } > > set_current_state(state); > > schedule(); > } > > I also moved the call to set_current_state() before calling schedule(), > not sure this has to be done this way, but my system seems to work fine > like this. No, we do not hold any locks. Please keep set_current_state() the very first thing we do in the loop. You can check Documentation/memory-barriers.txt for details :-) Thanks, Davidlohr