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

[Laurent Vivier]

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?

Thanks,
Laurent