Re: [GIT PULL] ring-buffer: Replace this_cpu_*() with __this_cpu_*()

[Steven Rostedt <rostedt@goodmis.org>]

Christoph,

Any comment on this?

-- Steve


On Thu, 19 Mar 2015 15:16:25 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> Ugh.
> 
> I think this is bogus.
> 
> Why?
> 
> On Thu, Mar 19, 2015 at 3:02 PM, Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> >     The generic version of this_cpu_read() and write() are:
> >
> >      #define this_cpu_generic_read(pcp)                                 \
> >      ({ typeof(pcp) ret__;                                              \
> >         preempt_disable();                                              \
> >         ret__ = *this_cpu_ptr(&(pcp));                                  \
> >         preempt_enable();                                               \
> >         ret__;                                                          \
> >      })
> >
> >      #define this_cpu_generic_to_op(pcp, val, op)                               \
> >      do {                                                                       \
> >         unsigned long flags;                                            \
> >         raw_local_irq_save(flags);                                      \
> >         *__this_cpu_ptr(&(pcp)) op val;                                 \
> >         raw_local_irq_restore(flags);                                   \
> >      } while (0)
> 
> Let's just fix the generic versions of this_cpu_read/write.
> 
> Now, it is true that for the "op" versions of "this_cpu_xyz" we need
> to disable preemption or interrupts or something for the generic case
> (where "generic case" is weasel-wording for "the architecture is crap
> and has horrible problems with any kinds of atomics, even just per-cpu
> ones").
> 
> But that is *not* true for plain read/write. There is no point in
> disabling preemption, because in the end, if preemption was enabled,
> it happens on a random CPU anyway (and that may be fine - many
> heuristics may not care *which* exact CPU it's about), and unlike the
> "op" cases, the actual read or write is a single access, so there's no
> reason to disable preemption/interrupts to make it "atomic" on that
> random CPU.
> 
> If you pair a this_cpu_read with a this_cpu_write and expect them to
> go to the same cpu, such a user obviously needs to disable preemption
> etc for bigger reasons - but disabling preemption inside the operation
> itself does absolutely nothing.
> 
> So is there really any reason to not make those simpler forms just do
> something simpler like
> 
>    #define this_cpu_generic_read(pcp) \
>       READ_ONCE(*this_cpu_ptr(&(pcp)))
> 
>   #define this_cpu_generic_read(pcp, val) \
>       WRITE_ONCE(*this_cpu_ptr(&(pcp)), val)
> 
> instead?
> 
> Even if we end up being preempted in the middle, do we *care*? It's
> going to one or the other CPU.
> 
> The only issue might be CPU hotplug in between (the previous CPU going
> away), but I'm not sure even that matterts.
> 
> So I don't think the ring-buffer change is necessarily _wrong_, but if
> this is a performance issue, why don't we just fix it up for the
> generic case rather than for just one user?
> 
> Or is the hotplug issue a big deal? I thought we already had some
> rcu-sched point for the cpu going away, so that even the hotplug case
> should be ok.
> 
>                            Linus