Messages in this thread | | | From | (Eric W. Biederman) | Date | Sun, 15 Sep 2019 13:25:02 -0500 | Subject | Re: [PATCH v2 4/4] task: RCUify the assignment of rq->curr |
| |
ebiederm@xmission.com (Eric W. Biederman) writes:
> "Paul E. McKenney" <paulmck@kernel.org> writes: > >> So this looks good in and of itself, but I still do not see what prevents >> the unfortunate sequence of events called out in my previous email. >> On the other hand, if ->rcu and ->rcu_users were not allocated on top >> of each other by a union, I would be happy to provide a Reviewed-by. >> >> And I am fundamentally distrusting of a refcount_dec_and_test() that >> is immediately followed by code that clobbers the now-zero value. >> Yes, this does have valid use cases, but it has a lot more invalid >> use cases. The valid use cases have excluded all increments somehow >> else, so that the refcount_dec_and_test() call's only job is to >> synchronize between concurrent calls to put_task_struct_rcu_user(). >> But I am not seeing the "excluded all increments somehow". >> >> So, what am I missing here? > > Probably only that the users of the task_struct in this sense are now > quite mature. > > The two data structures that allow rcu access to the task_struct are > the pid hash and the runqueue. The practical problem is that they > have two very different lifetimes. So we need some kind of logic that > let's us know when they are both done. A recount does that job very > well. > > Placing the recount on the same storage as the unused (at that point) > rcu_head removes the need to be clever in other ways to avoid bloating > the task_struct. > > If you really want a reference to the task_struct from rcu context you > can just use get_task_struct. Because until the grace period completes > it is guaranteed that the task_struct has a positive count. > > Right now I can't imagine a use case for wanting to increase rcu_users > anywhere or to decrease rcu_users except where we do. If there is such > a case most likely it will increase the reference count at > initialization time. > > If anyone validly wants to increment rcu_users from an rcu critical > section we can move it out of the union at that time.
Paul were you worrying about incrementing rcu_users because Frederic Weisbecker brought the concept up earlier in the review?
It was his confusion that the point of rcu_users was so that it could be incremented from an rcu critical section. That definitely is not the point of rcu_users.
If you were wondering about someone messing with rcu_users from an rcu critical region independently that does suggest the code could use a "comment saying don't do that!" Multiple people getting confused about the purpose of a reference count independently does suggest there is a human factor problem in there somewhere.
Eric
| |