linux-kernel - Re: [GIT PULL] ring-buffer: Replace this_cpu_*() with __this_cpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 19 Mar 2015 15:16:25 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Steven Rostedt <rostedt@...dmis.org>
Cc:	LKML <linux-kernel@...r.kernel.org>,
	Ingo Molnar <mingo@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	stable <stable@...r.kernel.org>,
	Christoph Lameter <cl@...ux.com>,
	Uwe Kleine-Koenig <u.kleine-koenig@...gutronix.de>
Subject: Re: [GIT PULL] ring-buffer: Replace this_cpu_*() with __this_cpu_*()

Ugh.

I think this is bogus.

Why?

On Thu, Mar 19, 2015 at 3:02 PM, Steven Rostedt <rostedt@...dmis.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/