Re: [PATCH RFC] time,signal: protect resource use statistics with seqlock

Rik van Riel
2014-08-14T19:05:27Z

On 08/14/2014 02:15 PM, Oleg Nesterov wrote:
> On 08/14, Rik van Riel wrote:
>>
>> On 08/14/2014 12:12 PM, Oleg Nesterov wrote:
>>>
>>> Or you can expand the scope of write_seqlock/write_sequnlock, so that
>>> __unhash_process in called from inside the critical section. This looks
>>> simpler at first glance.
>>
>> The problem with that is that wait_task_zombie() calls
>> thread_group_cputime_adjusted() in that if() branch, and
>> that code ends up taking the seqlock for read...
>
> Not sure I understand... This modifies parent->signal->c* counters,
> and obviously the exiting thread is not the member of parent's thread
> group, so thread_group_cputime_adjusted(parent) can never account the
> exiting child twice simply because it won't see it?

You are right, the tree of processes only goes one way,
so there should be no deadlock in taking psig->stats_lock
and having thread_group_cputime_adjusted take sig->stats_lock
for read within that section.

However, it might need some lockdep annotation to keep
lockdep from thinking we might the same lock recursively :)

>> However, in __exit_signal that approach should work.
>
> Yes,
>
>>> Hmm, wait, it seems there is yet another problem ;) Afaics, you also
>>> need to modify __exit_signal() so that ->sum_sched_runtime/etc are
>>> accounted unconditionally, even if the group leader exits.
>>>
>>> Probably this is not a big problem, and sys_times() or clock_gettime()
>>> do not care at all because they use current.
>>>
>>> But without this change thread_group_cputime(reaped_zombie) won't look
>>> at this task_struct at all, this can lead to non-monotonic result if
>>> it was previously called when this task was alive (non-reaped).
>>
>> You mean this whole block needs to run regardless of whether
>> the group is dead?
>>
>> task_cputime(tsk, &utime, &stime);
>> write_seqlock(&sig->stats_lock);
>> sig->utime += utime;
>> sig->stime += stime;
>> sig->gtime += task_gtime(tsk);
>> sig->min_flt += tsk->min_flt;
>> sig->maj_flt += tsk->maj_flt;
>> sig->nvcsw += tsk->nvcsw;
>> sig->nivcsw += tsk->nivcsw;
>> sig->inblock += task_io_get_inblock(tsk);
>> sig->oublock += task_io_get_oublock(tsk);
>> task_io_accounting_add(&sig->ioac, &tsk->ioac);
>> sig->sum_sched_runtime += tsk->se.sum_exec_runtime;
>
> Yes.

Let me give that a try and see what happens :)

>> How does that square with wait_task_zombie reaping the
>> statistics of the whole group with thread_group_cputime_adjusted()
>> when the group leader is exiting?
>
> Again, not sure I understand... thread_group_cputime_adjusted() in
> wait_task_zombie() is fine in any case. Nobody but us can reap this
> zombie.
>
> It seems that we misunderstood each other, let me try again. Just to
> simplify, suppose we have, say,
>
> sys_times_by_pid(pid, ...)
> {
> rcu_read_lock();
> task = find_task_by_vpid(pid);
> if (task)
> get_task_struct(task);
> rcu_read_unlock();
>
> if (!task)
> return -ESRCH;
>
> thread_group_cputime(task, ...);
> copy_to_user();
> return 0;
> }
>
> Note that this task can exit right after rcu_read_unlock(), and it can
> be also reaped (by its parent or by itself) and removed from the thread
> list. In this case for_each_thread() will see no threads, and thus it
> will only read task->signal->*time.
>
> This means that sys_times_by_pid() can simply return the wrong result
> instead of failure. Say, It can even return "all zeros" if this task was
> single-threaded.

Ahh, that makes sense.