[PATCH] mm/slub: Refactor note_cmpxchg_failure for better readability

[PATCH] mm/slub: Refactor note_cmpxchg_failure for better readability

[Ye Liu]

From: Ye Liu <liuye@kylinos.cn>

Use IS_ENABLED() and standard if-else to make the code clearer.

Signed-off-by: Ye Liu <liuye@kylinos.cn>
---
 mm/slub.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 30003763d224..34853c0441a6 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3064,18 +3064,16 @@ static inline void note_cmpxchg_failure(const char *n,
 
 	pr_info("%s %s: cmpxchg redo ", n, s->name);
 
-#ifdef CONFIG_PREEMPTION
-	if (tid_to_cpu(tid) != tid_to_cpu(actual_tid))
+	if (IS_ENABLED(CONFIG_PREEMPTION) && tid_to_cpu(tid) != tid_to_cpu(actual_tid)) {
 		pr_warn("due to cpu change %d -> %d\n",
 			tid_to_cpu(tid), tid_to_cpu(actual_tid));
-	else
-#endif
-	if (tid_to_event(tid) != tid_to_event(actual_tid))
+	} else if (tid_to_event(tid) != tid_to_event(actual_tid)) {
 		pr_warn("due to cpu running other code. Event %ld->%ld\n",
 			tid_to_event(tid), tid_to_event(actual_tid));
-	else
+	} else {
 		pr_warn("for unknown reason: actual=%lx was=%lx target=%lx\n",
 			actual_tid, tid, next_tid(tid));
+	}
 #endif
 	stat(s, CMPXCHG_DOUBLE_CPU_FAIL);
 }
-- 
2.43.0

Re: [PATCH] mm/slub: Refactor note_cmpxchg_failure for better readability

[Harry Yoo]

On Mon, Sep 08, 2025 at 03:19:21PM +0800, Ye Liu wrote:
> From: Ye Liu <liuye@kylinos.cn>
> 
> Use IS_ENABLED() and standard if-else to make the code clearer.
> 
> Signed-off-by: Ye Liu <liuye@kylinos.cn>
> ---

Looks good to me,
Reviewed-by: Harry Yoo <harry.yoo@oracle.com>

with a nit,

>  mm/slub.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/slub.c b/mm/slub.c
> index 30003763d224..34853c0441a6 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -3064,18 +3064,16 @@ static inline void note_cmpxchg_failure(const char *n,
>  
>  	pr_info("%s %s: cmpxchg redo ", n, s->name);
>  
> -#ifdef CONFIG_PREEMPTION
> -	if (tid_to_cpu(tid) != tid_to_cpu(actual_tid))
> +	if (IS_ENABLED(CONFIG_PREEMPTION) && tid_to_cpu(tid) != tid_to_cpu(actual_tid)) {

nit: insert newline after && as it's more than 80 columns?

>  		pr_warn("due to cpu change %d -> %d\n",
>  			tid_to_cpu(tid), tid_to_cpu(actual_tid));
> -	else
> -#endif
> -	if (tid_to_event(tid) != tid_to_event(actual_tid))
> +	} else if (tid_to_event(tid) != tid_to_event(actual_tid)) {
>  		pr_warn("due to cpu running other code. Event %ld->%ld\n",
>  			tid_to_event(tid), tid_to_event(actual_tid));
> -	else
> +	} else {
>  		pr_warn("for unknown reason: actual=%lx was=%lx target=%lx\n",
>  			actual_tid, tid, next_tid(tid));
> +	}
>  #endif
>  	stat(s, CMPXCHG_DOUBLE_CPU_FAIL);
>  }
> -- 
> 2.43.0

Re: [PATCH] mm/slub: Refactor note_cmpxchg_failure for better readability

[Ye Liu]

