Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
MIME-Version: 1.0
References: <20231010164234.140750-1-ubizjak@gmail.com>
In-Reply-To: <20231010164234.140750-1-ubizjak@gmail.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue, 10 Oct 2023 10:32:21 -0700
Message-ID: <CAHk-=whYWhZN52SJN-Th9x2L2V-vHtAXUgiy_nSJ3+vQU6ak4Q@mail.gmail.com>
Subject: Re: [PATCH v2 -tip] x86/percpu: Use C for arch_raw_cpu_ptr()
To:     Uros Bizjak <ubizjak@gmail.com>
Cc:     x86@kernel.org, linux-kernel@vger.kernel.org,
        Nadav Amit <namit@vmware.com>,
        Andy Lutomirski <luto@kernel.org>,
        Brian Gerst <brgerst@gmail.com>,
        Denys Vlasenko <dvlasenk@redhat.com>,
        "H . Peter Anvin" <hpa@zytor.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Josh Poimboeuf <jpoimboe@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, 10 Oct 2023 at 09:43, Uros Bizjak <ubizjak@gmail.com> wrote:
>
> Implementing arch_raw_cpu_ptr() in C, allows the compiler to perform
> better optimizations, such as setting an appropriate base to compute
> the address instead of an add instruction.

Hmm. I wonder..

> +       tcp_ptr__ = __raw_cpu_read(, this_cpu_off) + (unsigned long)(ptr); \

Do we really even want to use __raw_cpu_read(this_cpu_off) at all?

On my machines (I tested an Intel 8th gen laptop, and my AMD Zen 2
Threadripper machine), 'rdgsbase' seems to be basically two cycles.

I wonder if we'd be better off using that, rather than doing the load.

Yes, a load that hits in L1D$ will schedule better, so it's "cheaper"
in that sense. The rdgsbase instruction *probably* will end up only
decoding in the first decoder etc. But we're talking single-cycle kind
of effects, and the rdgsbase case should be much better from a cache
perspective and might use fewer memory pipeline resources to offset
the fact that it uses an unusual front end decoder resource...

Yes, yes, we'd have to make it an alternative, and do something like

    static __always_inline unsigned long new_cpu_offset(void)
    {
        unsigned long res;
        asm(ALTERNATIVE(
                        "movq %%gs:this_cpu_off,%0",
                        "rdgsbase %0",
                        X86_FEATURE_FSGSBASE)
                : "=r" (res));
        return res;
    }

but it still seems fairly straightforward. Hmm?

             Linus