From mboxrd@z Thu Jan 1 00:00:00 1970 From: Malcolm Crossley Subject: Re: [PATCH 2/2] grant_table: convert grant table rwlock to percpu rwlock Date: Wed, 18 Nov 2015 11:56:30 +0000 Message-ID: <564C676E.3030603@citrix.com> References: <1446573502-8019-1-git-send-email-malcolm.crossley@citrix.com> <1446573502-8019-2-git-send-email-malcolm.crossley@citrix.com> <564B6C1A02000078000B603C@prv-mh.provo.novell.com> <564B6453.6050008@citrix.com> <564B746802000078000B60E1@prv-mh.provo.novell.com> <564B69A8.6050609@citrix.com> <1447842971.23626.30.camel@citrix.com> <564C670E02000078000B637B@prv-mh.provo.novell.com> <564C5FA8.8020808@citrix.com> <1447847400.23626.42.camel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Zz1Lq-0001j7-Mq for xen-devel@lists.xenproject.org; Wed, 18 Nov 2015 11:56:38 +0000 In-Reply-To: <1447847400.23626.42.camel@citrix.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: Ian Campbell , Jan Beulich Cc: Andrew Cooper , keir@xen.org, stefano.stabellini@citrix.com, xen-devel@lists.xenproject.org List-Id: xen-devel@lists.xenproject.org On 18/11/15 11:50, Ian Campbell wrote: > On Wed, 2015-11-18 at 11:23 +0000, Malcolm Crossley wrote: >> On 18/11/15 10:54, Jan Beulich wrote: >>>>>> On 18.11.15 at 11:36, wrote: >>>> On Tue, 2015-11-17 at 17:53 +0000, Andrew Cooper wrote: >>>>> On 17/11/15 17:39, Jan Beulich wrote: >>>>>>>>> On 17.11.15 at 18:30, wrote: >>>>>>> On 17/11/15 17:04, Jan Beulich wrote: >>>>>>>>>>> On 03.11.15 at 18:58, >>>>>>>>>>> wrote: >>>>>>>>> --- a/xen/common/grant_table.c >>>>>>>>> +++ b/xen/common/grant_table.c >>>>>>>>> @@ -178,6 +178,10 @@ struct active_grant_entry { >>>>>>>>> #define _active_entry(t, e) \ >>>>>>>>> ((t)->active[(e)/ACGNT_PER_PAGE][(e)%ACGNT_PER_PAGE]) >>>>>>>>> >>>>>>>>> +bool_t grant_rwlock_barrier; >>>>>>>>> + >>>>>>>>> +DEFINE_PER_CPU(rwlock_t *, grant_rwlock); >>>>>>>> Shouldn't these be per grant table? And wouldn't doing so >>>>>>>> eliminate >>>>>>>> the main limitation of the per-CPU rwlocks? >>>>>>> The grant rwlock is per grant table. >>>>>> That's understood, but I don't see why the above items aren't, >>>>>> too. >>>>> >>>>> Ah - because there is never any circumstance where two grant tables >>>>> are >>>>> locked on the same pcpu. >>>> >>>> So per-cpu rwlocks are really a per-pcpu read lock with a fallthrough >>>> to a >>>> per-$resource (here == granttable) rwlock when any writers are >>>> present for >>>> any instance $resource, not just the one where the write lock is >>>> desired, >>>> for the duration of any write lock? >>> >> >> The above description is the very good for for how the per-cpu rwlocks behave. >> The code stores a pointer to the per-$resource in the percpu area when a user is >> reading the per-$resource, this is why the lock is not safe if you take the lock >> for two different per-$resource simultaneously. The grant table code only takes >> one grant table lock at any one time so it is a safe user. > > So essentially the "per-pcpu read lock" as I called it is really in essence > a sort of "byte lock" via the NULL vs non-NULL state of the per-cpu pointer > to the underlying rwlock. It's not quite a byte lock because it stores a full pointer to the per-$resource that it's using. It could be changed to be a byte lock but then you will need a percpu area per-$resource. > >>> That's not how I understood it, the rwlock isn't per-pCPU (at least not >>> in what this patch does - it remains a per-domain one). The per-pCPU >>> object is a pointer to an rwlock, which gets made point to whatever >>> domain's rwlock the pCPU wants to own. >>> >> >> This description is correct but it's important to note that the rwlock >> is only used by the writers and could be effectively replaced with a >> spinlock. > > The rwlock is taken (briefly) by readers if *writer_activating is, isn't > it? Yes I got this wrong. Sorry about causing confusion. Malcolm > > Ian. >