Messages in this thread | | | Subject | Re: [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug() | From | Christophe Leroy <> | Date | Wed, 13 Mar 2019 09:28:39 +0100 |
| |
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 >
| |