From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Vrabel Subject: Re: [PATCHv8 1/3] gnttab: Introduce rwlock to protect updates to grant table state Date: Tue, 19 May 2015 14:20:54 +0100 Message-ID: <555B38B6.8080708@citrix.com> References: <1431440115-27936-1-git-send-email-david.vrabel@citrix.com> <1431440115-27936-2-git-send-email-david.vrabel@citrix.com> <555B0A3D020000780007B755@mail.emea.novell.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 1YuhS8-0007KP-Gl for xen-devel@lists.xenproject.org; Tue, 19 May 2015 13:21:00 +0000 In-Reply-To: <555B0A3D020000780007B755@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: Keir Fraser , Ian Campbell , Christoph Egger , Tim Deegan , MattWilson , xen-devel@lists.xenproject.org, Malcolm Crossley List-Id: xen-devel@lists.xenproject.org On 19/05/15 09:02, Jan Beulich wrote: > > Which then of course raises the question - is taking the read lock > here and in several other places really sufficient? The thing that the > original spin lock appears to protect here is not only the grant table > structure itself, but also the active entry. I.e. relaxing to a read > lock would seem possible only after having put per-active-entry > locking in place. Yes, this series was split the wrong way round. I could: a) squash the first two patches back together; b) refactor the first two patches into - introduce active entry locks - introduce maptrack lock - convert grant table lock to rw lock. Which approach is preferred (obviously (a) is a lot less work)? >> @@ -64,6 +64,11 @@ struct grant_mapping { >> >> /* Per-domain grant information. */ >> struct grant_table { >> + /* >> + * Lock protecting updates to grant table state (version, active >> + * entry list, etc.) >> + */ >> + rwlock_t lock; >> /* Table size. Number of frames shared with guest */ >> unsigned int nr_grant_frames; >> /* Shared grant table (see include/public/grant_table.h). */ >> @@ -82,8 +87,8 @@ struct grant_table { >> struct grant_mapping **maptrack; >> unsigned int maptrack_head; >> unsigned int maptrack_limit; >> - /* Lock protecting updates to active and shared grant tables. */ >> - spinlock_t lock; >> + /* Lock protecting the maptrack page list, head, and limit */ >> + spinlock_t maptrack_lock; >> /* The defined versions are 1 and 2. Set to 0 if we don't know >> what version to use yet. */ >> unsigned gt_version; > > So in order for fields protected by one of the locks to be as likely > as possible on the same cache line as the lock itself, I think > gt_version should also be moved up in the structure. We may > even want/need to add some separator between basic and > maptrack data to ensure they end up on different cache lines. I think this sort of structure field shuffling should be a follow on patch. David