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

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

[Laurent Vivier]

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:
-- 
2.20.1

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

[Laurent Vivier]

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:
> 

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

[David Gibson]

[-- 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 --]

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

[Christophe Leroy]

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:
> 

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

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

[Christophe Leroy]

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
> 

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

[Laurent Vivier]

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.

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

[Christophe Leroy]

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