Re: [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug()

[Christophe Leroy]

Le 13/03/2019 Ã 09:01, Laurent Vivier a ÃcritÂ:
> On 13/03/2019 07:03, Christophe Leroy wrote:
> > Le 08/03/2019 Ã 11:54, Laurent Vivier a ÃcritÂ:
> > > resize_hpt_for_hotplug() reports a warning when it cannot
> > > resize the hash page table ("Unable to resize hash page
> > > table to target order") but in some cases it's not a problem
> > > and can make user thinks something has not worked properly.
> > >
> > > This patch moves the warning to arch_remove_memory() to
> > > only report the problem when it is needed.
> > >
> > > Signed-off-by: Laurent Vivier <lvivier@xxxxxxxxxx>
> > > ---
> > >   arch/powerpc/include/asm/sparsemem.h | 4 ++--
> > >  Â arch/powerpc/mm/hash_utils_64.cÂÂÂÂÂÂ | 17 ++++++-----------
> > >  Â arch/powerpc/mm/mem.cÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ |Â 3 ++-
> > >  Â arch/powerpc/platforms/pseries/lpar.c |Â 3 ++-
> > >  Â 4 files changed, 12 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/arch/powerpc/include/asm/sparsemem.h
> > > b/arch/powerpc/include/asm/sparsemem.h
> > > index 68da49320592..3192d454a733 100644
> > > --- a/arch/powerpc/include/asm/sparsemem.h
> > > +++ b/arch/powerpc/include/asm/sparsemem.h
> > > @@ -17,9 +17,9 @@ extern int create_section_mapping(unsigned long
> > > start, unsigned long end, int ni
> > >  Â extern int remove_section_mapping(unsigned long start, unsigned long
> > > end);
> > >  Â Â #ifdef CONFIG_PPC_BOOK3S_64
> > > -extern void resize_hpt_for_hotplug(unsigned long new_mem_size);
> > > +extern int resize_hpt_for_hotplug(unsigned long new_mem_size);
> > >  Â #else
> > > -static inline void resize_hpt_for_hotplug(unsigned long new_mem_size)
> > > { }
> > > +static inline int resize_hpt_for_hotplug(unsigned long new_mem_size)
> > > { return 0; }
> > >  Â #endif
> > >  Â Â #ifdef CONFIG_NUMA
> > > diff --git a/arch/powerpc/mm/hash_utils_64.c
> > > b/arch/powerpc/mm/hash_utils_64.c
> > > index 0cc7fbc3bd1c..40bb2a8326bb 100644
> > > --- a/arch/powerpc/mm/hash_utils_64.c
> > > +++ b/arch/powerpc/mm/hash_utils_64.c
> > > @@ -755,12 +755,12 @@ static unsigned long __init
> > > htab_get_table_size(void)
> > >  Â }
> > >  Â Â #ifdef CONFIG_MEMORY_HOTPLUG
> > > -void resize_hpt_for_hotplug(unsigned long new_mem_size)
> > > +int resize_hpt_for_hotplug(unsigned long new_mem_size)
> > >  Â {
> > >  ÂÂÂÂÂ unsigned target_hpt_shift;
> > >  Â ÂÂÂÂÂ if (!mmu_hash_ops.resize_hpt)
> > > -ÂÂÂÂÂÂÂ return;
> > > +ÂÂÂÂÂÂÂ return 0;
> > >  Â ÂÂÂÂÂ target_hpt_shift = htab_shift_for_mem_size(new_mem_size);
> > >  Â @@ -773,15 +773,10 @@ void resize_hpt_for_hotplug(unsigned long
> > > new_mem_size)
> > >  ÂÂÂÂÂÂ * current shift
> > >  ÂÂÂÂÂÂ */
> > >  ÂÂÂÂÂ if ((target_hpt_shift > ppc64_pft_size)
> > > -ÂÂÂÂÂÂÂ || (target_hpt_shift < (ppc64_pft_size - 1))) {
> > > -ÂÂÂÂÂÂÂ int rc;
> > > -
> > > -ÂÂÂÂÂÂÂ rc = mmu_hash_ops.resize_hpt(target_hpt_shift);
> > > -ÂÂÂÂÂÂÂ if (rc && (rc != -ENODEV))
> > > -ÂÂÂÂÂÂÂÂÂÂÂ printk(KERN_WARNING
> > > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "Unable to resize hash page table to target order
> > > %d: %d\n",
> > > -ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ target_hpt_shift, rc);
> > > -ÂÂÂ }
> > > +ÂÂÂÂÂÂÂ || (target_hpt_shift < (ppc64_pft_size - 1)))
> >
> > The || should go on the line above and the two (target_hpt... should be
> > aligned, and the () after the < are superflous.
> >
> > And indeed, we should (in another patch) rename 'target_hpt_shift' with
> > a shorter name, this would avoid multiple lines.
> >
> > Ref
> > https://www.kernel.org/doc/html/latest/process/coding-style.html#naming :
> >
> > LOCAL variable names should be short, and to the point. If you have some
> > random integer loop counter, it should probably be called i. Calling it
> > loop_counter is non-productive, if there is no chance of it being
> > mis-understood. Similarly, tmp can be just about any type of variable
> > that is used to hold a temporary value.
> >
> > If you are afraid to mix up your local variable names, you have another
> > problem, which is called the function-growth-hormone-imbalance syndrome.
> > See chapter 6 (Functions).
>
> I'm only removing a warning. Do we really need to rewrite all the code
> around for that?

No, and that's the reason why I said it could be done in another 
(future) patch.

Anyway, your patch should be clean regarding checkpatch

See https://patchwork.ozlabs.org/patch/1052984/
And 
https://openpower.xyz/job/snowpatch/job/snowpatch-linux-checkpatch/3298//artifact/linux/checkpatch.log

CHECK:AVOID_EXTERNS: extern prototypes should be avoided in .h files
#31: FILE: arch/powerpc/include/asm/sparsemem.h:20:
+extern int resize_hpt_for_hotplug(unsigned long new_mem_size);

CHECK:LOGICAL_CONTINUATIONS: Logical continuations should be on the 
previous line
#70: FILE: arch/powerpc/mm/hash_utils_64.c:776:
 	if ((target_hpt_shift > ppc64_pft_size)
+	    || (target_hpt_shift < (ppc64_pft_size - 1)))

total: 0 errors, 0 warnings, 2 checks, 60 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or 
--fix-inplace.

the.patch has style problems, please review.

NOTE: Ignored message types: ARCH_INCLUDE_LINUX BIT_MACRO 
COMPARISON_TO_NULL DT_SPLIT_BINDING_PATCH EMAIL_SUBJECT 
FILE_PATH_CHANGES GLOBAL_INITIALISERS LINE_SPACING MULTIPLE_ASSIGNMENTS

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.

Christophe

> Thanks,
> Laurent