Re: [PATCH 2/2 v2] powerpc: Enable NO_BOOTMEM

[Emil Medve <Emilian.Medve@Freescale.com>]

Hello Scott,


On 05/06/2014 09:44 PM, Scott Wood wrote:
> On Tue, 2014-05-06 at 19:16 -0500, Emil Medve wrote:
>> Hello Scott,
>>
>>
>> On 05/06/2014 04:49 PM, Scott Wood wrote:
>>> On Tue, 2014-05-06 at 13:48 -0500, Emil Medve wrote:
>>>> Currently bootmem is just a wrapper around memblock. This gets rid of
>>>> the wrapper code just as other ARHC(es) did: x86, arm, etc.
>>>>
>>>> For now only cover !NUMA systems/builds
>>>>
>>>> Signed-off-by: Emil Medve <Emilian.Medve@Freescale.com>
>>>> ---
>>>>
>>>> v2: Acknowledge that NUMA systems/builds are not covered by this patch
>>>>
>>>>  arch/powerpc/Kconfig  | 3 +++
>>>>  arch/powerpc/mm/mem.c | 8 ++++++++
>>>>  2 files changed, 11 insertions(+)
>>>>
>>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>>> index e099899..07b164b 100644
>>>> --- a/arch/powerpc/Kconfig
>>>> +++ b/arch/powerpc/Kconfig
>>>> @@ -475,6 +475,9 @@ config SYS_SUPPORTS_HUGETLBFS
>>>>  
>>>>  source "mm/Kconfig"
>>>>  
>>>> +config NO_BOOTMEM
>>>> +	def_bool !NUMA
>>>
>>> This will allow a user to manually turn on CONFIG_NO_BOOTMEM in the
>>> presence of NUMA.  From the changelog it sounds like this is not what
>>> you intended.
>>>
>>> What are the issues with NUMA?
>>
>> Well, I don't have access to a NUMA box/board. I could enable NUMA for a
>> !NUMA board but I'd feel better if I could actually test/debug on a
>> relevant system
> 
> You could first test with NUMA on a non-NUMA board, and then if that
> works ask the list for help testing on NUMA hardware (and various
> non-Freescale non-NUMA hardware, for that matter).
> 
> Is there a specific issue that would need to be addressed to make it
> work on NUMA?

Nope. Just different code/file(s)... with NUMA specific details...

>>> As is, you're not getting rid of wrapper code -- only adding ifdefs.
>>
>> First, you're talking about the bootmem initialization wrapper code for
>> powerpc. The actual bootmem code is in include/linux/bootmem.h and
>> mm/bootmem.c. We can't remove those files as they are still used by
>> other arches. Also, the word wrapper is somewhat imprecise as in powerpc
>> land bootmem sort of runs on top of memblock
> 
> My point was just that the changelog says "This gets rid of wrapper
> code" when it actually removes no source code, and adds configuration
> complexity.

The patch gets rid of the wrapper code, bootmem itself and arch specific
initialization, from the build/kernel image. I guess I'll re-word that
so it doesn't sound so literal

>> When NO_BOOTMEM is configured the mm/nobootmem.c is used that is the
>> bootmem API actually re-implemented with memblock. The bootmem API is
>> used in various places in the arch independent code
>>
>> This patch wants to isolate for removal the bootmem initialization code
>> for powerpc and to exclude mm/bootmem.c from being built. This being the
>> first step I didn't want to actually remove the code, so it will be easy
>> to debug if some issues crop up. Also, people that want the use the
>> bootmem code for some reason can easily do that. Once this change spends
>> some time in the tree, we can actually remove the bootmem initialization
>> code
> 
> Is there a plausible reason someone would "want to use the bootmem
> code"?

Don't know, but as before it wasn't even possible to make a build with
NO_BOOTMEM I decided to err on the side of caution

> While the "ifdef it for a while" approach is sometimes sensible, usually
> it's better to just make the change rather than ifdef it.

I felt it was sensible in this case

> Consider what
> the code would look like if there were ifdefs for a ton of random
> changes, half of which nobody ever bothered to go back and clean up
> after the change got widespread testing.

Well, this is not a random change, but I certainly don't disagree with
the principle of what you're saying

> Why is this patch risky enough to warrant such an approach?

I don't think it's risky and we've been using it for months on various
SoC(s)/boards. Just didn't want to be very abrupt in removing the option
of using bootmem

> Shouldn't boot-time issues be pretty obvious?

Gross errors should be obvious. But the devil is in the details, and
even tough I've debugged/compared the memory layout/allocations with
bootmem and memblock only, I'm prepared to admit I might have missed
something (especially in the first patch of the sequence)


Cheers,