On Tue, Jun 06, 2017 at 06:15:51PM +0200, Peter Zijlstra wrote:
> On Tue, Jun 06, 2017 at 05:27:06PM +0200, Heiko Carstens wrote:
> > On Tue, Jun 06, 2017 at 04:45:57PM +0200, Christian Borntraeger wrote:
>
> > > As a side note, I am asking myself, though, why we do need the
> > > preempt_disable/enable for the cases where we use the opcodes
> > > like lao (atomic load and or to a memory location) and friends.
> >
> > Because you want the atomic instruction to be executed on the local cpu for
> > which you have to per cpu pointer. If you get preempted to a different cpu
> > between the ptr__ assignment and lan instruction it might be executed not
> > on the local cpu. It's not really a correctness issue.
>
> As per the previous email, I think it is a correctness issue wrt CPU
> hotplug.
In the specific case of SRCU, this might actually be OK. We have not
yet entered the SRCU read-side critical section, and SRCU grace periods
don't interact with CPU hotplug. And the per-CPU variable stick around.
So as long as one of the per-CPU counters is incremented properly,
it doesn't really matter which one is incremented.
But if you overwrite one CPU's counter with the incremented version of
some other CPU's counter, yes, that would be very bad indeed.
> > #define arch_this_cpu_to_op(pcp, val, op) \
> > { \
> > typedef typeof(pcp) pcp_op_T__; \
> > pcp_op_T__ val__ = (val); \
> > pcp_op_T__ old__, *ptr__; \
> > preempt_disable(); \
> > ptr__ = raw_cpu_ptr(&(pcp)); \
> > asm volatile( \
> > op " %[old__],%[val__],%[ptr__]\n" \
> > : [old__] "=d" (old__), [ptr__] "+Q" (*ptr__) \
> > : [val__] "d" (val__) \
> > : "cc"); \
> > preempt_enable(); \
> > }
> >
> > #define this_cpu_and_4(pcp, val) arch_this_cpu_to_op(pcp, val, "lan")
> >
> > However in reality it doesn't matter at all, since all distributions we
> > care about have preemption disabled.
>
> Well, either you support PREEMPT=y or you don't :-) If you do, it needs
> to be correct, irrespective of what distro's do with it.
>
> > So this_cpu_inc() should just generate three instructions: two to calculate
> > the percpu pointer and an additional asi for the atomic increment, with
> > operand specific serialization. This is supposed to be a lot faster than
> > disabling/enabling interrupts around a non-atomic operation.
>
> So typically we joke about s390 that it has an instruction for this
> 'very-complicated-thing', but here you guys do not, what gives? ;-)
Even mainframes have finite silicon area? ;-)
Thanx, Paul
