Re: [PATCH RFC] time,signal: protect resource use statistics with seqlock
From: Rik van Riel
Date: Thu Aug 14 2014 - 15:05:38 EST
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.
>>> 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);
>> 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;
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
> It seems that we misunderstood each other, let me try again. Just to
> simplify, suppose we have, say,
> sys_times_by_pid(pid, ...)
> task = find_task_by_vpid(pid);
> if (task)
> if (!task)
> return -ESRCH;
> thread_group_cputime(task, ...);
> 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
Ahh, that makes sense.
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/