From mboxrd@z Thu Jan 1 00:00:00 1970 From: Malcolm Crossley Subject: Re: [PATCHv2 2/3] grant_table: convert grant table rwlock to percpu rwlock Date: Wed, 25 Nov 2015 13:43:25 +0000 Message-ID: <5655BAFD.6000001@citrix.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> 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 1a1aMV-0003sa-Vs for xen-devel@lists.xenproject.org; Wed, 25 Nov 2015 13:43:56 +0000 In-Reply-To: <5655B93202000078000B8FBF@prv-mh.provo.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich 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 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? > >> @@ -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'll add the wrappers to for the grant table but will this make it harder to determine how the locking works? Thanks for the comments Malcolm > > Jan >