Re: posix-cpu-timers revamp

[Roland McGrath <roland@redhat.com>]

> Yeah, I checked that out.  The one difference here is that that was a
> race between do_exit() (actually release_task()/__exit_signal()) and
> run_posix_cpu_timers().  While this race was the same in that respect,
> there was also a race between all of the timer-tick routines that call
> any of account_group_user_time(), account_group_system_time() or
> account_group_exec_runtime() and __exit_signal().  This is because those
> functions all dereference tsk->signal.

The essence that matters is the same: something that current does with its
own ->signal on a tick vs something that the release_task path does.

> Erm, well, this isn't reorganizing the data structures per se, since
> these are new data structures.

Tomato, tomato.  You're adding new data structures and lifetime rules to
replace data that was described in a different data structure before, yet
your new data's meaningful semantic lifetime exactly matches that of
signal_struct.  You could as well make everything release_task cleans up be
done in __put_task_struct instead, but that would not be a good idea
either.  You've added a word to task_struct (100000 words per 100000-thread
process, vs one word).  It's just not warranted.

> The upshot of this is that none of the timer routines dereference
> tsk->signal, so the races go away, no locking needed.  From my
> perspective this was the simplest solution, since lock dependency
> ordering is _really_ a can of worms.

With the perspective of tunnel vision to just your one test case, adding
something entirely new considering only that case always seems simplest.
That's not how we keep the entire system from getting the wrong kinds of
complexity.

> Regarding the second approach, without locking wouldn't that still be
> racy?  Couldn't exit_state change (and therefore __exit_signal() run)
> between the check and the dereference?

No.  current->exit_state can go from zero to nonzero only by current
running code in the do_exit path.  current does not progress on that
path while current is inside one update_process_times call.


Thanks,
Roland