From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hollis Blanchard Subject: Re: [XenPPC] Re: [PATCH] [GNTTAB] expandable grant table Date: Mon, 26 Feb 2007 18:05:18 -0600 Message-ID: <1172534718.910.43.camel@basalt> References: Reply-To: Hollis Blanchard Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Keir Fraser Cc: xen-ppc-devel@lists.xensource.com, Isaku Yamahata , xen-devel , Keir Fraser , xen-ia64-devel@lists.xensource.com List-Id: xen-devel@lists.xenproject.org On Mon, 2007-02-26 at 23:39 +0000, Keir Fraser wrote: > On 26/2/07 23:31, "Hollis Blanchard" wrote: > > > > > Hi Yamahata-san, thanks very much for your patch! > > > > I'm confused about one thing though: why do we need to take a lock to > > read d->grant_table->nr_grant_frames? It's a simple integer, so no lock > > is required or useful. > > I haven't got the ppc code immediately to hand but the x86 code will > dynamically grow the grant table based on requests made via the > add_to_physmap hypercall, hence it needs to take the lock. OK, I see x86's XENMAPSPACE_grant_table handler; the locking makes sense there. > I see ia64 takes > it also even though it only reads from nr_grant_frames (it won;t dynamically > expand via the add_to_physmap hypercall). That's possibly overkill although > there is some concern over reading nr_grant_frames versus looking up a > grant-table page that appears within the range [0, nr_grant_frames-1] which > taking the lock trivially sidesteps. If ia64 didn't take the lock it might > be possible to see an increased value of nr_grant_frames that the expand > function hadn't yet fully installed the new grant-table pages for yet. I don't believe that's a concern, since updating grant_table->nr_grant_frames is the very last step in gnttab_grow_table(), and it will only grow. The comments about locking around nr_grant_frames() and nr_grant_entries() are confusing at best, since you don't need a lock to read an integer... -- Hollis Blanchard IBM Linux Technology Center