Submitted For Peer Review: My Memory Manager / Paging Mech.

[astrocrep]

Located at address: http://dev.drcoders.com/ukon/review/

You will find a couple of files.

My Physical Memory Allocator called: allocator.c and .h
My Virtual Memory Allocator (Paging): called pageman.c and .h
My Higher Level Memory Allocator (malloc): Borrowed from old Freedows code
Lastly: my gdt.c which is used from bran, and the mm_low.s which includes more code from bran along with cr0 and cr3 access registers...

My purpose of this message is to submit this code to my peers for review. I would like to know where I have areas of improvment, where I may have designed pit-falls, if there are any blantant bugs... etc etc etc... I will be getting back into OS dev, and rewriting my kernel. I was hoping to be able to reuse much of my mem/paging code.

Its not to much code, and it does have *some* comments.

I believe that the code is fairly stable and seems to run without any issues.

Below is how the code is used (initialized).

Thanks in advance for your help.
Rich

Code: Select all

gdt_install();

...

//kernel_end is a label at the end of my linker script. 
// the + 256k is to give it some breaking room before the phys memory map starts
// The second number is the number of bytes required to represent each page as a bit


mm_install(kernel_end + (256 * 1024), (mbd->mem_upper + 1024) / 32);

//Now we loop through grubs memory mapper and reserve everything it tells us to...		
mmap = (memory_map_t *) (mbd->mmap_addr); 
while((unsigned long) mmap < mbd->mmap_addr + mbd->mmap_length) 
 { 
  if (mmap->type != 1)
   mm_reserve(mmap->base_addr_low,mmap->length_low);
  mmap = (memory_map_t *) ((unsigned long) mmap + mmap->size + sizeof (mmap->size)); 
 }

//Start the VMM (Paging)
vmm_install();

[Brendan]

Hi,

So far I've only looked at your physical memory manager...

First, I think there's a problem with the way you initialize your physical memory manager's bitmap. In the system memory map from Grub/BIOS, some areas are reserved by the BIOS, some areas are RAM and some areas aren't listed at all. With the way you're initializing the bitmap the areas that aren't listed in Grub's system memory map would end up being marked as "free RAM" (when they're not RAM or reserved).

Basically your initialization should be:

Code: Select all

Mark all bits in the bitmap as "not free RAM"
For each area in Grub's system memory map {
    if(the area's type is RAM) {
        for each whole page that's in the area {
            change the page's bit to "free RAM" in the bitmap
        }
    }
}
For each page that's reserved by the OS {
    change the page's bit to "not free RAM" in the bitmap
}
For each page that's currently in use {
    change the page's bit to "not free RAM" in the bitmap
}

For the bitmap itself, use an array of 32-bit integers instead of an array of 8-bit integers so that you can do "if(memory_bitmap == 0xFFFFFFFF)" to check 32 bits/pages at once. Also it's worth keeping track of the "highest possibly free page" (or lowest) to reduce the time spent searching - e.g. if the highest 2 GB of RAM is already in use, then you'd avoid checking 64 KB of the bitmap (or 524288 bits).

Your "mm_alloc()" could be something like:

Code: Select all

unsigned long mm_alloc()
{
	int i;
	unsigned int bit_mask;
	unsigned long page;

	if ((free_pages_above16 + free_pages_below16) == 0)
		return 0;   //I hope the physical page at 0x00000000 can never be allocated!

	for (i = (highest_possibly_free_page >> 5); i >= 0; i--)
	{
		if(memory_bitmap[i] != 0xFFFFFFFF) {
			page = i << 5 + 7;
			bit_mask = 0x80000000;
			while( (memory_bitmap[i] & bit_mask) != 0) {
				page--;
				bit_mask = bit_mask >> 1;
			}
			memory_bitmap[i] |= bit_mask;
			highest_possibly_free_page = page - 1;
			if (page * 4096) >= 0x1000000) free_pages_above16--;
			else free_pages_below16--;
			return page;
		}
	}
	return 0;   //I hope the physical page at 0x00000000 can never be allocated!
}

Of course your "mm_free()" would need to update the "highest_possibly_free_page" variable too:

Code: Select all

int mm_free(unsigned long page)
{
	unsigned int bit_mask;

	if (page > (total_pages_above16 + total_pages_below16)) {
		//ERROR: Freeing page that can't be RAM
		return ????;
	}
	bit_mask = 1 << (page & 31);
	if( (memory_bitmap[page >> 5] & bit_mask) == 0) {
		//ERROR: Freeing page that's already free
		return ????;
	}
	memory_bitmap[page >> 5] &= ~bit_mask;

	if(highest_possibly_free_page < page) highest_possibly_free_page = page;

	// Note: The next line was "if (page * 4096 > 0x1000000)", which isn't right
	if (page * 4096 >= 0x1000000) free_pages_above16++;
	else free_pages_below16++;

	return TRUE;
}

The "check_page()" and "change_page()" functions should never be used/needed - they hide ways to improve the algorithms you're using and make it harder for the compiler to optimize the code.


Cheers,

Brendan