Message-ID: <20231214153636.655e18ce@gandalf.local.home>
Date: Thu, 14 Dec 2023 15:36:36 -0500
From: Steven Rostedt <rostedt@...dmis.org>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: "linux-arch@...r.kernel.org" <linux-arch@...r.kernel.org>, LKML
 <linux-kernel@...r.kernel.org>, Linux Trace Kernel
 <linux-trace-kernel@...r.kernel.org>, Masami Hiramatsu
 <mhiramat@...nel.org>, Mark Rutland <mark.rutland@....com>, Mathieu
 Desnoyers <mathieu.desnoyers@...icios.com>
Subject: Re: [PATCH] ring-buffer: Remove 32bit timestamp logic

On Thu, 14 Dec 2023 11:44:55 -0800
Linus Torvalds <torvalds@...ux-foundation.org> wrote:

> On Thu, 14 Dec 2023 at 08:55, Steven Rostedt <rostedt@...dmis.org> wrote:
> >
> > And yes, this does get called in NMI context.  
> 
> Not on an i486-class machine they won't. You don't have a local apic
> on those, and they won't have any NMI sources under our control (ie
> NMI does exist, but we're talking purely legacy NMI for "motherboard
> problems" like RAM parity errors etc)

Ah, so we should not worry about being in NMI context without a 64bit cmpxchg?

> 
> > I had a patch that added:
> >
> > +       /* ring buffer does cmpxchg, make sure it is safe in NMI context */
> > +       if (!IS_ENABLED(CONFIG_ARCH_HAVE_NMI_SAFE_CMPXCHG) &&
> > +           (unlikely(in_nmi()))) {
> > +               return NULL;
> > +       }  
> 
> CONFIG_ARCH_HAVE_NMI_SAFE_CMPXCHG doesn't work on x86 in this context,
> because the issue is that yes, there's a safe 'cmpxchg', but that's
> not what you want.

Sorry, that's from another patch that I combined into this one that I added
in case there's architectures that have NMIs but need to avoid cmpxchg, as
this code does normal long word cmpxchg too. And that change *should* go to
you and stable. It's either just luck that things didn't crash on those
systems today. Or it happens so infrequently, nobody reported it.

> 
> You want the save cmpxchg64, which is an entirely different beast.

Understood, but this code has both. It's just that the "special" code I'm
removing does the 64-bit cmpxchg.

> 
> And honestly, I don't think that NMI_SAFE_CMPXCHG covers the
> double-word case anywhere else either, except purely by luck.

I still need to cover the normal cmpxchg. I'll keep that a separate patch.

> 
> In mm/slab.c, we also use a double-wide cmpxchg, and there the rule
> has literally been that it's conditional on
> 
>  (a) system_has_cmpxchg64() existing as a macro
> 
>  (b) using that macro to then gate - at runtime - whether it actually
> works or not
> 
> I think - but didn't check - that we essentially only enable the
> two-word case on x86 as a result, and fall back to the slow case on
> all other architectures - and on the i486 case.
> 
> That said, other architectures *do* have a working double-word
> cmpxchg, but I wouldn't guarantee it. For example, 32-bit arm does
> have one using ldrexd/strexd, but that only exists on arm v6+.
> 
> And guess what? You'll silently get a "disable interrupts, do it as a
> non-atomic load-store" on arm too for that case. And again, pre-v6 arm
> is about as relevant as i486 is, but my point is, that double-word
> cmpxchg you rely on simply DOES NOT EXIST on 32-bit platforms except
> under special circumstances.
> 
> So this isn't a "x86 is the odd man out". This is literally generic.
> 
> > Now back to my original question. Are you OK with me sending this to you
> > now, or should I send you just the subtle fixes to the 32-bit rb_time_*
> > code and keep this patch for the merge window?  
> 
> I'm absolutely not taking some untested random "let's do 64-bit
> cmpxchg that we know is broken on 32-bit using broken conditionals"
> shit.
> 
> What *would* work is that slab approach, which is essentially
> 
>   #ifndef system_has_cmpxchg64
>       #define system_has_cmpxchg64() false
>   #endif
> 
>         ...
>         if (!system_has_cmpxchg64())
>                 return error or slow case
> 
>         do_64bit_cmpxchg_case();
> 
> (although the slub case is much more indirect, and uses a
> __CMPXCHG_DOUBLE flag that only gets set when that define exists etc).
> 
> But that would literally cut off support for all non-x86 32-bit architectures.
> 
> So no. You need to forget about the whole "do a 64-bit cmpxchg on
> 32-bit architectures" as being some kind of solution in the short
> term.

But do all archs have an implementation of cmpxchg64, even if it requires
disabling interrupts? If not, then I definitely cannot remove this code.

If they all have an implementation, where some is just very slow, then that
is also perfectly fine. The only time cmpxchg64 gets called is on the slow
path, which is if an event is interrupted by an interrupt, and that
interrupt writes an event to the same buffer.

This doesn't happen often, and if it requires disabling interrupts, then
it shouldn't be much notice.

I just want to avoid the case that it will simply break, which is the NMI
case. In which case, would:

	if (!system_has_cmpxchg64() && in_nmi())
		return NULL;

Be OK?

-- Steve

Powered by blists - more mailing lists