[PATCH] Trivial fix for latent bug in page_alloc.c

[PATCH] Trivial fix for latent bug in page_alloc.c

[Rusty Russell]

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

Re: [PATCH] Trivial fix for latent bug in page_alloc.c

[Keir Fraser]

> 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

Re: [PATCH] Trivial fix for latent bug in page_alloc.c

[David Hopwood]

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

Re: [PATCH] Trivial fix for latent bug in page_alloc.c

[Rusty Russell]

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

Re: [PATCH] Trivial fix for latent bug in page_alloc.c

[David Hopwood]

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

Re: [PATCH] Trivial fix for latent bug in page_alloc.c

[Rusty Russell]

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