linux-kernel - Re: [PATCH v2] powerpc: Fix irq_soft_mask_set() and irq_soft_mask

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]

Date:   Thu, 1 Sep 2022 13:07:51 -0500
From:   Segher Boessenkool <segher@...nel.crashing.org>
To:     Gabriel Paubert <paubert@...m.es>
Cc:     Christophe Leroy <christophe.leroy@...roup.eu>,
        Michael Ellerman <mpe@...erman.id.au>,
        Zhouyi Zhou <zhouzhouyi@...il.com>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Nicholas Piggin <npiggin@...il.com>
Subject: Re: [PATCH v2] powerpc: Fix irq_soft_mask_set() and irq_soft_mask_return() with sanitizer

On Thu, Sep 01, 2022 at 09:37:42AM +0200, Gabriel Paubert wrote:
> On Thu, Sep 01, 2022 at 05:22:32AM +0000, Christophe Leroy wrote:
> > Le 01/09/2022 à 00:45, Segher Boessenkool a écrit :
> > > I would have used real assembler code here (in a .s file).  But there
> > > probably are reasons to do things this way, performance probably?
> > 
> > We changed it to inline asm in order to ... inline it in the caller.
> 
> And there is a single caller it seems. Typically GCC inlines function
> with a single call site, but it may be confused by asm statements.

"Confused"...  It might estimate the assembler statement to be
significantly more expensive than it really is.  The compiler has to be
somewhat conservative, to be able to generate code that can be assembled
without errors.  It counts instructions by counting newlines and semis
and that kind of thing.  Since GCC 7 there is "asm inline", to make the
compiler think for inlining purposes that the asm outputs minimum size
code.  You can use asm_inline in the kernel for this.

> > I also find that those operand names make it awull more difficult to 
> > read that traditional numbering. I really dislike that new trend.

Yup.  All the extra markup it needs doesn't benefit readability either.

> > And same with those // comments, better use meaningfull C variable names.

I wrote:

> > > Comments like "// Inputs" are just harmful.  As is the "creative"
> > > indentation here.  Both harm readability and do not help understanding
> > > in any other way either.

This is a comment trying to explain (GNU) C syntax.  I certainly agree
that variable naming is very important, but this was meant as commentary
on a worse comment offence :-)

Segher