Re: per-cpu thoughts

[Mark Rutland <mark.rutland@arm.com>]

Hi Paul,

On Mon, Mar 11, 2019 at 08:26:45AM -0700, Paul Walmsley wrote:
> + the ARM64 guys and lakml
> 
> On Mon, 11 Mar 2019, Björn Töpel wrote:
> 
> > On Mon, 11 Mar 2019 at 15:56, Christopher Lameter <cl@linux.com> wrote:
> > >
> > > On Mon, 11 Mar 2019, Björn Töpel wrote:
> > >
> > > > > Thanks a bunch!  I feel like the best option here is to just use the AMOs
> > > > > without disabling preemption and ensuring that all other accesses are atomic
> > > > > (via AMOs or LR/SC).  The only reason I can see that wouldn't be the way to go
> > > > > would be if it requires non-arch modifications, as if we go down that path
> > > > > we'll be able to handle the performance edge cases in the implementation.
> > > >
> > > > Hmm, you mean AMO *with* preempt_{dis,en}able, right? (No, interrupt
> > > > disable, only preemption.)
> > >
> > > If you disable preemption then you dont need AMO anymore. In fact that is
> > > the default fallback generic implementation. Just dont do anything and you
> > > already have that solution.
> > 
> > But the generic one disables interrupts, right?
> > 
> > I believe the rational for RV is similar to ARM's; AMO+preemption
> > disable regions is *slightly* better than the generic, but not as good
> > as the IA one. Or am I missing something?
> 
> There's been a discussion going on in a private thread about this that I 
> unfortunately didn't add you to.  The discussion is still ongoing, but I 
> think Christoph and myself and a few other folks have agreed that the 
> preempt_disable/enable is not needed for the amoadd approach.  This is 
> since the apparent intention of the preemption disable/enable is to ensure 
> the correctness of the counter increment; however there is no risk of 
> incorrectness in an amoadd sequence since the atomic add is locked across 
> all of the cache coherency domain. 

We also thought that initially, but there's a sbutle race that can
occur, and so we added code to disable preemption in commit:

  f3eab7184ddcd486 ("arm64: percpu: Make this_cpu accessors pre-empt safe")

The problem on arm64 is that our atomics take a single base register,
and we have to generate the percpu address with separate instructions
from the atomic itself. That means we can get preempted between address
generation and the atomic, which is problematic for sequences like:

	// Thread-A			// Thread-B

	this_cpu_add(var)
					local_irq_disable(flags)
					...
					v = __this_cpu_read(var);
					v = some_function(v);
					__this_cpu_write(var, v);
					...
					local_irq_restore(flags)

... which can unexpectedly race as:


	// Thread-A			// Thread-B
	
	< generate CPU X addr >
	< preempted >

					< scheduled on CPU X >
					local_irq_disable(flags);
					v = __this_cpu_read(var);

	< scheduled on CPU Y >
	< add to CPU X's var >
					v = some_function(v);
					__this_cpu_write(var, v);
					local_irq_restore(flags);

... and hence we lose an update to a percpu variable.

I suspect RISC-V would have the same problem, unless its AMOs can
generate the percpu address and perform the update in a single
instruction.

> There are a few outstanding points that we're trying to talk through, but 
> it should be fine for an initial implementation to start with the 
> amoadd-based approach.
> 
> As far as the ARM LSE atomic implementation goes, I'm not an expert on 
> those instructions. If those instructions are locked across all of the 
> caches for the cores in the Linux system also, then they probably don't 
> need the preempt_disable/enable either - assuming our collective 
> understanding of the purpose of the preempt_disable/enable is correct.
> 
> All this is, of course, assuming there is no secondary purpose to the 
> preempt_disable/enable that we haven't managed to elicit yet.

Sorry to be the bearer of bad news! :)

FWIW, I had a go at building percpu ops that didn't need to disable
preemption, but that required LL/SC atomics, reserving a GPR for the
percpu offset, and didn't result in a measurable difference in practice.
The patches are at:

https://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git/commit/?h=arm64/this-cpu-reg&id=84ee5f23f93d4a650e828f831da9ed29c54623c5

Thanks,
Mark.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv