Re: [PATCH v2 4/4] task: RCUify the assignment of rq->curr

From: Eric W. Biederman
Date: Sun Sep 15 2019 - 14:32:05 EST

ebiederm@xxxxxxxxxxxx (Eric W. Biederman) writes:

> "Paul E. McKenney" <paulmck@xxxxxxxxxx> 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.