Re: [PATCH v2 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init()

Re: [PATCH v2 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init()

[Matt Fleming]

On Thu, 05 Jan, at 05:12:42PM, Dave Young wrote:
> On 12/22/16 at 11:23am, Nicolai Stange wrote:
> > Before invoking the arch specific handler, efi_mem_reserve() reserves
> > the given memory region through memblock.
> > 
> > efi_mem_reserve() can get called after mm_init() though -- through
> > efi_bgrt_init(), for example. After mm_init(), memblock is dead and should
> > not be used anymore.
> 
> It did not fail during previous test so we did not catch this bug, if memblock
> can not be used after mm_init(), IMHO it should fail instead of silently succeed.
 
This must literally be the fifth time or so that I've been caught out
by this over the years because there's no hard error if you call the
memblock code after slab and co. are up.

MM folks, is there some way to catch these errors without requiring
the sprinkling of slab_is_available() everywhere?

> Matt, can we move the efi_mem_reserve to earlier code for example in
> efi_memblock_x86_reserve_range just after reserving the memmap?
 
No, it *needs* to be callable from efi_bgrt_init(), because you only
want to reserve those regions if you have the BGRT driver available.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init()

[Mel Gorman]

On Mon, Jan 09, 2017 at 11:44:00AM +0000, Matt Fleming wrote:
> On Thu, 05 Jan, at 05:12:42PM, Dave Young wrote:
> > On 12/22/16 at 11:23am, Nicolai Stange wrote:
> > > Before invoking the arch specific handler, efi_mem_reserve() reserves
> > > the given memory region through memblock.
> > > 
> > > efi_mem_reserve() can get called after mm_init() though -- through
> > > efi_bgrt_init(), for example. After mm_init(), memblock is dead and should
> > > not be used anymore.
> > 
> > It did not fail during previous test so we did not catch this bug, if memblock
> > can not be used after mm_init(), IMHO it should fail instead of silently succeed.
>  
> This must literally be the fifth time or so that I've been caught out
> by this over the years because there's no hard error if you call the
> memblock code after slab and co. are up.
> 
> MM folks, is there some way to catch these errors without requiring
> the sprinkling of slab_is_available() everywhere?
> 

Well, you could put in a __init global variable about availability into
mm/memblock.c and then check it in memblock APIs like memblock_reserve()
to BUG_ON? I know BUG_ON is frowned upon but this is not likely to be a
situation that can be sensibly recovered.

-- 
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init()

[Matt Fleming]

On Mon, 09 Jan, at 01:31:52PM, Mel Gorman wrote:
> 
> Well, you could put in a __init global variable about availability into
> mm/memblock.c and then check it in memblock APIs like memblock_reserve()
> to BUG_ON? I know BUG_ON is frowned upon but this is not likely to be a
> situation that can be sensibly recovered.

Indeed. I've only ever seen this situation lead to silent memory
corruption and bitter tears.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init()

[Dave Young]

On 01/09/17 at 11:44am, Matt Fleming wrote:
> On Thu, 05 Jan, at 05:12:42PM, Dave Young wrote:
> > On 12/22/16 at 11:23am, Nicolai Stange wrote:
> > > Before invoking the arch specific handler, efi_mem_reserve() reserves
> > > the given memory region through memblock.
> > > 
> > > efi_mem_reserve() can get called after mm_init() though -- through
> > > efi_bgrt_init(), for example. After mm_init(), memblock is dead and should
> > > not be used anymore.
> > 
> > It did not fail during previous test so we did not catch this bug, if memblock
> > can not be used after mm_init(), IMHO it should fail instead of silently succeed.
>  
> This must literally be the fifth time or so that I've been caught out
> by this over the years because there's no hard error if you call the
> memblock code after slab and co. are up.
> 
> MM folks, is there some way to catch these errors without requiring
> the sprinkling of slab_is_available() everywhere?
> 
> > Matt, can we move the efi_mem_reserve to earlier code for example in
> > efi_memblock_x86_reserve_range just after reserving the memmap?
>  
> No, it *needs* to be callable from efi_bgrt_init(), because you only
> want to reserve those regions if you have the BGRT driver available.

It is true that it depends on acpi init, I was wondering if bgrt parsing can
be moved to early acpi code. But anyway I'm not sure it is doable and
worth.

Thanks
Dave 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init()

[Matt Fleming]

On Tue, 10 Jan, at 08:37:35AM, Dave Young wrote:
> 
> It is true that it depends on acpi init, I was wondering if bgrt parsing can
> be moved to early acpi code. But anyway I'm not sure it is doable and
> worth.

That's a good question. I think I gave up last time I tried to move
the BGRT code to early boot because of the dependencies involved with
having the ACPI table parsing code initialised.

But if you want to take a crack at it, I'd be happy to review the
patches.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init()

[Dave Young]

On 01/10/17 at 12:51pm, Matt Fleming wrote:
> On Tue, 10 Jan, at 08:37:35AM, Dave Young wrote:
> > 
> > It is true that it depends on acpi init, I was wondering if bgrt parsing can
> > be moved to early acpi code. But anyway I'm not sure it is doable and
> > worth.
> 
> That's a good question. I think I gave up last time I tried to move
> the BGRT code to early boot because of the dependencies involved with
> having the ACPI table parsing code initialised.
> 
> But if you want to take a crack at it, I'd be happy to review the
> patches.

Ok, I will have a try. 

Thanks
Dave

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 2/2] efi: efi_mem_reserve(): don't reserve through memblock after mm_init()

[Matt Fleming]

On Mon, 09 Jan, at 01:31:52PM, Mel Gorman wrote:
> 
> Well, you could put in a __init global variable about availability into
> mm/memblock.c and then check it in memblock APIs like memblock_reserve()
> to BUG_ON? I know BUG_ON is frowned upon but this is not likely to be a
> situation that can be sensibly recovered.

What about something like this?

BUG_ON() shouldn't actually be necessary because I couldn't think of a
situation where A) memblock would be unavailable and B) returning an
error would prevent us from making progress.

---->8----