On Tue, Feb 12, 2013 at 10:31:48AM -0800, Paul E. McKenney wrote: > On Tue, Feb 12, 2013 at 12:25:26PM -0500, Johannes Weiner wrote: > > On Tue, Feb 12, 2013 at 08:10:51AM -0800, Paul E. McKenney wrote: > > > On Tue, Feb 12, 2013 at 04:43:30PM +0100, Michal Hocko wrote: > > > > On Tue 12-02-13 10:10:02, Johannes Weiner wrote: > > > > > On Tue, Feb 12, 2013 at 10:54:19AM +0100, Michal Hocko wrote: > > > > > > On Mon 11-02-13 17:39:43, Johannes Weiner wrote: > > > > > > > On Mon, Feb 11, 2013 at 10:27:56PM +0100, Michal Hocko wrote: > > > > > > > > On Mon 11-02-13 14:58:24, Johannes Weiner wrote: > > > > > > > > > That way, if the dead count gives the go-ahead, you KNOW that the > > > > > > > > > position cache is valid, because it has been updated first. > > > > > > > > > > > > > > > > OK, you are right. We can live without css_tryget because dead_count is > > > > > > > > either OK which means that css would be alive at least this rcu period > > > > > > > > (and RCU walk would be safe as well) or it is incremented which means > > > > > > > > that we have started css_offline already and then css is dead already. > > > > > > > > So css_tryget can be dropped. > > > > > > > > > > > > > > Not quite :) > > > > > > > > > > > > > > The dead_count check is for completed destructions, > > > > > > > > > > > > Not quite :P. dead_count is incremented in css_offline callback which is > > > > > > called before the cgroup core releases its last reference and unlinks > > > > > > the group from the siblinks. css_tryget would already fail at this stage > > > > > > because CSS_DEACT_BIAS is in place at that time but this doesn't break > > > > > > RCU walk. So I think we are safe even without css_get. > > > > > > > > > > But you drop the RCU lock before you return. > > > > > > > > > > dead_count IS incremented for every destruction, but it's not reliable > > > > > for concurrent ones, is what I meant. Again, if there is a dead_count > > > > > mismatch, your pointer might be dangling, easy case. However, even if > > > > > there is no mismatch, you could still race with a destruction that has > > > > > marked the object dead, and then frees it once you drop the RCU lock, > > > > > so you need try_get() to check if the object is dead, or you could > > > > > return a pointer to freed or soon to be freed memory. > > > > > > > > Wait a moment. But what prevents from the following race? > > > > > > > > rcu_read_lock() > > > > mem_cgroup_css_offline(memcg) > > > > root->dead_count++ > > > > iter->last_dead_count = root->dead_count > > > > iter->last_visited = memcg > > > > // final > > > > css_put(memcg); > > > > // last_visited is still valid > > > > rcu_read_unlock() > > > > [...] > > > > // next iteration > > > > rcu_read_lock() > > > > iter->last_dead_count == root->dead_count > > > > // KABOOM > > > > > > > > The race window between dead_count++ and css_put is quite big but that > > > > is not important because that css_put can happen anytime before we start > > > > the next iteration and take rcu_read_lock. > > > > > > The usual approach is to make sure that there is a grace period (either > > > synchronize_rcu() or call_rcu()) between the time that the data is > > > made inaccessible to readers (this would be mem_cgroup_css_offline()?) > > > and the time it is freed (css_put(), correct?). > > > > Absolutely! And there is a synchronize_rcu() in between those two > > operations. > > > > However, we want to keep a weak reference to the cgroup after we drop > > the rcu read-side lock, so rcu alone is not enough for us to guarantee > > object life time. We still have to carefully detect any concurrent > > offlinings in order to validate the weak reference next time around. > > That would make things more interesting. ;-) > > Exactly who or what holds the weak reference? And the idea is that if > you attempt to use the weak reference beforehand, the css_put() does not > actually free it, but if you attempt to use it afterwards, you get some > sort of failure indication? Yes, exactly. We are using a seqlock-style cookie comparison to see if any objects in the pool of objects that we may point to was destroyed. We are having trouble to agree on how to safely read the counter :-) Long version: It's an iterator over a hierarchy of cgroups, but page reclaim may stop iteration at will and might not come back for an indefinite amount of time (until memory pressure triggers reclaim again). So we want to allow cgroups to be destroyed while one of the iterators may still pointing at it (we have iterators per-node, per-zone, per reclaim priority level, that's why it's not feasible to invalidate them pro-actively upon cgroup destruction). The idea is that we have a counter that counts cgroup destructions in each cgroup hierarchy and we remember a snapshot of that counter at the time we remember the iterator position. If any group in that group's hierarchy gets killed before we come back to the iterator, the counter mismatches. Easy. If any group is getting killed concurrently, the counter might match our cookie, but the object could be marked dead already, while rcu prevents it from being freed. The remaining worry is/was that we have two reads of the destruction counter: one when validating the weak reference, another one when updating the iterator. If a destruction starts in between those two, and modifies the counter, we would miss that destruction and the object that is now weakly referenced could get freed while the corresponding snapshot matches the latest value of the destruction counter. Michal's idea was to hold off the destruction counter inc between those reads with synchronize_rcu(). My idea was to simply read the counter only once and use that same value to both check and update the iterator with. That should catch this type of race condition and save the atomic & the extra synchronize_rcu(). At least I fail to see the downside of reading it only once: iteration: rcu_read_lock() dead_count = atomic_read(&hierarchy->dead_count) smp_rmb() previous = iterator->position if (iterator->dead_count != dead_count) /* A cgroup in our hierarchy was killed, pointer might be dangling */ don't use iterator if (!tryget(&previous)) /* The cgroup is marked dead, don't use it */ don't use iterator next = find_next_and_tryget(hierarchy, &previous) /* what happens if destruction of next starts NOW? */ css_put(previous) iterator->position = next smp_wmb() iterator->dead_count = dead_count /* my suggestion, instead of a second atomic_read() */ rcu_read_unlock() return next /* caller drops ref eventually, iterator->cgroup becomes weak */ destruction: bias(cgroup->refcount) /* disables future tryget */ //synchronize_rcu() /* Michal's suggestion */ atomic_inc(&cgroup->hierarchy->dead_count) synchronize_rcu() free(cgroup) -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>