| Message ID | 1399402084-6325-2-git-send-email-Emilian.Medve@Freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Benjamin Herrenschmidt |
| Headers | show |
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? As is, you're not getting rid of wrapper code -- only adding ifdefs. -Scott
On Tue, 2014-05-06 at 16:49 -0500, 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. Ignore this part -- I see it doesn't have an option string for it to show up to the user. -Scott
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 > 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 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 Cheers,
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? > > 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. > 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"? While the "ifdef it for a while" approach is sometimes sensible, usually it's better to just make the change rather than ifdef it. 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. Why is this patch risky enough to warrant such an approach? Shouldn't boot-time issues be pretty obvious? -Scott
Emil Medve <Emilian.Medve@Freescale.com> writes: > 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 There is actually one in mm/Kconfig So I guess you should make the platform that you are interested /tested just do select No_BOOTMEM like we do in arch/arm/Kconfig etc ? -aneesh
Hello Aneesh, On 05/07/2014 01:35 AM, Aneesh Kumar K.V wrote: > Emil Medve <Emilian.Medve@Freescale.com> writes: > >> 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 > > > There is actually one in mm/Kconfig > > So I guess you should make the platform that you are interested > /tested just do select No_BOOTMEM like we do in arch/arm/Kconfig etc ? I had a look at what was done for x86 and I missed why the mm/Kconfig symbol was added. Will include your suggestion in the next version Cheers,
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,
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 + config ARCH_MEMORY_PROBE def_bool y depends on MEMORY_HOTPLUG diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c index eaf5d1d8..d3e1d5f 100644 --- a/arch/powerpc/mm/mem.c +++ b/arch/powerpc/mm/mem.c @@ -187,10 +187,12 @@ EXPORT_SYMBOL_GPL(walk_system_ram_range); #ifndef CONFIG_NEED_MULTIPLE_NODES void __init do_init_bootmem(void) { +#ifndef CONFIG_NO_BOOTMEM unsigned long start, bootmap_pages; struct memblock_region *reg; int boot_mapsize; phys_addr_t _total_lowmem; +#endif phys_addr_t _lowmem_end_addr; #ifndef CONFIG_HIGHMEM @@ -203,6 +205,7 @@ void __init do_init_bootmem(void) max_low_pfn = _lowmem_end_addr >> PAGE_SHIFT; min_low_pfn = MEMORY_START >> PAGE_SHIFT; +#ifndef CONFIG_NO_BOOTMEM /* * Find an area to use for the bootmem bitmap. Calculate the size of * bitmap required as (Total Memory) / PAGE_SIZE / BITS_PER_BYTE. @@ -214,12 +217,14 @@ void __init do_init_bootmem(void) start = memblock_alloc(bootmap_pages << PAGE_SHIFT, PAGE_SIZE); boot_mapsize = init_bootmem_node(NODE_DATA(0), start >> PAGE_SHIFT, min_low_pfn, max_low_pfn); +#endif /* Place all memblock_regions in the same node and merge contiguous * memblock_regions */ memblock_set_node(0, (phys_addr_t)ULLONG_MAX, &memblock.memory, 0); +#ifndef CONFIG_NO_BOOTMEM /* Add all physical memory to the bootmem map, mark each area * present. */ @@ -234,11 +239,14 @@ void __init do_init_bootmem(void) reserve_bootmem(reg->base, trunc_size, BOOTMEM_DEFAULT); } } +#endif /* XXX need to clip this if using highmem? */ sparse_memory_present_with_active_regions(0); +#ifndef CONFIG_NO_BOOTMEM init_bootmem_done = 1; +#endif } /* mark pages that don't exist as nosave */
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(+)