在 2025/9/8 16:39, Harry Yoo 写道:
> On Mon, Sep 08, 2025 at 03:19:21PM +0800, Ye Liu wrote:
>> From: Ye Liu <liuye@kylinos.cn>
>>
>> Use IS_ENABLED() and standard if-else to make the code clearer.
>>
>> Signed-off-by: Ye Liu <liuye@kylinos.cn>
>> ---
> 
> Looks good to me,
> Reviewed-by: Harry Yoo <harry.yoo@oracle.com>
> 
> with a nit,
> 
>>  mm/slub.c | 10 ++++------
>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 30003763d224..34853c0441a6 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -3064,18 +3064,16 @@ static inline void note_cmpxchg_failure(const char *n,
>>  
>>  	pr_info("%s %s: cmpxchg redo ", n, s->name);
>>  
>> -#ifdef CONFIG_PREEMPTION
>> -	if (tid_to_cpu(tid) != tid_to_cpu(actual_tid))
>> +	if (IS_ENABLED(CONFIG_PREEMPTION) && tid_to_cpu(tid) != tid_to_cpu(actual_tid)) {
> 
> nit: insert newline after && as it's more than 80 columns?

I noticed this too, but using the script ./scripts/checkpatch.pl , there were no warnings.

$ ./scripts/checkpatch.pl 0001-mm-slub-Refactor-note_cmpxchg_failure-for-better-rea.patch 
total: 0 errors, 0 warnings, 22 lines checked

0001-mm-slub-Refactor-note_cmpxchg_failure-for-better-rea.patch has no obvious style problems and is ready for submission.


> 
>>  		pr_warn("due to cpu change %d -> %d\n",
>>  			tid_to_cpu(tid), tid_to_cpu(actual_tid));
>> -	else
>> -#endif
>> -	if (tid_to_event(tid) != tid_to_event(actual_tid))
>> +	} else if (tid_to_event(tid) != tid_to_event(actual_tid)) {
>>  		pr_warn("due to cpu running other code. Event %ld->%ld\n",
>>  			tid_to_event(tid), tid_to_event(actual_tid));
>> -	else
>> +	} else {
>>  		pr_warn("for unknown reason: actual=%lx was=%lx target=%lx\n",
>>  			actual_tid, tid, next_tid(tid));
>> +	}
>>  #endif
>>  	stat(s, CMPXCHG_DOUBLE_CPU_FAIL);
>>  }
>> -- 
>> 2.43.0

-- 
Thanks,
Ye Liu

Re: [PATCH] mm/slub: Refactor note_cmpxchg_failure for better readability

[Harry Yoo]

On Mon, Sep 08, 2025 at 04:56:40PM +0800, Ye Liu wrote:
> 
> 
> 在 2025/9/8 16:39, Harry Yoo 写道:
> > On Mon, Sep 08, 2025 at 03:19:21PM +0800, Ye Liu wrote:
> >> From: Ye Liu <liuye@kylinos.cn>
> >>
> >> Use IS_ENABLED() and standard if-else to make the code clearer.
> >>
> >> Signed-off-by: Ye Liu <liuye@kylinos.cn>
> >> ---
> > 
> > Looks good to me,
> > Reviewed-by: Harry Yoo <harry.yoo@oracle.com>
> > 
> > with a nit,
> > 
> >>  mm/slub.c | 10 ++++------
> >>  1 file changed, 4 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/mm/slub.c b/mm/slub.c
> >> index 30003763d224..34853c0441a6 100644
> >> --- a/mm/slub.c
> >> +++ b/mm/slub.c
> >> @@ -3064,18 +3064,16 @@ static inline void note_cmpxchg_failure(const char *n,
> >>  
> >>  	pr_info("%s %s: cmpxchg redo ", n, s->name);
> >>  
> >> -#ifdef CONFIG_PREEMPTION
> >> -	if (tid_to_cpu(tid) != tid_to_cpu(actual_tid))
> >> +	if (IS_ENABLED(CONFIG_PREEMPTION) && tid_to_cpu(tid) != tid_to_cpu(actual_tid)) {
> > 
> > nit: insert newline after && as it's more than 80 columns?
> 
> I noticed this too, but using the script ./scripts/checkpatch.pl , there were no warnings.
> 
> $ ./scripts/checkpatch.pl 0001-mm-slub-Refactor-note_cmpxchg_failure-for-better-rea.patch 
> total: 0 errors, 0 warnings, 22 lines checked
> 
> 0001-mm-slub-Refactor-note_cmpxchg_failure-for-better-rea.patch has no obvious style problems and is ready for submission.

IIUC the default limit is increased to 100 characters in checkpatch
but 80 characters is still preferred.

Quoting Documentation/process/coding-style.rst:

2) Breaking long lines and strings
----------------------------------

Coding style is all about readability and maintainability using commonlu
available tools.

The preferred limit on the length of a single line is 80 columns.

Statements longer than 80 columns should be broken into sensible chunks,
unless exceeding 80 columns significantly increases readability and does
not hide information.

Descendants are always substantially shorter than the parent and                
are placed substantially to the right.  A very commonly used style              
is to align descendants to a function open parenthesis.                         

These same rules are applied to function headers with a long argument list.

However, never break user-visible strings such as printk messages because
that breaks the ability to grep for them. 

And also quoting a commit message:

commit bdc48fa11e46f867ea4d75fa59ee87a7f48be144
Author: Joe Perches <joe@perches.com>
Date:   Fri May 29 16:12:21 2020 -0700

    checkpatch/coding-style: deprecate 80-column warning

    Yes, staying withing 80 columns is certainly still _preferred_.  But
    it's not the hard limit that the checkpatch warnings imply, and other
    concerns can most certainly dominate.

    Increase the default limit to 100 characters.  Not because 100
    characters is some hard limit either, but that's certainly a "what are
    you doing" kind of value and less likely to be about the occasional
    slightly longer lines.

    Miscellanea:

     - to avoid unnecessary whitespace changes in files, checkpatch will no
       longer emit a warning about line length when scanning files unless
       --strict is also used

     - Add a bit to coding-style about alignment to open parenthesis

    Signed-off-by: Joe Perches <joe@perches.com>
    Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

> >>  		pr_warn("due to cpu change %d -> %d\n",
> >>  			tid_to_cpu(tid), tid_to_cpu(actual_tid));
> >> -	else
> >> -#endif
> >> -	if (tid_to_event(tid) != tid_to_event(actual_tid))
> >> +	} else if (tid_to_event(tid) != tid_to_event(actual_tid)) {
> >>  		pr_warn("due to cpu running other code. Event %ld->%ld\n",
> >>  			tid_to_event(tid), tid_to_event(actual_tid));
> >> -	else
> >> +	} else {
> >>  		pr_warn("for unknown reason: actual=%lx was=%lx target=%lx\n",
> >>  			actual_tid, tid, next_tid(tid));
> >> +	}
> >>  #endif
> >>  	stat(s, CMPXCHG_DOUBLE_CPU_FAIL);
> >>  }
> >> -- 
> >> 2.43.0
> 
> -- 
> Thanks,
> Ye Liu
> 
> 

-- 
Cheers,
Harry / Hyeonggon

Re: [PATCH] mm/slub: Refactor note_cmpxchg_failure for better readability

[Ye Liu]

在 2025/9/8 17:21, Harry Yoo 写道:
> On Mon, Sep 08, 2025 at 04:56:40PM +0800, Ye Liu wrote:
>>
>>
>> 在 2025/9/8 16:39, Harry Yoo 写道:
>>> On Mon, Sep 08, 2025 at 03:19:21PM +0800, Ye Liu wrote:
>>>> From: Ye Liu <liuye@kylinos.cn>
>>>>
>>>> Use IS_ENABLED() and standard if-else to make the code clearer.
>>>>
>>>> Signed-off-by: Ye Liu <liuye@kylinos.cn>
>>>> ---
>>>
>>> Looks good to me,
>>> Reviewed-by: Harry Yoo <harry.yoo@oracle.com>
>>>
>>> with a nit,
>>>
>>>>  mm/slub.c | 10 ++++------
>>>>  1 file changed, 4 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/mm/slub.c b/mm/slub.c
>>>> index 30003763d224..34853c0441a6 100644
>>>> --- a/mm/slub.c
>>>> +++ b/mm/slub.c
>>>> @@ -3064,18 +3064,16 @@ static inline void note_cmpxchg_failure(const char *n,
>>>>  
>>>>  	pr_info("%s %s: cmpxchg redo ", n, s->name);
>>>>  
>>>> -#ifdef CONFIG_PREEMPTION
>>>> -	if (tid_to_cpu(tid) != tid_to_cpu(actual_tid))
>>>> +	if (IS_ENABLED(CONFIG_PREEMPTION) && tid_to_cpu(tid) != tid_to_cpu(actual_tid)) {
>>>
>>> nit: insert newline after && as it's more than 80 columns?
>>
>> I noticed this too, but using the script ./scripts/checkpatch.pl , there were no warnings.
>>
>> $ ./scripts/checkpatch.pl 0001-mm-slub-Refactor-note_cmpxchg_failure-for-better-rea.patch 
>> total: 0 errors, 0 warnings, 22 lines checked
>>
>> 0001-mm-slub-Refactor-note_cmpxchg_failure-for-better-rea.patch has no obvious style problems and is ready for submission.
> 
> IIUC the default limit is increased to 100 characters in checkpatch
> but 80 characters is still preferred.
Thank you for your patience.
I will update the V2 version and include your Reviewed-by: Harry Yoo <harry.yoo@oracle.com>
> 
> Quoting Documentation/process/coding-style.rst:
> 
> 2) Breaking long lines and strings
> ----------------------------------
> 
> Coding style is all about readability and maintainability using commonlu
> available tools.
> 
> The preferred limit on the length of a single line is 80 columns.
> 
> Statements longer than 80 columns should be broken into sensible chunks,
> unless exceeding 80 columns significantly increases readability and does
> not hide information.
> 
> Descendants are always substantially shorter than the parent and                
> are placed substantially to the right.  A very commonly used style              
> is to align descendants to a function open parenthesis.                         
> 
> These same rules are applied to function headers with a long argument list.
> 
> However, never break user-visible strings such as printk messages because
> that breaks the ability to grep for them. 
> 
> And also quoting a commit message:
> 
> commit bdc48fa11e46f867ea4d75fa59ee87a7f48be144
> Author: Joe Perches <joe@perches.com>
> Date:   Fri May 29 16:12:21 2020 -0700
> 
>     checkpatch/coding-style: deprecate 80-column warning
> 
>     Yes, staying withing 80 columns is certainly still _preferred_.  But
>     it's not the hard limit that the checkpatch warnings imply, and other
>     concerns can most certainly dominate.
> 
>     Increase the default limit to 100 characters.  Not because 100
>     characters is some hard limit either, but that's certainly a "what are
>     you doing" kind of value and less likely to be about the occasional
>     slightly longer lines.
> 
>     Miscellanea:
> 
>      - to avoid unnecessary whitespace changes in files, checkpatch will no
>        longer emit a warning about line length when scanning files unless
>        --strict is also used
> 
>      - Add a bit to coding-style about alignment to open parenthesis
> 
>     Signed-off-by: Joe Perches <joe@perches.com>
>     Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
> 
>>>>  		pr_warn("due to cpu change %d -> %d\n",
>>>>  			tid_to_cpu(tid), tid_to_cpu(actual_tid));
>>>> -	else
>>>> -#endif
>>>> -	if (tid_to_event(tid) != tid_to_event(actual_tid))
>>>> +	} else if (tid_to_event(tid) != tid_to_event(actual_tid)) {
>>>>  		pr_warn("due to cpu running other code. Event %ld->%ld\n",
>>>>  			tid_to_event(tid), tid_to_event(actual_tid));
>>>> -	else
>>>> +	} else {
>>>>  		pr_warn("for unknown reason: actual=%lx was=%lx target=%lx\n",
>>>>  			actual_tid, tid, next_tid(tid));
>>>> +	}
>>>>  #endif
>>>>  	stat(s, CMPXCHG_DOUBLE_CPU_FAIL);
>>>>  }
>>>> -- 
>>>> 2.43.0
>>
>> -- 
>> Thanks,
>> Ye Liu
>>
>>
> 

-- 
Thanks,
Ye Liu