linux-kernel - Re: [RFC][PATCH 3/5] refcount: Improve out-of-line code-gen

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] [day] [month] [year] [list]

Date:   Thu, 9 Dec 2021 09:51:57 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Peter Zijlstra <peterz@...radead.org>
Cc:     Will Deacon <will@...nel.org>, Boqun Feng <boqun.feng@...il.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Marco Elver <elver@...gle.com>,
        Kees Cook <keescook@...omium.org>,
        Christoph Hellwig <hch@...radead.org>,
        Jens Axboe <axboe@...nel.dk>
Subject: Re: [RFC][PATCH 3/5] refcount: Improve out-of-line code-gen

On Thu, Dec 9, 2021 at 12:33 AM Peter Zijlstra <peterz@...radead.org> wrote:
>
> For some obscure raisin it causes this: [snip]

Looks like the code generation at one point thought that it needs to
get the return value from the slow-path function call, and by the time
it had optimized that away it had already ended up with enough
temporaries that it needed to spill things into %rbx.

That looks like a fairly "internal to gcc" thing, but I think dropping
that patch is indeed the right thing to do.

When you made it do

        return refcount_warn_saturate(r, ..);

it now means that those inline functions don't obviously always return
'false' any more, so the compiler did end up needing a variable for
the return value at one point, and it really was a much more complex
decision. After it has inlined that top-level function, the return
value is not visible to the compiler because it doesn't see that
refcount_warn_saturate() always returns false.

The fact that quite often that whole refcount_warn_saturate() call
ended up then being in a cold part of the code, and could then be
optimized to a tailcall if that was the last thing generated, doesn't
end up helping in the general case, and instead only hurts: the
compiler doesn't see what the return value is for the warning case, so
it might end up doing other things in the place that the function was
inlined into.

I think the original patch was likely a mis-optimization triggered by
your test-case being a function that *only* did that
refcount_add_not_zero(), and returned the value. It didn't actually
have other code that then depended on the return value of it.

              Linus