From: Christophe Leroy <christophe.leroy@c-s.fr>
To: Laurent Vivier <lvivier@redhat.com>, linux-kernel@vger.kernel.org
Cc: Michael Ellerman <mpe@ellerman.id.au>,
linuxppc-dev@lists.ozlabs.org,
David Gibson <david@gibson.dropbear.id.au>
Subject: Re: [PATCH v2] powerpc/mm: move warning from resize_hpt_for_hotplug()
Date: Wed, 13 Mar 2019 09:28:39 +0100 [thread overview]
Message-ID: <6f089371-29e6-6fa9-379d-76d3a87e2b8b@c-s.fr> (raw)
In-Reply-To: <1dfe1a2e-f6f9-2d5d-a38e-ba4517794bad@redhat.com>
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
>
next prev parent reply other threads:[~2019-03-13 8:28 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
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
2019-03-13 8:28 ` Christophe Leroy [this message]
2019-03-13 8:50 ` Laurent Vivier
2019-03-13 9:10 ` Christophe Leroy
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6f089371-29e6-6fa9-379d-76d3a87e2b8b@c-s.fr \
--to=christophe.leroy@c-s.fr \
--cc=david@gibson.dropbear.id.au \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=lvivier@redhat.com \
--cc=mpe@ellerman.id.au \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).