Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug()
To:     Laurent Vivier <lvivier@redhat.com>, linux-kernel@vger.kernel.org
Cc:     Michael Ellerman <mpe@ellerman.id.au>,
        linuxppc-dev@lists.ozlabs.org,
        David Gibson <david@gibson.dropbear.id.au>
References: <20190308105413.4302-1-lvivier@redhat.com>
 <3db6b64d-02d3-8aa7-80ca-3e469ea743ff@c-s.fr>
 <1dfe1a2e-f6f9-2d5d-a38e-ba4517794bad@redhat.com>
 <6f089371-29e6-6fa9-379d-76d3a87e2b8b@c-s.fr>
 <8a22d3a7-f574-19f1-46d0-a54a5343dec8@redhat.com>
From:   Christophe Leroy <christophe.leroy@c-s.fr>
Message-ID: <86c23a4a-2545-2025-cd61-51f77f1c4393@c-s.fr>
Date:   Wed, 13 Mar 2019 10:10:37 +0100
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.5.3
MIME-Version: 1.0
In-Reply-To: <8a22d3a7-f574-19f1-46d0-a54a5343dec8@redhat.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: fr
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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