Xen devel - Jan 2007 - [PATCH] [GNTTAB] expandable grant table

This one is waiting for final review before being checked in. Some people
want the functionality this enables (>3 VIFs per guest) so I''m
posting the
patch in advance of applying it to xen-unstable.

 -- Keir



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Fri, Jan 05, 2007 at 01:43:10PM +0000, Keir Fraser
wrote:
> This one is waiting for final review before being checked in. Some people
> want the functionality this enables (>3 VIFs per guest) so I''m
posting the
> patch in advance of applying it to xen-unstable.
I''ve been working on the same issue so that I have some patches.
I attached a part of them. Although they should be adjusted to 
your patch which is to be commited, I wanted to send these out before 
checking in.
If necessary, I''m willing to update them.

- acm code uses NR_GRANT_ENTRIES.
  The attached patch is only compile-tested.

- IA64 code needs a slight domain initialization adjustment.

- PPC code depends on the fact that shared grant table is machine continguous.
  I''m not familiar with PPC code so that I''m not sure how to
solve it.
  If arch specific code can override maximal grant table size,
  Xen/PPC can prohibit growing grant table.
  After solving the issue, they can allow to grow grant table.

-- 
yamahata




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Hi Isaku,

The dynamic grant table patch is now in xen-unstable (c/s
13952:4bd0ea9c499). Can you update and resend these patches?

 Thanks,
 Keir


On 6/1/07 06:48, "Isaku Yamahata" <yamahata@valinux.co.jp>
wrote:
> On Fri, Jan 05, 2007 at 01:43:10PM +0000, Keir Fraser wrote:
>> This one is waiting for final review before being checked in. Some
people
>> want the functionality this enables (>3 VIFs per guest) so
I''m posting the
>> patch in advance of applying it to xen-unstable.
> 
> I''ve been working on the same issue so that I have some patches.
> I attached a part of them. Although they should be adjusted to
> your patch which is to be commited, I wanted to send these out before
> checking in.
> If necessary, I''m willing to update them.
> 
> - acm code uses NR_GRANT_ENTRIES.
>   The attached patch is only compile-tested.
> 
> - IA64 code needs a slight domain initialization adjustment.
> 
> - PPC code depends on the fact that shared grant table is machine
continguous.
>   I''m not familiar with PPC code so that I''m not sure how
to solve it.
>   If arch specific code can override maximal grant table size,
>   Xen/PPC can prohibit growing grant table.
>   After solving the issue, they can allow to grow grant table.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Thu, Feb 15, 2007 at 02:39:37PM +0000, Keir Fraser
wrote:
> The dynamic grant table patch is now in xen-unstable (c/s
> 13952:4bd0ea9c499). Can you update and resend these patches?
Will do. Probably next week.

-- 
yamahata

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Attached the updated patches. Please find them

- 13985_4fa9b86d41b8_grow_granttable_acm.patch
	acm part. I did only compile test.

- 13986_cca098fd122c_grow_granttable_ia64.patch
	ia64 part.

- 13987_f5f232aef7bd_grow_granttable_powerpc.patch
	powerpc part. This patch prohibits growing grant table.
	I didn''t even compile test.
	It needs review by xen/powerpc developers and they may
	have their own opinions.

Thanks,

On Thu, Feb 15, 2007 at 02:39:37PM +0000, Keir Fraser
wrote:
> 
> Hi Isaku,
> 
> The dynamic grant table patch is now in xen-unstable (c/s
> 13952:4bd0ea9c499). Can you update and resend these patches?
> 
>  Thanks,
>  Keir
> 
> 
> On 6/1/07 06:48, "Isaku Yamahata" <yamahata@valinux.co.jp>
wrote:
> 
> > On Fri, Jan 05, 2007 at 01:43:10PM +0000, Keir Fraser wrote:
> >> This one is waiting for final review before being checked in. Some
people
> >> want the functionality this enables (>3 VIFs per guest) so
I''m posting the
> >> patch in advance of applying it to xen-unstable.
> > 
> > I''ve been working on the same issue so that I have some
patches.
> > I attached a part of them. Although they should be adjusted to
> > your patch which is to be commited, I wanted to send these out before
> > checking in.
> > If necessary, I''m willing to update them.
> > 
> > - acm code uses NR_GRANT_ENTRIES.
> >   The attached patch is only compile-tested.
> > 
> > - IA64 code needs a slight domain initialization adjustment.
> > 
> > - PPC code depends on the fact that shared grant table is machine
continguous.
> >   I''m not familiar with PPC code so that I''m not
sure how to solve it.
> >   If arch specific code can override maximal grant table size,
> >   Xen/PPC can prohibit growing grant table.
> >   After solving the issue, they can allow to grow grant table.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
> 
-- 
yamahata




_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Mon, 2007-02-19 at 17:03 +0900, Isaku Yamahata wrote:
> Attached the updated patches. Please find them
> 
> - 13985_4fa9b86d41b8_grow_granttable_acm.patch
> 	acm part. I did only compile test.
> 
> - 13986_cca098fd122c_grow_granttable_ia64.patch
> 	ia64 part.
> 
> - 13987_f5f232aef7bd_grow_granttable_powerpc.patch
> 	powerpc part. This patch prohibits growing grant table.
> 	I didn''t even compile test.
> 	It needs review by xen/powerpc developers and they may
> 	have their own opinions.
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.

-- 
Hollis Blanchard
IBM Linux Technology Center


_______________________________________________
Xen-ppc-devel mailing list
Xen-ppc-devel@lists.xensource.com
http://lists.xensource.com/xen-ppc-devel

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. 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.

 -- Keir



_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@lists.xensource.com
http://lists.xensource.com/xen-ia64-devel

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 27/2/07 00:05, "Hollis Blanchard" <hollisb@us.ibm.com> wrote:
> 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.
If there''s a memory barrier before the update of nr_grant_frames,
explicitly
or implied, then removing the locking from add_to_physmap is fine. Otherwise
not: reading an integer is atomic, but using that as a flag to indicate
presence of other state updated under the same lock is just a little bit
suspect (but might be okay).

 -- Keir



_______________________________________________
Xen-ia64-devel mailing list
Xen-ia64-devel@lists.xensource.com
http://lists.xensource.com/xen-ia64-devel