Re: [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug()
From: Christophe Leroy
Date: Wed Mar 13 2019 - 05:10:42 EST
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@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?
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