Re: [XenPPC] Re: [PATCH] [GNTTAB] expandable grant table

[Hollis Blanchard <hollisb@us.ibm.com>]

On Mon, 2007-02-26 at 23:39 +0000, Keir Fraser wrote:
> On 26/2/07 23:31, "Hollis Blanchard" <hollisb@us.ibm.com> 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