linux-kernel - Re: [PATCH] mm/slub: Refactor note_cmpxchg

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aL6gMEvR8RqW7YrE@hyeyoo>
Date: Mon, 8 Sep 2025 18:21:52 +0900
From: Harry Yoo <harry.yoo@...cle.com>
To: Ye Liu <ye.liu@...ux.dev>
Cc: Vlastimil Babka <vbabka@...e.cz>,
        Andrew Morton <akpm@...ux-foundation.org>, Ye Liu <liuye@...inos.cn>,
        Christoph Lameter <cl@...two.org>,
        David Rientjes <rientjes@...gle.com>,
        Roman Gushchin <roman.gushchin@...ux.dev>, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/slub: Refactor note_cmpxchg_failure for better
 readability

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@...inos.cn>
> >>
> >> Use IS_ENABLED() and standard if-else to make the code clearer.
> >>
> >> Signed-off-by: Ye Liu <liuye@...inos.cn>
> >> ---
> > 
> > Looks good to me,
> > Reviewed-by: Harry Yoo <harry.yoo@...cle.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@...ches.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@...ches.com>
    Signed-off-by: Linus Torvalds <torvalds@...ux-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