Message-ID: <8a22d3a7-f574-19f1-46d0-a54a5343dec8@redhat.com>
Date:   Wed, 13 Mar 2019 09:50:46 +0100
From:   Laurent Vivier <lvivier@...hat.com>
To:     Christophe Leroy <christophe.leroy@....fr>,
        linux-kernel@...r.kernel.org
Cc:     Michael Ellerman <mpe@...erman.id.au>,
        linuxppc-dev@...ts.ozlabs.org,
        David Gibson <david@...son.dropbear.id.au>
Subject: Re: [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug()

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@...hat.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

doesn't output this error [1]. Why linux-ppc doesn't use the same script as in the kernel directory?

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; }

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.

Powered by blists - more mailing lists