From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCHv2 2/3] grant_table: convert grant table rwlock to percpu rwlock Date: Wed, 25 Nov 2015 06:53:24 -0700 Message-ID: <5655CB6402000078000B906C@prv-mh.provo.novell.com> References: <1448035423-24242-1-git-send-email-malcolm.crossley@citrix.com> <1448035423-24242-3-git-send-email-malcolm.crossley@citrix.com> <5655B93202000078000B8FBF@prv-mh.provo.novell.com> <5655BAFD.6000001@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1a1aVn-0004LH-4C for xen-devel@lists.xenproject.org; Wed, 25 Nov 2015 13:53:31 +0000 In-Reply-To: <5655BAFD.6000001@citrix.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Malcolm Crossley Cc: keir@xen.org, ian.campbell@citrix.com, george.dunlap@eu.citrix.com, andrew.cooper3@citrix.com, Marcos.Matsunaga@oracle.com, stefano.stabellini@citrix.com, xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org >>> On 25.11.15 at 14:43, wrote: > On 25/11/15 12:35, Jan Beulich wrote: >>>>> On 20.11.15 at 17:03, wrote: >>> @@ -208,8 +210,6 @@ active_entry_acquire(struct grant_table *t, grant_ref_t > e) >>> { >>> struct active_grant_entry *act; >>> >>> - ASSERT(rw_is_locked(&t->lock)); >> >> Even if not covering all cases, I don't think this should be dropped, >> just like you don't drop rw_is_write_locked() asserts. Would properly >> replacing this be rather expensive? > > I could add a percpu_rw_is_locked function but it will be racy because I > can't > set the local lock barrier variable ( set/clearing that would race with real > users of the write lock ) Therefore I would be left polling the percpu > readers > which is not reliable without the local barrier variable. Should I still add > the function if it won't reliably detect the locked condition? Iiuc that would then risk the ASSERT() producing false positives, which clearly is a no-go. In which case I'd still like you to leave in some sort of comment (perhaps just the current code converted to a comment) to document the requirement. >>> @@ -3180,7 +3178,7 @@ grant_table_create( >>> goto no_mem_0; >>> >>> /* Simple stuff. */ >>> - rwlock_init(&t->lock); >>> + percpu_rwlock_resource_init(&t->lock); >> >> Considering George's general comment (on patch 1), would it perhaps >> make sense to store (in debug builds) the per-CPU variable's offset >> in the lock structure, having percpu_{read,write}_lock() verify it? Or >> at the very least have gt_{read,write}_lock() wrappers, thus >> avoiding the need to explicitly pass grant_rwlock at each use site? I >> certainly agree with George that we should make it as hard as >> possible for someone to get using these constructs wrong. > > I can add storing the "per-CPU variable" owner in the local lock structure for > debug builds and have the percpu_{read,write} verify it at runtime. > > The downsides is that the size of structures will vary between debug and non-debug > builds and that perhaps users may be confused what the per-CPU variables are > used for. I'm not sure what confusion might arise here. > I'll add the wrappers to for the grant table but will this make it harder to > determine how the locking works? Why would it? At the call sites we don't really care about the mechanism. But perhaps I'm not seeing what you're thinking of... Jan