* Re: [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug()
2019-03-08 10:54 [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug() Laurent Vivier
@ 2019-03-08 10:56 ` Laurent Vivier
2019-03-13 4:27 ` David Gibson
2019-03-13 6:03 ` Christophe Leroy
2 siblings, 0 replies; 8+ messages in thread
From: Laurent Vivier @ 2019-03-08 10:56 UTC (permalink / raw)
To: linux-kernel
Cc: Michael Ellerman, linuxppc-dev, David Gibson, Christophe Leroy
I forgot the version change note:
v2: add warning messages for H_PARAMETER and H_RESOURCE
Thanks,
Laurent
On 08/03/2019 11:54, Laurent Vivier wrote:
> 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)))
> + return mmu_hash_ops.resize_hpt(target_hpt_shift);
> +
> + return 0;
> }
>
> int hash__create_section_mapping(unsigned long start, unsigned long end, int nid)
> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> index 33cc6f676fa6..0d40d970cf4a 100644
> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -169,7 +169,8 @@ int __meminit arch_remove_memory(int nid, u64 start, u64 size,
> */
> vm_unmap_aliases();
>
> - resize_hpt_for_hotplug(memblock_phys_mem_size());
> + if (resize_hpt_for_hotplug(memblock_phys_mem_size()) == -ENOSPC)
> + pr_warn("Hash collision while resizing HPT\n");
>
> return ret;
> }
> diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
> index f2a9f0adc2d3..1034ef1fe2b4 100644
> --- a/arch/powerpc/platforms/pseries/lpar.c
> +++ b/arch/powerpc/platforms/pseries/lpar.c
> @@ -901,8 +901,10 @@ static int pseries_lpar_resize_hpt(unsigned long shift)
> break;
>
> case H_PARAMETER:
> + pr_warn("Invalid argument from H_RESIZE_HPT_PREPARE\n");
> return -EINVAL;
> case H_RESOURCE:
> + pr_warn("Operation not permitted from H_RESIZE_HPT_PREPARE\n");
> return -EPERM;
> default:
> pr_warn("Unexpected error %d from H_RESIZE_HPT_PREPARE\n", rc);
> @@ -918,7 +920,6 @@ static int pseries_lpar_resize_hpt(unsigned long shift)
> if (rc != 0) {
> switch (state.commit_rc) {
> case H_PTEG_FULL:
> - pr_warn("Hash collision while resizing HPT\n");
> return -ENOSPC;
>
> default:
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug()
2019-03-08 10:54 [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug() Laurent Vivier
2019-03-08 10:56 ` Laurent Vivier
@ 2019-03-13 4:27 ` David Gibson
2019-03-13 6:03 ` Christophe Leroy
2 siblings, 0 replies; 8+ messages in thread
From: David Gibson @ 2019-03-13 4:27 UTC (permalink / raw)
To: Laurent Vivier
Cc: linux-kernel, Michael Ellerman, linuxppc-dev, Christophe Leroy
[-- Attachment #1: Type: text/plain, Size: 4526 bytes --]
On Fri, Mar 08, 2019 at 11:54:13AM +0100, Laurent Vivier wrote:
> 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>
Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> 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)))
> + return mmu_hash_ops.resize_hpt(target_hpt_shift);
> +
> + return 0;
> }
>
> int hash__create_section_mapping(unsigned long start, unsigned long end, int nid)
> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> index 33cc6f676fa6..0d40d970cf4a 100644
> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -169,7 +169,8 @@ int __meminit arch_remove_memory(int nid, u64 start, u64 size,
> */
> vm_unmap_aliases();
>
> - resize_hpt_for_hotplug(memblock_phys_mem_size());
> + if (resize_hpt_for_hotplug(memblock_phys_mem_size()) == -ENOSPC)
> + pr_warn("Hash collision while resizing HPT\n");
>
> return ret;
> }
> diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
> index f2a9f0adc2d3..1034ef1fe2b4 100644
> --- a/arch/powerpc/platforms/pseries/lpar.c
> +++ b/arch/powerpc/platforms/pseries/lpar.c
> @@ -901,8 +901,10 @@ static int pseries_lpar_resize_hpt(unsigned long shift)
> break;
>
> case H_PARAMETER:
> + pr_warn("Invalid argument from H_RESIZE_HPT_PREPARE\n");
> return -EINVAL;
> case H_RESOURCE:
> + pr_warn("Operation not permitted from H_RESIZE_HPT_PREPARE\n");
> return -EPERM;
> default:
> pr_warn("Unexpected error %d from H_RESIZE_HPT_PREPARE\n", rc);
> @@ -918,7 +920,6 @@ static int pseries_lpar_resize_hpt(unsigned long shift)
> if (rc != 0) {
> switch (state.commit_rc) {
> case H_PTEG_FULL:
> - pr_warn("Hash collision while resizing HPT\n");
> return -ENOSPC;
>
> default:
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug()
2019-03-08 10:54 [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug() Laurent Vivier
2019-03-08 10:56 ` Laurent Vivier
2019-03-13 4:27 ` David Gibson
@ 2019-03-13 6:03 ` Christophe Leroy
2019-03-13 8:01 ` Laurent Vivier
2 siblings, 1 reply; 8+ messages in thread
From: Christophe Leroy @ 2019-03-13 6:03 UTC (permalink / raw)
To: Laurent Vivier, linux-kernel; +Cc: Michael Ellerman, linuxppc-dev, David Gibson
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).
Christophe
> + return mmu_hash_ops.resize_hpt(target_hpt_shift);
> +
> + return 0;
> }
>
> int hash__create_section_mapping(unsigned long start, unsigned long end, int nid)
> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> index 33cc6f676fa6..0d40d970cf4a 100644
> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -169,7 +169,8 @@ int __meminit arch_remove_memory(int nid, u64 start, u64 size,
> */
> vm_unmap_aliases();
>
> - resize_hpt_for_hotplug(memblock_phys_mem_size());
> + if (resize_hpt_for_hotplug(memblock_phys_mem_size()) == -ENOSPC)
> + pr_warn("Hash collision while resizing HPT\n");
>
> return ret;
> }
> diff --git a/arch/powerpc/platforms/pseries/lpar.c b/arch/powerpc/platforms/pseries/lpar.c
> index f2a9f0adc2d3..1034ef1fe2b4 100644
> --- a/arch/powerpc/platforms/pseries/lpar.c
> +++ b/arch/powerpc/platforms/pseries/lpar.c
> @@ -901,8 +901,10 @@ static int pseries_lpar_resize_hpt(unsigned long shift)
> break;
>
> case H_PARAMETER:
> + pr_warn("Invalid argument from H_RESIZE_HPT_PREPARE\n");
> return -EINVAL;
> case H_RESOURCE:
> + pr_warn("Operation not permitted from H_RESIZE_HPT_PREPARE\n");
> return -EPERM;
> default:
> pr_warn("Unexpected error %d from H_RESIZE_HPT_PREPARE\n", rc);
> @@ -918,7 +920,6 @@ static int pseries_lpar_resize_hpt(unsigned long shift)
> if (rc != 0) {
> switch (state.commit_rc) {
> case H_PTEG_FULL:
> - pr_warn("Hash collision while resizing HPT\n");
> return -ENOSPC;
>
> default:
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug()
2019-03-13 6:03 ` Christophe Leroy
@ 2019-03-13 8:01 ` Laurent Vivier
2019-03-13 8:28 ` Christophe Leroy
0 siblings, 1 reply; 8+ messages in thread
From: Laurent Vivier @ 2019-03-13 8:01 UTC (permalink / raw)
To: Christophe Leroy, linux-kernel
Cc: Michael Ellerman, linuxppc-dev, David Gibson
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?
Thanks,
Laurent
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug()
2019-03-13 8:01 ` Laurent Vivier
@ 2019-03-13 8:28 ` Christophe Leroy
2019-03-13 8:50 ` Laurent Vivier
0 siblings, 1 reply; 8+ messages in thread
From: Christophe Leroy @ 2019-03-13 8:28 UTC (permalink / raw)
To: Laurent Vivier, linux-kernel; +Cc: Michael Ellerman, linuxppc-dev, David Gibson
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)))
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug()
2019-03-13 8:28 ` Christophe Leroy
@ 2019-03-13 8:50 ` Laurent Vivier
2019-03-13 9:10 ` Christophe Leroy
0 siblings, 1 reply; 8+ messages in thread
From: Laurent Vivier @ 2019-03-13 8:50 UTC (permalink / raw)
To: Christophe Leroy, linux-kernel
Cc: Michael Ellerman, linuxppc-dev, David Gibson
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
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug()
2019-03-13 8:50 ` Laurent Vivier
@ 2019-03-13 9:10 ` Christophe Leroy
0 siblings, 0 replies; 8+ messages in thread
From: Christophe Leroy @ 2019-03-13 9:10 UTC (permalink / raw)
To: Laurent Vivier, linux-kernel; +Cc: Michael Ellerman, linuxppc-dev, David Gibson
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
^ permalink raw reply [flat|nested] 8+ messages in thread