* [PATCH 1/1] mm/mm_init.c: eliminate a local variable in mem_debugging_and_hardening_init()
@ 2024-03-18 13:57 thunder.leizhen
2024-03-18 16:33 ` Mike Rapoport
0 siblings, 1 reply; 6+ messages in thread
From: thunder.leizhen @ 2024-03-18 13:57 UTC (permalink / raw)
To: Mike Rapoport, Andrew Morton, linux-mm, linux-kernel
From: Zhen Lei <thunder.leizhen@huawei.com>
The local variable 'page_poisoning_requested' is assigned true at only
one point. It can be eliminated by moving the code that depends on it
to the location where it is assigned true. This also make the moved
code to be compiled only if CONFIG_PAGE_POISONING is set.
Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
---
mm/mm_init.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/mm/mm_init.c b/mm/mm_init.c
index 549e76af8f82a8e..3eb217130bcb2b5 100644
--- a/mm/mm_init.c
+++ b/mm/mm_init.c
@@ -2614,7 +2614,6 @@ DEFINE_STATIC_KEY_MAYBE(CONFIG_DEBUG_VM, check_pages_enabled);
*/
static void __init mem_debugging_and_hardening_init(void)
{
- bool page_poisoning_requested = false;
bool want_check_pages = false;
#ifdef CONFIG_PAGE_POISONING
@@ -2626,18 +2625,16 @@ static void __init mem_debugging_and_hardening_init(void)
(!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
debug_pagealloc_enabled())) {
static_branch_enable(&_page_poisoning_enabled);
- page_poisoning_requested = true;
want_check_pages = true;
- }
-#endif
- if ((_init_on_alloc_enabled_early || _init_on_free_enabled_early) &&
- page_poisoning_requested) {
- pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, "
- "will take precedence over init_on_alloc and init_on_free\n");
- _init_on_alloc_enabled_early = false;
- _init_on_free_enabled_early = false;
+ if (_init_on_alloc_enabled_early || _init_on_free_enabled_early) {
+ pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, "
+ "will take precedence over init_on_alloc and init_on_free\n");
+ _init_on_alloc_enabled_early = false;
+ _init_on_free_enabled_early = false;
+ }
}
+#endif
if (_init_on_alloc_enabled_early) {
want_check_pages = true;
--
2.34.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] mm/mm_init.c: eliminate a local variable in mem_debugging_and_hardening_init()
2024-03-18 13:57 [PATCH 1/1] mm/mm_init.c: eliminate a local variable in mem_debugging_and_hardening_init() thunder.leizhen
@ 2024-03-18 16:33 ` Mike Rapoport
2024-03-19 1:22 ` Leizhen (ThunderTown)
2024-03-19 6:23 ` Anshuman Khandual
0 siblings, 2 replies; 6+ messages in thread
From: Mike Rapoport @ 2024-03-18 16:33 UTC (permalink / raw)
To: thunder.leizhen; +Cc: Andrew Morton, linux-mm, linux-kernel
On Mon, Mar 18, 2024 at 09:57:14PM +0800, thunder.leizhen@huaweicloud.com wrote:
> From: Zhen Lei <thunder.leizhen@huawei.com>
>
> The local variable 'page_poisoning_requested' is assigned true at only
> one point. It can be eliminated by moving the code that depends on it
> to the location where it is assigned true. This also make the moved
> code to be compiled only if CONFIG_PAGE_POISONING is set.
I don't see it as much of an improvement and code readability becomes worse
IMO.
> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> ---
> mm/mm_init.c | 17 +++++++----------
> 1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/mm/mm_init.c b/mm/mm_init.c
> index 549e76af8f82a8e..3eb217130bcb2b5 100644
> --- a/mm/mm_init.c
> +++ b/mm/mm_init.c
> @@ -2614,7 +2614,6 @@ DEFINE_STATIC_KEY_MAYBE(CONFIG_DEBUG_VM, check_pages_enabled);
> */
> static void __init mem_debugging_and_hardening_init(void)
> {
> - bool page_poisoning_requested = false;
> bool want_check_pages = false;
>
> #ifdef CONFIG_PAGE_POISONING
> @@ -2626,18 +2625,16 @@ static void __init mem_debugging_and_hardening_init(void)
> (!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
> debug_pagealloc_enabled())) {
> static_branch_enable(&_page_poisoning_enabled);
> - page_poisoning_requested = true;
> want_check_pages = true;
> - }
> -#endif
>
> - if ((_init_on_alloc_enabled_early || _init_on_free_enabled_early) &&
> - page_poisoning_requested) {
> - pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, "
> - "will take precedence over init_on_alloc and init_on_free\n");
> - _init_on_alloc_enabled_early = false;
> - _init_on_free_enabled_early = false;
> + if (_init_on_alloc_enabled_early || _init_on_free_enabled_early) {
> + pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, "
> + "will take precedence over init_on_alloc and init_on_free\n");
> + _init_on_alloc_enabled_early = false;
> + _init_on_free_enabled_early = false;
> + }
> }
> +#endif
>
> if (_init_on_alloc_enabled_early) {
> want_check_pages = true;
> --
> 2.34.1
>
--
Sincerely yours,
Mike.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] mm/mm_init.c: eliminate a local variable in mem_debugging_and_hardening_init()
2024-03-18 16:33 ` Mike Rapoport
@ 2024-03-19 1:22 ` Leizhen (ThunderTown)
2024-03-19 15:01 ` Mike Rapoport
2024-03-19 6:23 ` Anshuman Khandual
1 sibling, 1 reply; 6+ messages in thread
From: Leizhen (ThunderTown) @ 2024-03-19 1:22 UTC (permalink / raw)
To: Mike Rapoport; +Cc: Andrew Morton, linux-mm, linux-kernel, Zhen Lei
On 2024/3/19 0:33, Mike Rapoport wrote:
> On Mon, Mar 18, 2024 at 09:57:14PM +0800, thunder.leizhen@huaweicloud.com wrote:
>> From: Zhen Lei <thunder.leizhen@huawei.com>
>>
>> The local variable 'page_poisoning_requested' is assigned true at only
>> one point. It can be eliminated by moving the code that depends on it
>> to the location where it is assigned true. This also make the moved
>> code to be compiled only if CONFIG_PAGE_POISONING is set.
>
> I don't see it as much of an improvement and code readability becomes worse
> IMO.
Yes, the moved branch will be optimized by the compiler if CONFIG_PAGE_POISONING is
not set. But for a reader, he can simply skip over that moved branch when
CONFIG_PAGE_POISONING is not set.
>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>> mm/mm_init.c | 17 +++++++----------
>> 1 file changed, 7 insertions(+), 10 deletions(-)
>>
>> diff --git a/mm/mm_init.c b/mm/mm_init.c
>> index 549e76af8f82a8e..3eb217130bcb2b5 100644
>> --- a/mm/mm_init.c
>> +++ b/mm/mm_init.c
>> @@ -2614,7 +2614,6 @@ DEFINE_STATIC_KEY_MAYBE(CONFIG_DEBUG_VM, check_pages_enabled);
>> */
>> static void __init mem_debugging_and_hardening_init(void)
>> {
>> - bool page_poisoning_requested = false;
>> bool want_check_pages = false;
>>
>> #ifdef CONFIG_PAGE_POISONING
>> @@ -2626,18 +2625,16 @@ static void __init mem_debugging_and_hardening_init(void)
>> (!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
>> debug_pagealloc_enabled())) {
>> static_branch_enable(&_page_poisoning_enabled);
>> - page_poisoning_requested = true;
>> want_check_pages = true;
>> - }
>> -#endif
>>
>> - if ((_init_on_alloc_enabled_early || _init_on_free_enabled_early) &&
>> - page_poisoning_requested) {
>> - pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, "
>> - "will take precedence over init_on_alloc and init_on_free\n");
>> - _init_on_alloc_enabled_early = false;
>> - _init_on_free_enabled_early = false;
>> + if (_init_on_alloc_enabled_early || _init_on_free_enabled_early) {
>> + pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, "
>> + "will take precedence over init_on_alloc and init_on_free\n");
>> + _init_on_alloc_enabled_early = false;
>> + _init_on_free_enabled_early = false;
>> + }
>> }
>> +#endif
>>
>> if (_init_on_alloc_enabled_early) {
>> want_check_pages = true;
>> --
>> 2.34.1
>>
>
--
Regards,
Zhen Lei
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] mm/mm_init.c: eliminate a local variable in mem_debugging_and_hardening_init()
2024-03-18 16:33 ` Mike Rapoport
2024-03-19 1:22 ` Leizhen (ThunderTown)
@ 2024-03-19 6:23 ` Anshuman Khandual
1 sibling, 0 replies; 6+ messages in thread
From: Anshuman Khandual @ 2024-03-19 6:23 UTC (permalink / raw)
To: Mike Rapoport, thunder.leizhen; +Cc: Andrew Morton, linux-mm, linux-kernel
On 3/18/24 22:03, Mike Rapoport wrote:
> On Mon, Mar 18, 2024 at 09:57:14PM +0800, thunder.leizhen@huaweicloud.com wrote:
>> From: Zhen Lei <thunder.leizhen@huawei.com>
>>
>> The local variable 'page_poisoning_requested' is assigned true at only
>> one point. It can be eliminated by moving the code that depends on it
>> to the location where it is assigned true. This also make the moved
>> code to be compiled only if CONFIG_PAGE_POISONING is set.
>
> I don't see it as much of an improvement and code readability becomes worse
> IMO.
+1 agreed.
>
>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>> ---
>> mm/mm_init.c | 17 +++++++----------
>> 1 file changed, 7 insertions(+), 10 deletions(-)
>>
>> diff --git a/mm/mm_init.c b/mm/mm_init.c
>> index 549e76af8f82a8e..3eb217130bcb2b5 100644
>> --- a/mm/mm_init.c
>> +++ b/mm/mm_init.c
>> @@ -2614,7 +2614,6 @@ DEFINE_STATIC_KEY_MAYBE(CONFIG_DEBUG_VM, check_pages_enabled);
>> */
>> static void __init mem_debugging_and_hardening_init(void)
>> {
>> - bool page_poisoning_requested = false;
>> bool want_check_pages = false;
>>
>> #ifdef CONFIG_PAGE_POISONING
>> @@ -2626,18 +2625,16 @@ static void __init mem_debugging_and_hardening_init(void)
>> (!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
>> debug_pagealloc_enabled())) {
>> static_branch_enable(&_page_poisoning_enabled);
>> - page_poisoning_requested = true;
>> want_check_pages = true;
>> - }
>> -#endif
>>
>> - if ((_init_on_alloc_enabled_early || _init_on_free_enabled_early) &&
>> - page_poisoning_requested) {
>> - pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, "
>> - "will take precedence over init_on_alloc and init_on_free\n");
>> - _init_on_alloc_enabled_early = false;
>> - _init_on_free_enabled_early = false;
>> + if (_init_on_alloc_enabled_early || _init_on_free_enabled_early) {
>> + pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, "
>> + "will take precedence over init_on_alloc and init_on_free\n");
>> + _init_on_alloc_enabled_early = false;
>> + _init_on_free_enabled_early = false;
>> + }
>> }
>> +#endif
>>
>> if (_init_on_alloc_enabled_early) {
>> want_check_pages = true;
>> --
>> 2.34.1
>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] mm/mm_init.c: eliminate a local variable in mem_debugging_and_hardening_init()
2024-03-19 1:22 ` Leizhen (ThunderTown)
@ 2024-03-19 15:01 ` Mike Rapoport
2024-03-20 1:18 ` Leizhen (ThunderTown)
0 siblings, 1 reply; 6+ messages in thread
From: Mike Rapoport @ 2024-03-19 15:01 UTC (permalink / raw)
To: Leizhen (ThunderTown); +Cc: Andrew Morton, linux-mm, linux-kernel, Zhen Lei
On Tue, Mar 19, 2024 at 09:22:03AM +0800, Leizhen (ThunderTown) wrote:
>
>
> On 2024/3/19 0:33, Mike Rapoport wrote:
> > On Mon, Mar 18, 2024 at 09:57:14PM +0800, thunder.leizhen@huaweicloud.com wrote:
> >> From: Zhen Lei <thunder.leizhen@huawei.com>
> >>
> >> The local variable 'page_poisoning_requested' is assigned true at only
> >> one point. It can be eliminated by moving the code that depends on it
> >> to the location where it is assigned true. This also make the moved
> >> code to be compiled only if CONFIG_PAGE_POISONING is set.
> >
> > I don't see it as much of an improvement and code readability becomes worse
> > IMO.
>
> Yes, the moved branch will be optimized by the compiler if CONFIG_PAGE_POISONING is
> not set. But for a reader, he can simply skip over that moved branch when
> CONFIG_PAGE_POISONING is not set.
Saving one branch at init does not justify the churn and reduced
readability.
> >> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
> >> ---
> >> mm/mm_init.c | 17 +++++++----------
> >> 1 file changed, 7 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/mm/mm_init.c b/mm/mm_init.c
> >> index 549e76af8f82a8e..3eb217130bcb2b5 100644
> >> --- a/mm/mm_init.c
> >> +++ b/mm/mm_init.c
> >> @@ -2614,7 +2614,6 @@ DEFINE_STATIC_KEY_MAYBE(CONFIG_DEBUG_VM, check_pages_enabled);
> >> */
> >> static void __init mem_debugging_and_hardening_init(void)
> >> {
> >> - bool page_poisoning_requested = false;
> >> bool want_check_pages = false;
> >>
> >> #ifdef CONFIG_PAGE_POISONING
> >> @@ -2626,18 +2625,16 @@ static void __init mem_debugging_and_hardening_init(void)
> >> (!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
> >> debug_pagealloc_enabled())) {
> >> static_branch_enable(&_page_poisoning_enabled);
> >> - page_poisoning_requested = true;
> >> want_check_pages = true;
> >> - }
> >> -#endif
> >>
> >> - if ((_init_on_alloc_enabled_early || _init_on_free_enabled_early) &&
> >> - page_poisoning_requested) {
> >> - pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, "
> >> - "will take precedence over init_on_alloc and init_on_free\n");
> >> - _init_on_alloc_enabled_early = false;
> >> - _init_on_free_enabled_early = false;
> >> + if (_init_on_alloc_enabled_early || _init_on_free_enabled_early) {
> >> + pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, "
> >> + "will take precedence over init_on_alloc and init_on_free\n");
> >> + _init_on_alloc_enabled_early = false;
> >> + _init_on_free_enabled_early = false;
> >> + }
> >> }
> >> +#endif
> >>
> >> if (_init_on_alloc_enabled_early) {
> >> want_check_pages = true;
> >> --
> >> 2.34.1
> >>
> >
>
> --
> Regards,
> Zhen Lei
>
--
Sincerely yours,
Mike.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] mm/mm_init.c: eliminate a local variable in mem_debugging_and_hardening_init()
2024-03-19 15:01 ` Mike Rapoport
@ 2024-03-20 1:18 ` Leizhen (ThunderTown)
0 siblings, 0 replies; 6+ messages in thread
From: Leizhen (ThunderTown) @ 2024-03-20 1:18 UTC (permalink / raw)
To: Mike Rapoport; +Cc: Andrew Morton, linux-mm, linux-kernel, Zhen Lei
On 2024/3/19 23:01, Mike Rapoport wrote:
> On Tue, Mar 19, 2024 at 09:22:03AM +0800, Leizhen (ThunderTown) wrote:
>>
>>
>> On 2024/3/19 0:33, Mike Rapoport wrote:
>>> On Mon, Mar 18, 2024 at 09:57:14PM +0800, thunder.leizhen@huaweicloud.com wrote:
>>>> From: Zhen Lei <thunder.leizhen@huawei.com>
>>>>
>>>> The local variable 'page_poisoning_requested' is assigned true at only
>>>> one point. It can be eliminated by moving the code that depends on it
>>>> to the location where it is assigned true. This also make the moved
>>>> code to be compiled only if CONFIG_PAGE_POISONING is set.
>>>
>>> I don't see it as much of an improvement and code readability becomes worse
>>> IMO.
>>
>> Yes, the moved branch will be optimized by the compiler if CONFIG_PAGE_POISONING is
>> not set. But for a reader, he can simply skip over that moved branch when
>> CONFIG_PAGE_POISONING is not set.
>
> Saving one branch at init does not justify the churn and reduced
> readability.
OK, I got it.
>
>>>> Signed-off-by: Zhen Lei <thunder.leizhen@huawei.com>
>>>> ---
>>>> mm/mm_init.c | 17 +++++++----------
>>>> 1 file changed, 7 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/mm/mm_init.c b/mm/mm_init.c
>>>> index 549e76af8f82a8e..3eb217130bcb2b5 100644
>>>> --- a/mm/mm_init.c
>>>> +++ b/mm/mm_init.c
>>>> @@ -2614,7 +2614,6 @@ DEFINE_STATIC_KEY_MAYBE(CONFIG_DEBUG_VM, check_pages_enabled);
>>>> */
>>>> static void __init mem_debugging_and_hardening_init(void)
>>>> {
>>>> - bool page_poisoning_requested = false;
>>>> bool want_check_pages = false;
>>>>
>>>> #ifdef CONFIG_PAGE_POISONING
>>>> @@ -2626,18 +2625,16 @@ static void __init mem_debugging_and_hardening_init(void)
>>>> (!IS_ENABLED(CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC) &&
>>>> debug_pagealloc_enabled())) {
>>>> static_branch_enable(&_page_poisoning_enabled);
>>>> - page_poisoning_requested = true;
>>>> want_check_pages = true;
>>>> - }
>>>> -#endif
>>>>
>>>> - if ((_init_on_alloc_enabled_early || _init_on_free_enabled_early) &&
>>>> - page_poisoning_requested) {
>>>> - pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, "
>>>> - "will take precedence over init_on_alloc and init_on_free\n");
>>>> - _init_on_alloc_enabled_early = false;
>>>> - _init_on_free_enabled_early = false;
>>>> + if (_init_on_alloc_enabled_early || _init_on_free_enabled_early) {
>>>> + pr_info("mem auto-init: CONFIG_PAGE_POISONING is on, "
>>>> + "will take precedence over init_on_alloc and init_on_free\n");
>>>> + _init_on_alloc_enabled_early = false;
>>>> + _init_on_free_enabled_early = false;
>>>> + }
>>>> }
>>>> +#endif
>>>>
>>>> if (_init_on_alloc_enabled_early) {
>>>> want_check_pages = true;
>>>> --
>>>> 2.34.1
>>>>
>>>
>>
>> --
>> Regards,
>> Zhen Lei
>>
>
--
Regards,
Zhen Lei
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-03-20 1:18 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-18 13:57 [PATCH 1/1] mm/mm_init.c: eliminate a local variable in mem_debugging_and_hardening_init() thunder.leizhen
2024-03-18 16:33 ` Mike Rapoport
2024-03-19 1:22 ` Leizhen (ThunderTown)
2024-03-19 15:01 ` Mike Rapoport
2024-03-20 1:18 ` Leizhen (ThunderTown)
2024-03-19 6:23 ` Anshuman Khandual
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.