Re: [Xen-devel] [PATCH 03/27] xen/x86: mm: Don't check alloc_boot_pages return
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 GrallGiven that you adjust the commit message: Reviewed-by: Andre Przywara The first 3 patches were already committed a few days ago, so we would have to stick with the current message. Cheers, -- Julien Grall ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 03/27] xen/x86: mm: Don't check alloc_boot_pages return
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 GrallGiven that you adjust the commit message: Reviewed-by: Andre Przywara Cheers, Andre. > Cc: Jan Beulich > Cc: Andrew Cooper > > 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); > } > > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH 03/27] xen/x86: mm: Don't check alloc_boot_pages return
>>> On 14.08.17 at 16:23,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 This as well as the earlier two patches Reviewed-by: Jan Beulich Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
[Xen-devel] [PATCH 03/27] xen/x86: mm: Don't check alloc_boot_pages return
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--- Cc: Jan Beulich Cc: Andrew Cooper 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); } -- 2.11.0 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel