Messages in this thread | | | Date | Sat, 28 Jan 2017 02:09:03 +0800 | From | Wei Yang <> | Subject | Re: [Resend PATCH] x86/e820: break the loop when the region is less then current region |
| |
Hi, Ingo
Glad to see your comments.
On Fri, Jan 27, 2017 at 09:47:23AM +0100, Ingo Molnar wrote: > >* Wei Yang <richard.weiyang@gmail.com> wrote: > >> e820_all_mapped() iterates the e820 table to check whether a region is >> mapped or not. Since the e820 table is sorted, when the region is less than >> the current region, no need to continue the iteration. >> >> The patch breaks the loop accordingly. >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> >> --- >> arch/x86/kernel/e820.c | 8 +++++++- >> 1 file changed, 7 insertions(+), 1 deletion(-) >> >> diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c >> index 90e8dde..f4fb197 100644 >> --- a/arch/x86/kernel/e820.c >> +++ b/arch/x86/kernel/e820.c >> @@ -86,10 +86,16 @@ int __init e820_all_mapped(u64 start, u64 end, unsigned type) >> for (i = 0; i < e820->nr_map; i++) { >> struct e820entry *ei = &e820->map[i]; >> >> + /* Since the e820 table is sorted, when the region is less >> + * than the current region, break it. >> + */ >> + if (ei->addr >= end) >> + break; > >Please have a look at the relevant sections in Documentation/CodingStyle. (And >yes, this file violates it in a number of ways, but that's no reason to add to the >mess.) >
I took a look in the Documentation/CodingStyle, looks the comment style is not the preferred one. While I have checked this by scripts/checkpatch.pl, which prompts 0 error and 0 warning, so I thought it is fine.
I would change this if you like.
>But, more importantly, the reason I have not applied the patch before is that >while it's true that the e820 map is _eventually_ sorted, have you made certain >that all calls to e820_all_mapped() are done when the map is already sorted? >
I think so, if not those caller really need to make sure not to do this at that moment.
The e820_all_mapped() can't work well if it is not sorted, even without this change.
For example, we have two ranges [0x1000, 0x1FFF] and [0x2000, 0x2FFF] but they are not sorted in e820. Then a range [0x1500, 0x2500] would be judge not all mapped, which actually is in range [0x1000, 0x2FFF].
BTW, I went through all the callers, and most of them are called from subsys_init() or arch_initcall() which is after setup_memory_map() called in setup_arch().
And two of them are called by e820_add_kernel_range() and efi_reserve_boot_services(), and both are after setup_memory_map().
>Thanks, > > Ingo
-- Wei Yang Help you, Help me [unhandled content-type:application/pgp-signature] | |