Message-ID: <CAFULd4avm_TaEoRauohRc90SUrx-D+wBJvg+htQDQ1_N=zNemw@mail.gmail.com>
Date:   Thu, 19 Oct 2023 09:04:30 +0200
From:   Uros Bizjak <ubizjak@...il.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     peterz@...radead.org, Nadav Amit <namit@...are.com>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Andy Lutomirski <luto@...nel.org>,
        Brian Gerst <brgerst@...il.com>,
        Denys Vlasenko <dvlasenk@...hat.com>,
        "H . Peter Anvin" <hpa@...or.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Josh Poimboeuf <jpoimboe@...hat.com>,
        Nick Desaulniers <ndesaulniers@...gle.com>
Subject: Re: [PATCH v2 -tip] x86/percpu: Use C for arch_raw_cpu_ptr()

On Thu, Oct 19, 2023 at 12:40 AM Linus Torvalds
<torvalds@...ux-foundation.org> wrote:
>
> On Wed, 18 Oct 2023 at 14:40, Uros Bizjak <ubizjak@...il.com> wrote:
> >
> > The ones in "raw" form are not IRQ safe and these are implemented
> > without volatile qualifier.
>
> You are misreading it.
>
> Both *are* irq safe - on x86.
>
> The difference between "this_cpu_xyz()" and "raw_cpu_xyz()" is that on
> *other* architectures, "raw_cpu_xyz():" can be a lot more efficient,
> because other architectures may need to do extra work to make the
> "this" version be atomic on a particular CPU.
>
> See for example __count_vm_event() vs count_vm_event().
>
> In fact, that particular use isn't even in an interrupt-safe context,
> that's an example of literally "I'd rather be fast that correct for
> certain statistics that aren't all that important".
>
> They two versions generate the same code on x86, but on other
> architectures, __count_vm_event() can *much* simpler and faster
> because it doesn't disable interrupts or do other special things.
>
> But on x86, the whole "interrupt safety" is a complete red herring.
> Both of them generate the exact same instruction.
>
> On x86, the "volatile" is actually for a completely different reason:
> to avoid too much CSE by the compiler.

Let me explain how the compiler handles volatile. Basically, it
disables most of the optimizations when volatile is encountered, be it
on volatile asm or volatile memory access. It is important that
argument propagation is NOT disabled with volatile memory, so the
compiler can still propagate arguments into the following instruction,
as long as no memory access is crossed. This is all that [1]
implements, while keeping volatile around.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=x86/percpu&id=ca4256348660cb2162668ec3d13d1f921d05374a

Remark: You raised a question on why "testl $0xf0000,%gs:0x0(%rip)"
was not optimized to testb - because of volatile access on memory, the
compiler won't shrink memory read.

The compiler won't CSE volatile-asm or volatile reads, so the number
of memory accesses stay the same. This is an important property,
promised by this_* API, and this_* should be used almost everywhere
because of this property.

raw_* is a relaxed version that allows CSE of asm and memory accesses.
The number of memory accesses can change, and additionally, it *can*
rematerialize arguments from memory [2].

[2] If there is an instruction that uses all registers in between the
load from memory and the insn that uses the loaded value, then the
compiler will try to accommodate the instruction by pushing all active
registers to the stack (stack is owned by the compiler). Instead of
pushing and popping the register with a loaded value, it can read it
from the original non-volatile location as well. If the location is
qualified as volatile, this optimization is off.

This is the rematerialization that you are worrying about. As well as
CSE, it can be allowed under special conditions, in a section, where
it is *guaranteed* that the value in memory won't change in the whole
section. This can be guaranteed in a (kind of) critical section (e.g.
when interrupts are disabled), otherwise the rematerialized value can
differ, because some interrupt handler changed value in memory. Also,
the opposite is true, we will miss the changed value when the read is
CSE'd.

That means, that even when the instruction is the same (and IRQ safe),
"volatile" inhibits unwanted optimisations, and should be there
because of the compiler, to guarantee the access to memory. Since RMW
asms are accessing memory, they should be volatile, too for
non-relaxed versions.

Following with the raw_* issue: These are relaxed versions that
require stable operands, in a section, marked with some barriers (so,
critical section). Those barriers (e.g. volatile asm memory clobber)
will prevent scheduling of memory accesses outside this critical
section, and memory operand stability will be achieved by e.g.
disabling interrupts. Since the user *guarantees* memory stability,
the compiler can relax its CSE and rematerialization requirement *for
this section*. Accessors that are implemented without "volatile"
communicate this relaxation to the compiler.

