From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933374AbcIFIXj (ORCPT ); Tue, 6 Sep 2016 04:23:39 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:47448 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752848AbcIFIXe (ORCPT ); Tue, 6 Sep 2016 04:23:34 -0400 Date: Tue, 6 Sep 2016 10:23:28 +0200 From: Peter Zijlstra To: Andreas Mohr Cc: der.herr@hofr.at, Al Viro , mingo@redhat.com, torvalds@linux-foundation.org, dave@stgolabs.net, Oleg Nesterov , riel@redhat.com, tj@kernel.org, paulmck@linux.vnet.ibm.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/7] fs/locks: Replace lg_global with a percpu-rwsem Message-ID: <20160906082328.GF10153@twins.programming.kicks-ass.net> References: <20160906015800.GA31162@rhlx01.hs-esslingen.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160906015800.GA31162@rhlx01.hs-esslingen.de> User-Agent: Mutt/1.5.23.1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Sep 06, 2016 at 03:58:00AM +0200, Andreas Mohr wrote: > Hi, > > [no properly binding reference via In-Reply-To: available thus manually re-creating, sorry] > > https://lkml.org/lkml/2016/9/5/832 Subscribe to lkml already.. also lkml.org is near useless these days, please use any other archive. > Two thoughts: > > ***multiple locks > > Don't have much insight into this > (didn't spend much thinking on this), > but of course it's unfortunate > that two lock types need to be serviced > rather than having the atomic handling exchange area/section > be sufficiently guarded by one lock only > (the question here could possibly be: > what kind of currently existing > structural disadvantage/layer distribution > prevents us from being able to > have things simply serviced > within one granular lock area only?) percpu rwsem is a sleeping lock, flc_flock is not. flc_flock also nests under other non-sleeping locks, and therefore cannot (trivially) be made a sleeping lock. This is in the Changelog. Furthermore, in the fast path of percpu_{down,up}_read() are exactly 0 atomic/serializing instructions. > ***lock handling > > > + percpu_down_read(&file_rwsem); > > spin_lock(&ctx->flc_lock); > > > > spin_unlock(&ctx->flc_lock); > > + percpu_up_read(&file_rwsem); > > > These are repeated multiple times in this commit, thus error-prone. > > A possibly good way to commit-micro-manage this would be: > 1. commit shoves things into a newly created encapsulation/wrapper helper > stuff_lock(&flc_lock); /* <---- naming surely can be improved here */ No, because not all instances of flc_flock need the percpu-rwsem held, creating such a wrapper could mistakenly create the impression it should. > That way it is pretty much guaranteed that: > a) neither one nor the other lock type > will get forgotten later, at a specific use site That's what we have lockdep_assert_held() for. Note how this patch also introduces percpu_rwsem_assert_held() usage, this insures we shall never 'forget' the appropriate locking. > b) lock order will be correctly maintained at all times > (AB-BA deadlock......) lockdep is rather good at finding those, also might_sleep() debugging would trivially catch those since percpu-rwsem is a sleeping lock while fcl_flock is not.