Message ID | 20190308105413.4302-1-lvivier@redhat.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
Series | [v2] powerpc/mm: move warning from resize_hpt_for_hotplug() | expand |
Context | Check | Description |
---|---|---|
snowpatch_ozlabs/apply_patch | success | Successfully applied on branch next (9580b71b5a7863c24a9bd18bcd2ad759b86b1eff) |
snowpatch_ozlabs/build-ppc64le | success | build succeeded & removed 0 sparse warning(s) |
snowpatch_ozlabs/build-ppc64be | success | build succeeded & removed 0 sparse warning(s) |
snowpatch_ozlabs/build-ppc64e | success | build succeeded & removed 0 sparse warning(s) |
snowpatch_ozlabs/build-pmac32 | success | build succeeded & removed 0 sparse warning(s) |
snowpatch_ozlabs/checkpatch | warning | total: 0 errors, 0 warnings, 2 checks, 70 lines checked |
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: >
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:
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: >
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
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 >
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.
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
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:
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(-)