From mboxrd@z Thu Jan 1 00:00:00 1970 From: David Hopwood Subject: Re: [PATCH] Trivial fix for latent bug in page_alloc.c Date: Wed, 19 Jan 2005 16:45:50 +0000 Message-ID: <41EE8EBE.2080900@blueyonder.co.uk> References: <1106106821.20879.14.camel@localhost.localdomain> Reply-To: david.nospam.hopwood@blueyonder.co.uk Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1106106821.20879.14.camel@localhost.localdomain> Sender: xen-devel-admin@lists.sourceforge.net Errors-To: xen-devel-admin@lists.sourceforge.net List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , List-Archive: To: xen-devel@lists.sourceforge.net List-Id: xen-devel@lists.xenproject.org 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 ------------------------------------------------------- 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