bug-hurd
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Replacing init_alloc_aligned with a bitmapped allocator


From: Samuel Thibault
Subject: Re: Replacing init_alloc_aligned with a bitmapped allocator
Date: Thu, 15 Apr 2010 02:09:57 +0200
User-agent: Mutt/1.5.12-2006-07-14

Hello,

Karim Allah Ahmed, le Wed 14 Apr 2010 13:11:05 +0200, a écrit :
> The current "init_alloc_aligned" function will skip regions of memory
> during an allocation if they aren't of "size" bytes of contigous
> physical memory , and will never reclaim them later. ( it can't do
> that with a simple "available_next" )

Mmm, indeed.

> In addition to the fact that it is unable to reserve any low memory
> (<MAX_DMA_ADDRESS) during the allocation phase because it didn't have

Is this really a concern with nowadays hardware? Apparently, only the
floppy disk, BusLogic scsi, and 3c505 network card have issues with DMAs
above 16MB.  I'm not sure it is worth caring about them any more. Floppy
disks could be a reason, but well they're slow anyway, not a problem to
use PIO there.

> [Note: The current bitmap is saved starting at address 0x4000 , which
> i think is a save place during initialization]

It is not necessarily.  You can not assume anything about modules being
loaded here or there by grub etc.  In the Xen case, it's actually almost
certain that 0x4000 is busy :)

To make it work properly, I guess the cleanest way would be, in
init_boot_allocator, to dynamically build an array of the excluded
ranges (using alloca to store the array). Then you can iterate over the
physical memory, excluding the excluded ranges, up to finding a place
where to put the bitmap. And then you can iterate over the exclusion
array to invalidate these ranges from the bitmap. That way the
"exclusion" information appears only once in the source code.

> +void set_bit(unsigned long nr, volatile vm_offset_t addr)
> +{
> +     test_and_set_bit(nr,addr);
> +}

It is quite odd to use test_and_set_bit just to set a bit...

> +int test_and_set_bit(unsigned long nr, volatile vm_offset_t addr)
> +{
> +     
> +        int oldbit;
> +
> +     __asm__ __volatile__( 
> +           "btsl %2,%1\n\tsbbl %0,%0"
> +           :"=r" (oldbit),"=m" (*(void volatile *)addr)
> +           :"Ir" (nr) : "memory");
> +     return oldbit;
> +}

Where does this code come from?  You can not throw code like this
without proper author and licence information.

In this case, it's not hard to just write generic versions of
set_bit/clear_bit/test_bit.

> diff --git a/i386/i386at/model_dep.c b/i386/i386at/model_dep.c
> index ca00078..6afb213 100644
> --- a/i386/i386at/model_dep.c
> +++ b/i386/i386at/model_dep.c
> @@ -32,10 +32,9 @@
>   *   Basic initialization for I386 - ISA bus machines.
>   */
>  
> +#include <i386/bitops.h>
>  #include <string.h>
> -
>  #include <device/cons.h>
> -
>  #include <mach/vm_param.h>
>  #include <mach/vm_prot.h>
>  #include <mach/machine.h>

Do not remove \n, there are here to separate generic C stuff (string.h),
device stuff, and mach kernel stuff. Here, i386/bitops should go along
the others below.

> @@ -74,6 +73,7 @@
>  #include <xen/xen.h>
>  #endif       /* MACH_XEN */
>  
> +
>  /* Location of the kernel's symbol table.
>     Both of these are 0 if none is available.  */
>  #if MACH_KDB

Before submitting, please check your patch and clean such dummy
modifications away.

> @@ -104,8 +104,11 @@ unsigned long la_shift = VM_MIN_KERNEL_ADDRESS;
>  struct multiboot_info boot_info;
>  #endif       /* MACH_XEN */
>  
> +/* To make sure that the boot memory allocator is initialized */
> +int boot_allocator_initialized= 0;

This isn't so much a helping comment. When is this set, what type of
functions is it for?

> -             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.

> +             bootmap_free(atop(boot_info.cmdline - phys_first_addr), 
> atop(round_page(kernel_cmdline_len)));

Take care with such computations. Note that you can not assume what is
just before the cmdline. It could be important information that mustn't
be overwritten. Since you can't assume the cmdline is aligned on a page,
you have to round the cmdline address up to the next page. Same for the
end of the cmdline: round it down to the previous page.

> +             bootmap_free(atop(round_page(boot_info.mods_addr)), 
> atop(round_page(size) ));

Same here.

> -                     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?

> @@ -395,7 +412,7 @@ i386at_init(void)
>  #if PAE
>       /* PAE page tables are 2MB only */
>       kernel_page_dir[lin2pdenum(VM_MIN_KERNEL_ADDRESS) + 1] =
> -             kernel_page_dir[lin2pdenum(LINEAR_MIN_KERNEL_ADDRESS) + 1];
> +     kernel_page_dir[lin2pdenum(LINEAR_MIN_KERNEL_ADDRESS) + 1];

The indentation was on purpose.

> +typedef struct init_bootmap {
> +     vm_offset_t bitmap;
> +     vm_offset_t next_avail;
> +} init_bootmap_t;
> +
> +static init_bootmap_t bootmap;

Mmm, is it really worth defining a structure just for this?

> +void
> +bootmap_free(unsigned long sidx, unsigned long pages)
>  {
> -     vm_offset_t addr;
> +     unsigned long eidx = sidx + pages;
> +     for (; sidx < eidx ; sidx++)
> +             clear_bit(sidx,bootmap.bitmap);
> +}

Ideally this should be optimized to do whole word clears.

> +void
> +init_boot_allocator(void){

See my comment above.

> +boolean_t
> +init_alloc_aligned(vm_size_t size, vm_offset_t *addrp)
> +{
> +     size = round_page(size);
> +     if(!_init_alloc_aligned(size, addrp,bootmap.next_avail)){
> +             if(_init_alloc_aligned(size,addrp,0)){
> +                     bootmap.next_avail = *addrp + size;
> +                     return TRUE;
> +             }else{
> +                     return FALSE;
> +             }
> +     }else{
> +             bootmap.next_avail = *addrp + size;
> +             return TRUE;
> +     }
> +}

Add comments: what do the ifs mean?

Samuel




reply via email to

[Prev in Thread] Current Thread [Next in Thread]