On Thu, Sep 05, 2019 at 03:02:49PM -0500, Eric W. Biederman wrote: > "Paul E. McKenney" <[email protected]> writes: > > > On Tue, Sep 03, 2019 at 10:06:03PM +0200, Peter Zijlstra wrote: > >> On Tue, Sep 03, 2019 at 12:18:47PM -0700, Linus Torvalds wrote: > >> > Now, if you can point to some particular field where that ordering > >> > makes sense for the particular case of "make it active on the > >> > runqueue" vs "look up the task from the runqueue using RCU", then I do > >> > think that the whole release->acquire consistency makes sense. > >> > > >> > But it's not clear that such a field exists, particularly when this is > >> > in no way the *common* way to even get a task pointer, and other paths > >> > do *not* use the runqueue as the serialization point. > >> > >> Even if we could find a case (and I'm not seeing one in a hurry), I > >> would try really hard to avoid adding extra barriers here and instead > >> make the consumer a little more complicated if at all possible. > >> > >> The Power folks got rid of a SYNC (yes, more expensive than LWSYNC) from > >> their __switch_to() implementation and that had a measurable impact. > >> > >> 9145effd626d ("powerpc/64: Drop explicit hwsync in context switch") > > > > The patch [1] looks good to me. And yes, if the structure pointed to by > > the second argument of rcu_assign_pointer() is already visible to readers, > > it is OK to instead use RCU_INIT_POINTER(). Yes, this loses ordering. > > But weren't these simple assignments before RCU got involved? > > > > As a very rough rule of thumb, LWSYNC is about twice as fast as SYNC. > > (Depends on workload, exact details of the hardware, timing, phase of > > the moon, you name it.) So removing the LWSYNC is likely to provide > > measureable benefit, but I must defer to the powerpc maintainers. > > To that end, I added Michael on CC. > > > > [1] https://lore.kernel.org/lkml/[email protected]/ > > Paul, what is the purpose of the barrier in rcu_assign_pointer? > > My intuition says it is the assignment half of rcu_dereference, and that > anything that rcu_dereference does not need is too strong. > > My basic understanding is that only alpha ever has the memory ordering > issue that rcu_dereference deals with. So I am a bit surprised that > this is anything other than a noop for anything except alpha.
Yes, only Alpha needs an actual memory-barrier instruction in rcu_dereference(). And it is the only one that gets one. > In my patch I used rcu_assign_pointer because that is the canonically > correct way to do things. Peter makes a good case that adding an extra > barrier in ___schedule could be detrimental to system performance. > At the same time if there is a correctness issue on alpha that we have > been overlooking because of low testing volume on alpha I don't want to > just let this slide and have very subtle bugs. But rcu_assign_pointer() needs a memory-barrier instruction on the weakly ordered architectures: ARM, powerpc, Itanium, and so on. > The practical concern is that people have been really wanting to do > lockless and rcu operations on tasks in the runqueue for a while and > there are several very clever pieces of code doing that now. By > changing the location of the rcu put I am trying to make these uses > ordinary rcu uses. > > The uses in question are the pieces of code I update in: > https://lore.kernel.org/lkml/[email protected]/ The rcu_assign_pointer() at the end of rcuwait_wait_event(), right? > In short. Why is rcu_assign_pointer() more than WRITE_ONCE() on > anything but alpha? Do other architectures need more? Is the trade off > worth it if we avoid using rcu_assign_pointer on performance critical > paths. Note the difference between the read-side rcu_dereference(), which does not require any memory-barrier instructions except on Alpha, and the update-side rcu_assign_pointer() which does require a memory-barrier instruction on quite a few architectures. Even on the strongly ordered architectures (x86, s390, ...) a compiler barrier() is required for rcu_assign_pointer(). Except note the exceptional cases where RCU_INIT_POINTER() may be used in place of rcu_assign_pointer(), which are called out in RCU_INIT_POINTER()'s header comment: * Initialize an RCU-protected pointer in special cases where readers * do not need ordering constraints on the CPU or the compiler. These * special cases are: * * 1. This use of RCU_INIT_POINTER() is NULLing out the pointer *or* * 2. The caller has taken whatever steps are required to prevent * RCU readers from concurrently accessing this pointer *or* * 3. The referenced data structure has already been exposed to * readers either at compile time or via rcu_assign_pointer() *and* * * a. You have not made *any* reader-visible changes to * this structure since then *or* * b. It is OK for readers accessing this structure from its * new location to see the old state of the structure. (For * example, the changes were to statistical counters or to * other state where exact synchronization is not required.) * * Failure to follow these rules governing use of RCU_INIT_POINTER() will * result in impossible-to-diagnose memory corruption. As in the structures * will look OK in crash dumps, but any concurrent RCU readers might * see pre-initialized values of the referenced data structure. So * please be very careful how you use RCU_INIT_POINTER()!!! If current is already visible (which it should be unless rcuwait_wait_event() is invoked at task-creation time), then RCU_INIT_POINTER() could be used in rcuwait_wait_event(). > Eric > > p.s. I am being slow at working through all of this as I am dealing > with my young baby son, and busy packing for the conference. Congratulations on the baby son!!! I remember those times well, but they were more than three decades ago for my oldest. ;-) > I might not be able to get back to this discussion until after > I have landed in Lisbon on Saturday night. Looking forward to it! Thanx, Paul
