Messages in this thread | | | Subject | Re: [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug() | From | Christophe Leroy <> | Date | Wed, 13 Mar 2019 10:10:37 +0100 |
| |
Le 13/03/2019 à 09:50, Laurent Vivier a écrit : > On 13/03/2019 09:28, Christophe Leroy wrote: >> >> >> 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@redhat.com> >>>>> --- >>>>> 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))) >> > > It's really strange, from linux directory: > > ./scripts/checkpatch.pl 0001-powerpc-mm-move-warning-from-resize_hpt_for_hotplug.patch
Try with --strict
> > doesn't output this error [1]. Why linux-ppc doesn't use the same script as in the kernel directory?
linux-ppc used it but with dedicated options, see arch/powerpc/tools/checkpatch.sh
> > Anyway, I send a v3. > > Thanks, > Laurent > > [1] only: > > WARNING: line over 80 characters > #34: FILE: arch/powerpc/include/asm/sparsemem.h:22: > +static inline int resize_hpt_for_hotplug(unsigned long new_mem_size) { return 0; }
linux-ppc allows 90 characters.
> > total: 0 errors, 1 warnings, 70 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. > > 0001-powerpc-mm-move-warning-from-resize_hpt_for_hotplug.patch has style problems, please review. > > NOTE: If any of the errors are false positives, please report > them to the maintainer, see CHECKPATCH in MAINTAINERS. > > > But I think it's cleaner to keep the over 80 characters line for the inline. >
Christophe
| |