Re: Replacing init_alloc_aligned with a bitmapped allocator

[Samuel Thibault]

Karim Allah Ahmed, le Fri 16 Apr 2010 09:57:24 +0200, a écrit :
> On 4/15/10, Samuel Thibault <samuel.thibault@gnu.org> wrote:
> >> -          assert(init_alloc_aligned(round_page(len), &addr));
> >> +          assert(init_alloc_aligned(kernel_cmdline_len, &addr));
> >
> > This is not the same: init_alloc_aligned used to required size aligned
> > on the page size.
> >
> i don't understand this one .. the modified "init_alloc_aligned" does
> the same thing.

Then document it: init_alloc_aligned will round the allocation size up
to a page boundary.

> >> -                  vm_size_t size = m[i].mod_end - m[i].mod_start;
> >> +                  vm_size_t size = m[i].mod_end - m[i].mod_start + 1;
> >
> > Mmm, the multiboot spec is not so much clear whether mod_end is the
> > address of the last byte, or the address after the last byte. Your
> > change seems to imply the former, is it really so?
> >
> well, the specs said : "The first two fields contain the start and end
> addresses of the boot module itself" . so , i thought that this means
> that "mod_end" is the address of the last byte.

With such wording, it's more often the address after the last byte than
the address of the last byte. In any case, if the code was one way,
that's probably for some reason, and you shouldn't change it if you are
not sure of that it should really be.

I've had a grep in grub2:

./include/multiboot.h:  /* the memory used goes from bytes 'mod_start' to 
'mod_end-1' inclusive */
./include/multiboot.h:  multiboot_uint32_t mod_end;

./loader/i386/multiboot_mbi.c:    modlist[i].mod_end = modlist[i].mod_start + 
cur->size;

So it really is the address after the last byte.

> +++ b/i386/i386/bitops.h
> @@ -0,0 +1,35 @@
> +#ifndef _MACH_I386_BITOPS_H_
> +#define _MACH_I386_BITOPS_H_
> +
> +#include <mach/vm_param.h>
> +
> +int clear_bit(int nr, unsigned long * addr)

This still lacks proper author and licence information.

> --- a/i386/i386/model_dep.h
> +++ b/i386/i386/model_dep.h
> @@ -28,6 +28,8 @@
>  
>  #include <mach/std_types.h>
>  
> +#define DEREF(addr) (*((vm_offset_t *)(addr)))
> +

The name is too short. Either make it local to model_dep.c, and then it
can remain that short, or choose a longer, more descriptive name (at
least telling that you consider the pointer as pointing to vm_offset_ts).

> +/* 
> + * Set to 1 in init_boot_allocator
> + * _init_alloc_aligned always checks it before any allocatoin,
> + * and invokes init_boot_allocator if it was '0'
> + */
> +int boot_allocator_initialized= 0;

Ok, much more clear.

Now, that being said, it is not the way it usually is done in GNU Mach:
there usually are explicit initialization calls from c_boot_entry and
i386at_init.

> @@ -334,6 +333,8 @@ i386at_init(void)
>        */
>       mem_size_init();
>  
> +     init_boot_allocator();
> +
>  #ifdef MACH_XEN
>       kernel_cmdline = (char*) boot_info.cmd_line;
>  #else        /* MACH_XEN */

And this is actually already what you do...

> +             bootmap_free(atop(round_page(boot_info.cmdline - 
> phys_first_addr)), atop(trunc_page(kernel_cmdline_len)));
...
> +             bootmap_free(atop(round_page(boot_info.mods_addr)), 
> atop(trunc_page(size) ));
...
> +                     
> bootmap_free(atop(round_page(m[i].mod_start)),atop(trunc_page(size)));
...
> +                     
> bootmap_free(atop(round_page(m[i].string-phys_first_addr)),atop(trunc_page(round_page(size))));

The size computation is bogus: the truncation needs to be done for
the address of the end, not on the size (since the module doesn't
necessarily start on a page boundary).

> +                     vm_size_t size = m[i].mod_end - m[i].mod_start + 1;

As said above, really remove +1.

> +                     m[i].mod_end = addr + size - 1;

And remove -1.

> +/* boot_bitmap will be initially at 16*1024*1024 and we will see if this is 
> suitable */
> +static vm_offset_t boot_bitmap = 16*1024*1024;

Re-read what I said: you can not assume anything about where things
are.  There might be modules put at 16MB, or at 0x1000, etc. You can't
know.  Really do like I said, by first generating an array of all the
constraints, then find a place where to place the bitmap according to
constraints, and then record constraints in the bitmap.

Samuel