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:

quoted

On Mon, 11 Mar 2019 at 15:56, Christopher Lameter [off-list ref] wrote:

quoted

On Mon, 11 Mar 2019, Björn Töpel wrote:

quoted

quoted

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-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel