Re: + mm-remove-find_max_pfn_with_active_regions.patch added to -mm tree

Mel Gorman
2008-07-28T11:38:37Z

On (28/07/08 04:15), Yinghai Lu didst pronounce:
> On Mon, Jul 28, 2008 at 2:16 AM, Mel Gorman <mel@csn.ul.ie> wrote:
> > On (27/07/08 20:13), akpm@linux-foundation.org didst pronounce:
> >>
> >> The patch titled
> >> mm: remove find_max_pfn_with_active_regions
> >> has been added to the -mm tree. Its filename is
> >> mm-remove-find_max_pfn_with_active_regions.patch
> >>
> >> Before you just go and hit "reply", please:
> >> a) Consider who else should be cc'ed
> >> b) Prefer to cc a suitable mailing list as well
> >> c) Ideally: find the original patch on the mailing list and do a
> >> reply-to-all to that, adding suitable additional cc's
> >>
> >> *** Remember to use Documentation/SubmitChecklist when testing your code ***
> >>
> >> See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find
> >> out what to do about this
> >>
> >> The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/
> >>
> >> ------------------------------------------------------
> >> Subject: mm: remove find_max_pfn_with_active_regions
> >> From: Yinghai Lu <yhlu.kernel@gmail.com>
> >>
> >> It has no user now
> >>
> >> Also print out info about adding/removing active regions.
> >>
> >> Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com>
> >> Cc: Mel Gorman <mel@csn.ul.ie>
> >> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> >> ---
> >>
> >> include/linux/mm.h | 1 -
> >> mm/page_alloc.c | 22 ++--------------------
> >> 2 files changed, 2 insertions(+), 21 deletions(-)
> >>
> >> diff -puN include/linux/mm.h~mm-remove-find_max_pfn_with_active_regions include/linux/mm.h
> >> --- a/include/linux/mm.h~mm-remove-find_max_pfn_with_active_regions
> >> +++ a/include/linux/mm.h
> >> @@ -1041,7 +1041,6 @@ extern unsigned long absent_pages_in_ran
> >> extern void get_pfn_range_for_nid(unsigned int nid,
> >> unsigned long *start_pfn, unsigned long *end_pfn);
> >> extern unsigned long find_min_pfn_with_active_regions(void);
> >> -extern unsigned long find_max_pfn_with_active_regions(void);
> >> extern void free_bootmem_with_active_regions(int nid,
> >> unsigned long max_low_pfn);
> >> typedef int (*work_fn_t)(unsigned long, unsigned long, void *);
> >> diff -puN mm/page_alloc.c~mm-remove-find_max_pfn_with_active_regions mm/page_alloc.c
> >> --- a/mm/page_alloc.c~mm-remove-find_max_pfn_with_active_regions
> >> +++ a/mm/page_alloc.c
> >> @@ -3572,8 +3572,7 @@ void __init add_active_range(unsigned in
> >> {
> >> int i;
> >>
> >> - mminit_dprintk(MMINIT_TRACE, "memory_register",
> >> - "Entering add_active_range(%d, %#lx, %#lx) "
> >> + printk(KERN_INFO "Adding active range (%d, %#lx, %#lx) "
> >> "%d entries of %d used\n",
> >> nid, start_pfn, end_pfn,
> >> nr_nodemap_entries, MAX_ACTIVE_REGIONS);
> >
> > Why are the mminit_dprintk() calls being converted to printk(KERN_INFO)? On
> > some machines, this will be very noisy. For example, some POWER configurations
> > will print out one line for every 16MB of memory with this patch.
>
> I don't know, on x86 esp the first node, that is some informative.
> or change that back to printk(KERN_DEBUG) ?
>
> hope the user put debug on command_line to get enough info.
>
> otherwise without "mminit_loglevel=" will get that debug info.
>

It's the type of information that is only useful when debugging memory
initialisation problems. The more friendly information can be found at
the lines starting with

early_node_map[1] active PFN ranges

and this is already logged. The fact that mminit_loglevel needs loglevel
needs to be at KERN_DEBUG level is already documented for the mminit_loglevel=
parameter. I still am not convinced that these needs to be logged at
KERN_INFO level.

> >
> >> @@ -3635,7 +3634,7 @@ void __init remove_active_range(unsigned
> >> int i, j;
> >> int removed = 0;
> >>
> >> - printk(KERN_DEBUG "remove_active_range (%d, %lu, %lu)\n",
> >> + printk(KERN_INFO "Removing active range (%d, %#lx, %#lx)\n",
> >> nid, start_pfn, end_pfn);
> >>
> >
> > This call is a lot rarer but I still don't see why it is being moved to
> > KERN_INFO. If anything, that should have been another mminit_printk() call
> > and one I obviously missed.
> use by 32bit x86 only.
>
> YH