[Xen-devel,03/27] xen/x86: mm: Don't check alloc_boot_pages return

Message ID 20170814142418.13267-4-julien.grall@arm.com
State Accepted
Commit 79d5dd06a677fcc8c5a585d95b32c35bd38bc34e
Headers show
Series
  • xen/arm: Memory subsystem clean-up
Related show

Commit Message

Julien Grall Aug. 14, 2017, 2:23 p.m.
The only way alloc_boot_pages will return 0 is during the error case.
Although, Xen will panic in the error path. So the check in the caller
is pointless.

Looking at the loop, my understanding is it will try to allocate in
smaller chunk if a bigger chunk fail. Given that alloc_boot_pages can
never check, the loop seems unecessary.

Signed-off-by: Julien Grall <julien.grall@arm.com>

---

Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

I haven't tested this code, only build test it. I can't see how
alloc_boot_pages would return 0 other than the error path.
---
 xen/arch/x86/mm.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

Comments

Jan Beulich Aug. 15, 2017, 3:55 p.m. | #1
>>> On 14.08.17 at 16:23, <julien.grall@arm.com> wrote:
> The only way alloc_boot_pages will return 0 is during the error case.
> Although, Xen will panic in the error path. So the check in the caller
> is pointless.
> 
> Looking at the loop, my understanding is it will try to allocate in
> smaller chunk if a bigger chunk fail. Given that alloc_boot_pages can
> never check, the loop seems unecessary.

Long, long ago alloc_boot_pages() could return 0 without
panic()-ing or BUG()-ing.

> Signed-off-by: Julien Grall <julien.grall@arm.com>

This as well as the earlier two patches
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan
Andre Przywara Aug. 22, 2017, 11:28 a.m. | #2
Hi,

On 14/08/17 15:23, Julien Grall wrote:
> The only way alloc_boot_pages will return 0 is during the error case.

This statement is not true. If alloc_boot_pages() returns, it has
succeeded. Returning 0 is nothing special.

> Although, Xen will panic in the error path. So the check in the caller
> is pointless.
> 
> Looking at the loop, my understanding is it will try to allocate in
> smaller chunk if a bigger chunk fail. Given that alloc_boot_pages can
> never check, the loop seems unecessary.

Agreed, the algorithm doesn't work with (current) implementation of
alloc_boot_pages(), so the patch is valid.

> Signed-off-by: Julien Grall <julien.grall@arm.com>

Given that you adjust the commit message:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre.

> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> I haven't tested this code, only build test it. I can't see how
> alloc_boot_pages would return 0 other than the error path.
> ---
>  xen/arch/x86/mm.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
> index f53ca43554..66e337109d 100644
> --- a/xen/arch/x86/mm.c
> +++ b/xen/arch/x86/mm.c
> @@ -200,11 +200,7 @@ static void __init init_frametable_chunk(void *start, void *end)
>           */
>          while ( step && s + (step << PAGE_SHIFT) > e + (4 << PAGE_SHIFT) )
>              step >>= PAGETABLE_ORDER;
> -        do {
> -            mfn = alloc_boot_pages(step, step);
> -        } while ( !mfn && (step >>= PAGETABLE_ORDER) );
> -        if ( !mfn )
> -            panic("Not enough memory for frame table");
> +        mfn = alloc_boot_pages(step, step);
>          map_pages_to_xen(s, mfn, step, PAGE_HYPERVISOR);
>      }
>  
>
Julien Grall Aug. 22, 2017, 5:28 p.m. | #3
On 22/08/17 12:28, Andre Przywara wrote:
> Hi,

Hi Andre,

>
> On 14/08/17 15:23, Julien Grall wrote:
>> The only way alloc_boot_pages will return 0 is during the error case.
>
> This statement is not true. If alloc_boot_pages() returns, it has
> succeeded. Returning 0 is nothing special.
>
>> Although, Xen will panic in the error path. So the check in the caller
>> is pointless.
>>
>> Looking at the loop, my understanding is it will try to allocate in
>> smaller chunk if a bigger chunk fail. Given that alloc_boot_pages can
>> never check, the loop seems unecessary.
>
> Agreed, the algorithm doesn't work with (current) implementation of
> alloc_boot_pages(), so the patch is valid.
>
>> Signed-off-by: Julien Grall <julien.grall@arm.com>
>
> Given that you adjust the commit message:
>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>

The first 3 patches were already committed a few days ago, so we would 
have to stick with the current message.

Cheers,

Patch

diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index f53ca43554..66e337109d 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -200,11 +200,7 @@  static void __init init_frametable_chunk(void *start, void *end)
          */
         while ( step && s + (step << PAGE_SHIFT) > e + (4 << PAGE_SHIFT) )
             step >>= PAGETABLE_ORDER;
-        do {
-            mfn = alloc_boot_pages(step, step);
-        } while ( !mfn && (step >>= PAGETABLE_ORDER) );
-        if ( !mfn )
-            panic("Not enough memory for frame table");
+        mfn = alloc_boot_pages(step, step);
         map_pages_to_xen(s, mfn, step, PAGE_HYPERVISOR);
     }