On Mon, 2005-02-21 at 14:24, Dave Hansen wrote:
> On Mon, 2005-02-21 at 14:03 -0800, keith wrote:
> > On Mon, 2005-02-21 at 12:39, Dave Hansen wrote:
> > > On Mon, 2005-02-21 at 12:17 -0800, keith wrote:
> > > > +               if (node_has_online_mem(nid)){
> > > > +                       if (start > low) {
> > > 
> > > Instead of indenting another level, can you just put a continue in the
> > > loop?  I think it makes it much easier to read.  
> > 
> > I cannot put a continue here.  I know it makes ugly code worse but we
> > have to call free area_init_node in all cases.   
> 
> If !node_has_online_mem(nid), then (node_start_pfn[nid] ==
> node_end_pfn[nid]), and running through this if() won't hurt anything
> here:

node_start_pfn[nid] == node_end_pfn[nid] == 0 
start and high are both 0.  That blows the chunk of code up :)

In the no memory in a node case things look like:
start  = 0
high = 0
low = max_low_pfn. 

> >                         if (start > low) {
> > #ifdef CONFIG_HIGHMEM
> >                                 BUG_ON(start > high);
> >                                 zones_size[ZONE_HIGHMEM] = high - start;
> > #endif
> >                         }

 start is 0 and low is max_low_pfn so (start < low) so I catch 
BUG_ON(low > high) in the else part the if. 
 
Since the right zone_sizes is 0 for everything I think it is best just
to skip that section of code altogether. 

> high==start, so the bug won't trip, and it will set
> zones_size[ZONE_HIGHMEM]=0, which is also OK.  Can you do this?
> 
> -               if (start > low) {
> +               if (node_has_online_mem(nid) || (start > low)) {

No, it is the else of that "if" that kills the kernel.  start < low.  
The zone_sizes will all be 0 in the !node_has_online_mem case. They are
initialized to 0 they stay that way as free area_init_node is called. 

> > +#define node_has_online_mem(nid) !(node_start_pfn[nid] == node_end_pfn[nid]) 
> > +/*
> > +inline int __node_has_online_mem(int nid) {
> > +        return !(node_start_pfn[nid]== node_end_pfn[nid]);
> > +}
> > +*/
> 
> You probably want to kill the extra definition.  Also, I prefer

thanks for catching that :)
> 
> 	(node_start_pfn[nid] != node_end_pfn[nid])
> 
> to
> 
> 	!(node_start_pfn[nid] == node_end_pfn[nid])
> 
> But, that's the most minor of nits.  
easy to do. 


Keith