Rusty Russell
2005-Jan-19 03:53 UTC
[Xen-devel] [PATCH] Trivial fix for latent bug in page_alloc.c
Was reading through page_alloc.c, and the "find smallest order" loop assumes MIN_ORDER is 0. Easiest fix is to get rid of "MIN_ORDER" and hence NR_ORDERS, and simplify code. Rusty. --- xen/common/page_alloc.c.~1~ 2005-01-11 15:35:54.000000000 +1100 +++ xen/common/page_alloc.c 2005-01-19 14:51:47.000000000 +1100 @@ -203,10 +203,8 @@ #define NR_ZONES 2 /* Up to 2^10 pages can be allocated at once. */ -#define MIN_ORDER 0 #define MAX_ORDER 10 -#define NR_ORDERS (MAX_ORDER - MIN_ORDER + 1) -static struct list_head heap[NR_ZONES][NR_ORDERS]; +static struct list_head heap[NR_ZONES][MAX_ORDER]; static unsigned long avail[NR_ZONES]; @@ -220,7 +218,7 @@ memset(avail, 0, sizeof(avail)); for ( i = 0; i < NR_ZONES; i++ ) - for ( j = 0; j < NR_ORDERS; j++ ) + for ( j = 0; j < MAX_ORDER; j++ ) INIT_LIST_HEAD(&heap[i][j]); /* Pages that are free now go to the domain sub-allocator. */ @@ -251,17 +249,18 @@ int i; struct pfn_info *pg; - if ( unlikely(order < MIN_ORDER) || unlikely(order > MAX_ORDER) ) + ASSERT(order >= 0); + if ( unlikely(order >= MAX_ORDER) ) return NULL; spin_lock(&heap_lock); /* Find smallest order which can satisfy the request. */ - for ( i = order; i < NR_ORDERS; i++ ) + for ( i = order; i < MAX_ORDER; i++ ) if ( !list_empty(&heap[zone][i]) ) break; - if ( i == NR_ORDERS ) + if ( i == MAX_ORDER ) goto no_memory; pg = list_entry(heap[zone][i].next, struct pfn_info, list); -- A bad analogy is like a leaky screwdriver -- Richard Braakman ------------------------------------------------------- The SF.Net email is sponsored by: Beat the post-holiday blues Get a FREE limited edition SourceForge.net t-shirt from ThinkGeek. It''s fun and FREE -- well, almost....http://www.thinkgeek.com/sfshirt _______________________________________________ Xen-devel mailing list Xen-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/xen-devel
Keir Fraser
2005-Jan-19 07:51 UTC
Re: [Xen-devel] [PATCH] Trivial fix for latent bug in page_alloc.c
> Was reading through page_alloc.c, and the "find smallest order" loop > assumes MIN_ORDER is 0. Easiest fix is to get rid of "MIN_ORDER" and > hence NR_ORDERS, and simplify code. > > Rusty.Thanks for the patch. I think nuking MIN_ORDER is an improvement. -- Keir ------------------------------------------------------- The SF.Net email is sponsored by: Beat the post-holiday blues Get a FREE limited edition SourceForge.net t-shirt from ThinkGeek. It''s fun and FREE -- well, almost....http://www.thinkgeek.com/sfshirt _______________________________________________ Xen-devel mailing list Xen-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/xen-devel
David Hopwood
2005-Jan-19 16:45 UTC
Re: [Xen-devel] [PATCH] Trivial fix for latent bug in page_alloc.c
Rusty Russell wrote:> Was reading through page_alloc.c, and the "find smallest order" loop > assumes MIN_ORDER is 0. Easiest fix is to get rid of "MIN_ORDER" and > hence NR_ORDERS, and simplify code. > > Rusty. > > --- xen/common/page_alloc.c.~1~ 2005-01-11 15:35:54.000000000 +1100 > +++ xen/common/page_alloc.c 2005-01-19 14:51:47.000000000 +1100 > @@ -203,10 +203,8 @@ > #define NR_ZONES 2 > > /* Up to 2^10 pages can be allocated at once. */ > -#define MIN_ORDER 0 > #define MAX_ORDER 10 > -#define NR_ORDERS (MAX_ORDER - MIN_ORDER + 1) > -static struct list_head heap[NR_ZONES][NR_ORDERS]; > +static struct list_head heap[NR_ZONES][MAX_ORDER];This patch is broken because it replaces NR_ORDERS with MAX_ORDER, not with MAX_ORDER+1. #define MAX_ORDER 10 static struct list_head heap[NR_ZONES][MAX_ORDER+1];> static unsigned long avail[NR_ZONES]; > > @@ -220,7 +218,7 @@ > memset(avail, 0, sizeof(avail)); > > for ( i = 0; i < NR_ZONES; i++ ) > - for ( j = 0; j < NR_ORDERS; j++ ) > + for ( j = 0; j < MAX_ORDER; j++ )for ( j = 0; j <= MAX_ORDER; j++ )> INIT_LIST_HEAD(&heap[i][j]); > > /* Pages that are free now go to the domain sub-allocator. */ > @@ -251,17 +249,18 @@ > int i; > struct pfn_info *pg; > > - if ( unlikely(order < MIN_ORDER) || unlikely(order > MAX_ORDER) ) > + ASSERT(order >= 0); > + if ( unlikely(order >= MAX_ORDER) ) > return NULL;Why is it valid to change ''return NULL'' to a failed ASSERT? Also changing > to >= is wrong. if ( unlikely(order < 0) || unlikely(order > MAX_ORDER) ) return NULL;> spin_lock(&heap_lock); > > /* Find smallest order which can satisfy the request. */ > - for ( i = order; i < NR_ORDERS; i++ ) > + for ( i = order; i < MAX_ORDER; i++ )for ( i = order; i <= MAX_ORDER; i++ )> if ( !list_empty(&heap[zone][i]) ) > break; > > - if ( i == NR_ORDERS ) > + if ( i == MAX_ORDER )if ( i > MAX_ORDER)> goto no_memory; > > pg = list_entry(heap[zone][i].next, struct pfn_info, list);-- David Hopwood <david.nospam.hopwood@blueyonder.co.uk> ------------------------------------------------------- The SF.Net email is sponsored by: Beat the post-holiday blues Get a FREE limited edition SourceForge.net t-shirt from ThinkGeek. It''s fun and FREE -- well, almost....http://www.thinkgeek.com/sfshirt _______________________________________________ Xen-devel mailing list Xen-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/xen-devel
Rusty Russell
2005-Feb-02 04:12 UTC
Re: [Xen-devel] [PATCH] Trivial fix for latent bug in page_alloc.c
On Wed, 2005-01-19 at 16:45 +0000, David Hopwood wrote:> Rusty Russell wrote: > > --- xen/common/page_alloc.c.~1~ 2005-01-11 15:35:54.000000000 +1100 > > +++ xen/common/page_alloc.c 2005-01-19 14:51:47.000000000 +1100 > > @@ -203,10 +203,8 @@ > > #define NR_ZONES 2 > > > > /* Up to 2^10 pages can be allocated at once. */ > > -#define MIN_ORDER 0 > > #define MAX_ORDER 10 > > -#define NR_ORDERS (MAX_ORDER - MIN_ORDER + 1) > > -static struct list_head heap[NR_ZONES][NR_ORDERS]; > > +static struct list_head heap[NR_ZONES][MAX_ORDER]; > > This patch is broken because it replaces NR_ORDERS with MAX_ORDER, > not with MAX_ORDER+1.Yes, I should have fixed the comment as well. The +1 version went into CVS though, which I feel is un-C-like, but I don''t feel strongly about it.> > @@ -251,17 +249,18 @@ > > int i; > > struct pfn_info *pg; > > > > - if ( unlikely(order < MIN_ORDER) || unlikely(order > MAX_ORDER) ) > > + ASSERT(order >= 0); > > + if ( unlikely(order >= MAX_ORDER) ) > > return NULL; > > Why is it valid to change ''return NULL'' to a failed ASSERT?Because noone should be handing a negative number there? In CVS it''s now an unsigned anyway.> Also changing > to >= is wrong.Well, it''s consistent with the rest of the patch. (sorry for the slow reply, reading mailing list with latency). Thanks, Rusty. -- A bad analogy is like a leaky screwdriver -- Richard Braakman ------------------------------------------------------- This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting Tool for open source databases. Create drag-&-drop reports. Save time by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc. Download a FREE copy at http://www.intelliview.com/go/osdn_nl _______________________________________________ Xen-devel mailing list Xen-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/xen-devel
David Hopwood
2005-Feb-02 23:31 UTC
Re: [Xen-devel] [PATCH] Trivial fix for latent bug in page_alloc.c
Rusty Russell wrote:>>>@@ -251,17 +249,18 @@ >>> int i; >>> struct pfn_info *pg; >>> >>>- if ( unlikely(order < MIN_ORDER) || unlikely(order > MAX_ORDER) ) >>>+ ASSERT(order >= 0); >>>+ if ( unlikely(order >= MAX_ORDER) ) >>> return NULL; >>[...]>>Also changing > to >= is wrong. > > Well, it''s consistent with the rest of the patch.How so? ''order == MAX_ORDER'' is possible and valid, unless MAX_ORDER is misnamed. -- David Hopwood <david.nospam.hopwood@blueyonder.co.uk> ------------------------------------------------------- This SF.Net email is sponsored by: IntelliVIEW -- Interactive Reporting Tool for open source databases. Create drag-&-drop reports. Save time by over 75%! Publish reports on the web. Export to DOC, XLS, RTF, etc. Download a FREE copy at http://www.intelliview.com/go/osdn_nl _______________________________________________ Xen-devel mailing list Xen-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/xen-devel
Rusty Russell
2005-Feb-08 02:32 UTC
Re: [Xen-devel] [PATCH] Trivial fix for latent bug in page_alloc.c
On Wed, 2005-02-02 at 23:31 +0000, David Hopwood wrote:> Rusty Russell wrote: > >>>@@ -251,17 +249,18 @@ > >>> int i; > >>> struct pfn_info *pg; > >>> > >>>- if ( unlikely(order < MIN_ORDER) || unlikely(order > MAX_ORDER) ) > >>>+ ASSERT(order >= 0); > >>>+ if ( unlikely(order >= MAX_ORDER) ) > >>> return NULL; > >> > [...] > >>Also changing > to >= is wrong. > > > > Well, it''s consistent with the rest of the patch. > > How so? ''order == MAX_ORDER'' is possible and valid, unless MAX_ORDER is > misnamed.Yes, I erred badly in not using NR_ORDERS, which lead to this conversation. Nomenclature is important, and I made a hash of it in this patch. Fortunately, greater minds such as yours spat it out 8) Thanks, Rusty. -- A bad analogy is like a leaky screwdriver -- Richard Braakman ------------------------------------------------------- SF email is sponsored by - The IT Product Guide Read honest & candid reviews on hundreds of IT Products from real users. Discover which products truly live up to the hype. Start reading now. http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click _______________________________________________ Xen-devel mailing list Xen-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/xen-devel