From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCHv9 1/4] gnttab: per-active entry locking Date: Thu, 21 May 2015 15:11:00 +0100 Message-ID: <555DE774.1080504@citrix.com> References: <1432137250-3700-1-git-send-email-david.vrabel@citrix.com> <1432137250-3700-2-git-send-email-david.vrabel@citrix.com> <555DA97F020000780007C91C@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YvRD1-0000z5-Gr for xen-devel@lists.xenproject.org; Thu, 21 May 2015 14:12:27 +0000 In-Reply-To: <555DA97F020000780007C91C@mail.emea.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 , David Vrabel Cc: xen-devel@lists.xenproject.org, Keir Fraser , Ian Campbell , Tim Deegan List-Id: xen-devel@lists.xenproject.org On 21/05/15 08:46, Jan Beulich wrote: >>>> On 20.05.15 at 17:54, wrote: >> @@ -702,6 +729,7 @@ __gnttab_map_grant_ref( >> >> cache_flags = (shah->flags & (GTF_PAT | GTF_PWT | GTF_PCD) ); >> >> + active_entry_release(act); >> spin_unlock(&rgt->lock); > > Just for my understanding: The lock isn't meant to also cover *shah? > I.e. it could be dropped ahead of the cache_flags assignment? I think so, but I think it's preferable (for now) to keep the locked regions as-is where possible. There are few other places where it looked like the locked regions can be reduced. It won't really make much difference though since the read lock and active entry locks are not contented. >> @@ -978,7 +1009,7 @@ __gnttab_unmap_common( >> if ( (rc = replace_grant_host_mapping(op->host_addr, >> op->frame, op->new_addr, >> op->flags)) < 0 ) >> - goto unmap_out; >> + goto act_release_out; >> >> ASSERT(act->pin & (GNTPIN_hstw_mask | GNTPIN_hstr_mask)); >> op->map->flags &= ~GNTMAP_host_map; > > act doesn't look to be accessed anymore after the if() this is > contained in - why don't you drop the lock as early as possible? > Or wait - do we need act->pin to remain stable until after that > subsequent if() (in which case dropping the lock before the > final if() in the function would mainly make error handling more > cumbersome without bying us much)? See comment above. David