The above also means that when raw_* versions are used on non-stable
operands, it is the user's fault to use it at the first place, not the
compiler's. raw_ versions should be used with great care.

To illustrate the above, let's look at the "xchg" RMW implementation.
The this_* one is interrupt safe in the sense that it is implemented
with cmpxchg operation that is atomic w.r.t interrupts (that can
change contents of the memory) [note, this is not "locked", and does
not have full atomic property], but raw_* is implemented simply by
raw_ reads and writes. In the raw_ case, we don't care what happens
with memory accesses, because they are required to be stable in the
section where raw_xchg is used.

Then we have this_cpu_read_stable that has even stricter requirements
on operand stability. It can be considered as a constant memory, and
now we have the means to communicate this to the compiler.

[Side note: this_cpu_read_stable is a misnomer. It should be named
raw_cpu_read_stable to reflect the operand stability, required by raw_
versions.

>From the compiler standpoint, "volatiles" are not optional, they
communicate operand stability state to the compiler. I have reviewed
percpu.h many times, and the current state of operations is correct,
even if they result in the same instruction.

BTW: About that raw_smp_processor_id():

The raw_ version should stay defined as
this_cpu_read(pcpu_hot.cpu_number), while __smp_processor_id() can be
redefined to raw_cpu_read. Please note the usage in
include/linux/smp.h:

#define get_cpu() ({ preempt_disable(); __smp_processor_id(); })
#define put_cpu() preempt_enable()

where preempt_disable and preempt_enable mark the boundaries of the
critical section.

Uros.

> See  commit b59167ac7baf ("x86/percpu: Fix this_cpu_read()").
>
> In fact, that commit went overboard, and just added "volatile" to
> *every* percpu read.
>
> So then people complained about *that*, and PeterZ did commit
> 0b9ccc0a9b14 ("x86/percpu: Differentiate this_cpu_{}() and
> __this_cpu_{}()"), which basically made that "qual or not" be a macro
> choice.
>
> And in the process, it now got added to all the RMW ops, that didn't
> actually need it or want it in the first place, since they won't be
> CSE'd, since they depend on the input.
>
> So that commit basically generalized the whole thing entirely
> pointlessly, and caused your current confusion.
>
> End result: we should remove 'volatile' from the RMW ops. It doesn't
> do anything on x86. All it does is make us have two subtly different
> versions that we don't care about the difference.
>
> End result two: we should make it clear that "this_cpu_read()" vs
> "raw_cpu_read()" are *NOT* about interrupts. Even on architectures
> where the RMW ops need to have irq protection (so that they are atomic
> wrt interrupts also modifying the value), the *READ* operation
> obviously has no such issue.
>
> For the raw_cpu_read() vs this_cpu_read() case, the only issue is
> whether you can CSE the result.
>
> And in 99% of all cases, you can - and want to - CSE it. But as that
> commit b59167ac7baf shows, sometimes you cannot.
>
> Side note: the code that caused that problem is this:
>
>   __always_inline void __cyc2ns_read(struct cyc2ns_data *data)
>   {
>         int seq, idx;
>
>         do {
>                 seq = this_cpu_read(cyc2ns.seq.seqcount.sequence);
>                 ...
>         } while (unlikely(seq != this_cpu_read(cyc2ns.seq.seqcount.sequence)));
>   }
>
> where the issue is that the this_cpu_read() of that sequence number
> needs to be ordered.
>
> Honestly, that code is just buggy and bad.  We should never have
> "fixed" it by changing the semantics of this_cpu_read() in the first
> place.
>
> The problem is that it re-implements its own locking model, and as so
> often happens when people do that, they do it completely wrongly.
>
> Look at the *REAL* sequence counter code in <linux/seqlock.h>. Notice
> how in raw_read_seqcount_begin() we have
>
>         unsigned _seq = __read_seqcount_begin(s);
>         smp_rmb();
>
> because it actually does the proper barriers. Notice how the garbage
> code in __cyc2ns_read() doesn't have them - and how it was buggy as a
> result.
>
> (Also notice how this all predates our "we should use load_acquire()
> instead of smb_rmb()", but whatever).
>
> IOW, all the "volatiles" in the x86 <asm/percpu.h> file are LITERAL
> GARBAGE and should not exist, and are due to a historical mistake.
>
>                    Linus

Powered by blists - more mailing lists