All of lore.kernel.org
 help / color / mirror / Atom feed
From: Linus Torvalds <torvalds@linux-foundation.org>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will@kernel.org>, Boqun Feng <boqun.feng@gmail.com>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	"the arch/x86 maintainers" <x86@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	Marco Elver <elver@google.com>, Kees Cook <keescook@chromium.org>,
	Christoph Hellwig <hch@infradead.org>,
	Jens Axboe <axboe@kernel.dk>
Subject: Re: [RFC][PATCH 3/5] refcount: Improve out-of-line code-gen
Date: Thu, 9 Dec 2021 09:51:57 -0800	[thread overview]
Message-ID: <CAHk-=wh5UcTh6bxhk_NO05Uyk8aHY_PUQx5UyrOBMFH8ATHcWg@mail.gmail.com> (raw)
In-Reply-To: <20211209083305.GN16608@worktop.programming.kicks-ass.net>

On Thu, Dec 9, 2021 at 12:33 AM Peter Zijlstra <peterz@infradead.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

  reply	other threads:[~2021-12-09 17:53 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-08 18:36 [RFC][PATCH 0/5] refcount: Improve code-gen Peter Zijlstra
2021-12-08 18:36 ` [RFC][PATCH 1/5] atomic: Introduce atomic_{inc,dec,dec_and_test}_ofl() Peter Zijlstra
2021-12-09 12:42   ` Mark Rutland
2021-12-09 13:34     ` Peter Zijlstra
2021-12-08 18:36 ` [RFC][PATCH 2/5] refcount: Use atomic_*_ofl() Peter Zijlstra
2021-12-08 19:19   ` Peter Zijlstra
2021-12-08 20:56     ` Peter Zijlstra
2021-12-09 13:17   ` Mark Rutland
2021-12-09 17:00     ` Mark Rutland
2021-12-08 18:36 ` [RFC][PATCH 3/5] refcount: Improve out-of-line code-gen Peter Zijlstra
2021-12-09  8:33   ` Peter Zijlstra
2021-12-09 17:51     ` Linus Torvalds [this message]
2021-12-08 18:36 ` [RFC][PATCH 4/5] atomic,x86: Implement atomic_dec_and_test_ofl() Peter Zijlstra
2021-12-08 18:37 ` [RFC][PATCH 5/5] atomic: Document the atomic_{}_ofl() functions Peter Zijlstra
2021-12-09  8:25 ` [RFC][PATCH 0/5] refcount: Improve code-gen Peter Zijlstra
2021-12-09 16:19   ` Jens Axboe
2021-12-09 16:51     ` Peter Zijlstra

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAHk-=wh5UcTh6bxhk_NO05Uyk8aHY_PUQx5UyrOBMFH8ATHcWg@mail.gmail.com' \
    --to=torvalds@linux-foundation.org \
    --cc=axboe@kernel.dk \
    --cc=boqun.feng@gmail.com \
    --cc=elver@google.com \
    --cc=hch@infradead.org \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=peterz@infradead.org \
    --cc=will@kernel.org \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.