On Mon, 21 Nov 2016, Oleg Nesterov wrote:
On 11/21, Oleg Nesterov wrote:No, no, I meant that afaics both readers can see per_cpu_sum() != 0 and thus the writer won't be woken up. Till the next down_read/up_read. Suppose that we have 2 CPU's, both counters == 1, both readers decrement. its counter at the same time. READER_ON_CPU_0 READER_ON_CPU_1 --ctr_0; --ctr_1; if (ctr_0 + ctr_1) if (ctr_0 + ctr_1) wakeup(); wakeup(); Why we can't miss a wakeup?
But the patch is really: if (!(ctr_0 + ctr_1)). wrt to stale values is this like due to the data dependency we only see the real value of this_cpu ctr, and no guarantee for the other cpus? If so I had not considered that scenario, and yes we'd need stronger guarantees. I'd have to wonder if other users of per_cpu_sum() would fall into a similar trap. Hmm and each user seems to implement its own copy of the same thing.
And in fact I am not sure this optimization makes sense... But it would be nice to avoid wake_up() when the writer sleeps in rcu_sync_enter(). Or this is the "slow mode" sem (cgroup_threadgroup_rwsem).
Why do you think using per_cpu_sum() does not make sense? As mentioned in the changelog it optimizes for incoming readers while the writer is doing sync_enter and getting the regular rwsem. What am I missing?
I need to re-check, but what do you think about the change below?
While optimizing for multiple writers (rcu_sync_enter) is certainly valid (at least considering the cgroups rwsem you mention), I think that my heuristic covers the otherwise more common case. Could both optimizations not work together? Of course, the window of where readers_block == 1 is quite large, so there can be a lot of false positives. Thanks, Davidlohr
