[Resend PATCH] x86/e820: break the loop when the region is less then current region

[Resend PATCH] x86/e820: break the loop when the region is less then current region

[Wei Yang]

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;
+
 		if (type && ei->type != type)
 			continue;
 		/* is the region (part) in overlap with the current region ?*/
-		if (ei->addr >= end || ei->addr + ei->size <= start)
+		if (ei->addr + ei->size <= start)
 			continue;
 
 		/* if the region is at the beginning of <start,end> we move
-- 
2.5.0

Re: [Resend PATCH] x86/e820: break the loop when the region is less then current region

[Ingo Molnar]

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

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?

Thanks,

	Ingo

Re: [Resend PATCH] x86/e820: break the loop when the region is less then current region

[Wei Yang]

[-- Attachment #1: Type: text/plain, Size: 2531 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]