weirdness in bootmem_init(), arch/mips64/kernel/setup.c

weirdness in bootmem_init(), arch/mips64/kernel/setup.c

[Andrew Clausen]

Hi all,

This code isn't really relevant to what I'm working on (it isn't compiled
in to kernels for the ip27), but I just noticed it, and it looks broken:

        /* Find the highest page frame number we have available.  */
        max_pfn = 0;
        for (i = 0; i < boot_mem_map.nr_map; i++) {
                unsigned long start, end;

                if (boot_mem_map.map[i].type != BOOT_MEM_RAM)
                        continue;

*****           start = PFN_UP(boot_mem_map.map[i].addr);
*****           end = PFN_DOWN(boot_mem_map.map[i].addr
                      + boot_mem_map.map[i].size);

*****           if (start >= end)
                        continue;
                if (end > max_pfn)
                        max_pfn = end;
        }


That test looks like it will always succeed... and it looks like the
author wanted it to be a sanity check.

Why all this business with PFN_UP and PFN_DOWN?  (They are bit
shifts... PFN_UP shifts left, PFN_DOWN shifts right)

Cheers,
Andrew

Re: weirdness in bootmem_init(), arch/mips64/kernel/setup.c

[Juan Quintela]

>>>>> "andrew" == Andrew Clausen <clausen@melbourne.sgi.com> writes:

andrew> Hi all,
andrew> This code isn't really relevant to what I'm working on (it isn't compiled
andrew> in to kernels for the ip27), but I just noticed it, and it looks broken:

andrew> /* Find the highest page frame number we have available.  */
andrew> max_pfn = 0;
andrew> for (i = 0; i < boot_mem_map.nr_map; i++) {
andrew> unsigned long start, end;

andrew> if (boot_mem_map.map[i].type != BOOT_MEM_RAM)
andrew> continue;

andrew> *****           start = PFN_UP(boot_mem_map.map[i].addr);
andrew> *****           end = PFN_DOWN(boot_mem_map.map[i].addr
andrew> + boot_mem_map.map[i].size);

andrew> *****           if (start >= end)
andrew> continue;
andrew> if (end > max_pfn)
andrew> max_pfn = end;
andrew> }


andrew> That test looks like it will always succeed... and it looks like the
andrew> author wanted it to be a sanity check.

andrew> Why all this business with PFN_UP and PFN_DOWN?  (They are bit
andrew> shifts... PFN_UP shifts left, PFN_DOWN shifts right)

Not completely sure, but I think that it is related with the weird
discontig memory that Origins (and I think other MIPS machines) have.

(Just having put the file where that thing are, will have saved me a
grep :)

1st- Looking at the code, both of them shift right:

#define PFN_UP(x)	(((x) + PAGE_SIZE - 1) >> PAGE_SHIFT)
#define PFN_DOWN(x)	((x) >> PAGE_SHIFT)

PFN_UP -> page frame of next page
PFN_DOWN -> page frame of this page

2nd - if the region is empty (size = 0), start will be == end, which
      means that we don't considerd that area for checking what memory
      are available.

Standard disclaimer: That is my reading/things that I remind about
   that, any resemblance with reality can be pure coincidence :p  I am
   not an expert in SGI machines lowlevel details but any mean.

Later, Juan.

-- 
In theory, practice and theory are the same, but in practice they 
are different -- Larry McVoy

Re: weirdness in bootmem_init(), arch/mips64/kernel/setup.c

[Ralf Baechle]

On Tue, Feb 18, 2003 at 11:27:23AM +0100, Juan Quintela wrote:

> andrew> That test looks like it will always succeed... and it looks like the
> andrew> author wanted it to be a sanity check.
> 
> andrew> Why all this business with PFN_UP and PFN_DOWN?  (They are bit
> andrew> shifts... PFN_UP shifts left, PFN_DOWN shifts right)
> 
> Not completely sure, but I think that it is related with the weird
> discontig memory that Origins (and I think other MIPS machines) have.

It's not used on Origins.

  Ralf

Re: weirdness in bootmem_init(), arch/mips64/kernel/setup.c

[Ralf Baechle]

On Tue, Feb 18, 2003 at 05:54:27PM +1100, Andrew Clausen wrote:

> This code isn't really relevant to what I'm working on (it isn't compiled
> in to kernels for the ip27), but I just noticed it, and it looks broken:
> 
>         /* Find the highest page frame number we have available.  */
>         max_pfn = 0;
>         for (i = 0; i < boot_mem_map.nr_map; i++) {
>                 unsigned long start, end;
> 
>                 if (boot_mem_map.map[i].type != BOOT_MEM_RAM)
>                         continue;
> 
> *****           start = PFN_UP(boot_mem_map.map[i].addr);
> *****           end = PFN_DOWN(boot_mem_map.map[i].addr
>                       + boot_mem_map.map[i].size);
> 
> *****           if (start >= end)
>                         continue;
>                 if (end > max_pfn)
>                         max_pfn = end;
>         }
> 
> 
> That test looks like it will always succeed... and it looks like the
> author wanted it to be a sanity check.

> Why all this business with PFN_UP and PFN_DOWN?  (They are bit
> shifts... PFN_UP shifts left, PFN_DOWN shifts right)

Read again.  PFN_PHYS is shifting left, the others shift right.

Mm is based on complete pages and page numbers.  This code simply discards
partial pages before initializing mm with the list of available memory.
The case start > end cannot happen but start = end is possible for small
areas near the end of a page - but such an area is not usable for mm so
it's ignored.

  Ralf

Re: weirdness in bootmem_init(), arch/mips64/kernel/setup.c

[Andrew Clausen]

On Tue, Feb 18, 2003 at 11:42:44AM +0100, Ralf Baechle wrote:
> Read again.  PFN_PHYS is shifting left, the others shift right.

Ooops, sorry for the noise...

Cheers,
Andrew