On Wed, 2005-08-03 at 15:45 -0700, Luck, Tony wrote:
> >one on x86. This patch is relative to 2.6.13-rc3 and applies on top of
> >the EFI memory map walk rewrite patch at
> ><http://free.linux.hp.com/~khalid/ia64/efi_memmapwalk_2.6.13rc3.patch>
> >(Tony, this EFI memory map walk patch is same as the one I sent you this
> >morning).
>
> Khalid,
>
> Thanks for working on this. I'm sorry it has taken this long to look
> at it.
>
> I think some areas can still benefit from more simplification. The only
> place I see you split a kern_memdesc_t is in efi_trim_memory() in order
> to limit memory according to either of mem_limit or max_addr. Wouldn't
> it be simpler to just adjust num_pages element of the element instead
> of splitting?
>
> If you do that ... then you don't need to have a linked list of kern_memdesc
> structures, you can treat them just like an array, nor do you need
> MEM_DESC_SAFETY_MARGIN
>
> Likewise the granule alignment functions. The original trim_top() and
> trim_bottom() are insanely complex ... and perhaps you were led astray
> trying to duplicate their behaivour? I believe that you should end up
> with the desired behaivour if you just do any coalescing of memory blocks
> that are WB and have one of the allowable types, then round the base
> addresses up to granule boundaries and the tops down. All that scanning
> around looking for holes or non-WB sections of memory looks pointless to
> me ... perhaps I'm missing some incredible subtlety in the original?
>
> By only copying from "is_available()" types into kern_memdesc_t structures
> you can avoid calling is_available() in your new efi_memmap_walk(), and
> indeed drop the "type" field from the structure.
>
> find_memmap_space() should be in efi.c ... just pass a pointer to space
> for it to fill in the address of the block it allocated and have it
> return the size so that reserve_memory() can fill these into an entry
> in rsvd_region[]. It shouldn't use is_available_memory() to decide
> which blocks are candidates for the allocation. This could result in
> choosing memory that is still in use by EFI, command line arguments, or
> even overwriting the kernel. EFI_CONVENTIONAL_MEMORY is definitely
> safe for this, perhaps EFI_LOADER_CODE too?
>
> It is not OK to call "machine_restart()" if you couldn't allocate space.
> This would put the machine into a loop ... rebooting and failing, with
> no oportunity to read the error message. Just use panic().
>
> -Tony
>
Tony,
Thanks for feedback. I will take a look at these and see if I can
simplify it further.
--
Khalid
====================================================================
Khalid Aziz Open Source and Linux Organization
(970)898-9214 Hewlett-Packard
[EMAIL PROTECTED] Fort Collins, CO
"The Linux kernel is subject to relentless development"
- Alessandro Rubini
-
To unsubscribe from this list: send the line "unsubscribe linux-ia64" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
