Re: [Patch][RFC] Disabling per-tgid stats on task exit in taskstats

Jay Lan
2006-06-26T17:33:17Z

Shailabh Nagar wrote:
> Andrew Morton wrote:
>
>> On Fri, 23 Jun 2006 22:59:04 -0400
>> Shailabh Nagar <nagar@watson.ibm.com> wrote:
>>
>>
>>
>>>>> It was due to a loop in fill_tgid() when per-TG stats
>>>>> data are assembled for netlink:
>>>>> do {
>>>>> rc = delayacct_add_tsk(stats, tsk);
>>>>> if (rc)
>>>>> break;
>>>>>
>>>>> } while_each_thread(first, tsk);
>>>>>
>>>>> and it is executed inside a lock.
>>>>> Fortunately single threaded appls do not hit this code.
>>>>>
>>>>>
>>>>
>>>> Am I reading this right? We do that loop when each thread within the
>>>> thread group exits?
>>>>
>>>>
>>>
>>> Yes.
>>>
>>>
>>>
>>>> How come?
>>>>
>>>>
>>>>
>>>
>>> To get the sum of all per-tid data for threads that are currently alive.
>>> This is returned to userspace with each thread exit.
>>>
>>
>>
>> I realise that. How about we stop doing it?
>>
>> When a thread exits it only makes sense to send up the stats for that
>> thread.
>
>
>> Why does the kernel assume that userspace is also interested in
>> the accumulated stats of its siblings? And if userspace _is_
>> interested in
>> that info, it's still present in-kernel and can be queried for.
>>
>>
> The reason for sending out sum of siblings's stats was as follows:
> I didn't maintain a per-tgid data structure in-kernel where the exiting
> threads taskstats could be accumalated
> , erroneously thinking that this would require such a structure to be
> *additionally* updated each time a statististic
> was being collected and that would be way too much overhead. Also to
> save on space. Thus if userspace wants to get the per-tgid stats for the
> thread group when the last thread exits, then it cannot
> do so by querying since such a query only returns the sum of currently
> live threads (data from exited threads is lost).
>
> So, the current design chooses to return the sum of all siblings + self
> when each thread exits. Using this userspace
> can maintain the per-tgid data for all currently living threads of the
> group + previously exited threads.
>
> But as pointed out in an earlier mail, it looks like this is
> unnecessarily elaborate way of trying to avoid maintaining
> a separate per-tgid data structure in the kernel (in addition to the
> per-tid ones we already have).
>
> What can be done is to create a taskstats structure for a thread group
> the moment the *second* thread gets created.
> Then each exiting thread can accumalate its stats to this struct. If
> userspace queries for per-tgid data, the sum of all
> live threads + value in this struct can be returned. And when the last
> thread of the thread group exits, the struct's
> value can be output.
>
> While this will mean an extra taskstats structure hanging around for the
> lifetime of a multithreaded app (not single threaded
> ones), it should cut down on the overhead of running through all threads
> that we see in the current design.
> More importantly, it will reduce the frequency of per-tgid data send to
> once for each thread group exit instead of once
> per thread exit.
>
> Will that work for everyone ?

As long as the per-pid delayacct struct has a pointer to the per-tgid
data struct and deoes not need to go through the loop on every exit.

>
>>>> Is there some better lock we can use in there? It only has to be
>>>> threadgroup-wide rather than kernel-wide.
>>>>
>>>>
>>>>
>>>
>>> The lock we're holding is the tasklist_lock. To go through all the
>>> threads of a thread group
>>> thats the only lock that can protect integrity of while_each_thread
>>> afaics.
>>>
>>
>>
>> At present, yes. That's persumably not impossible to fix.
>>
>>
> In the above design, if a userspace query for per-tgid data arrives,
> then I'll still need to run through
> all the threads of a thread group (to return their sum + that of already
> exited threads accumalated in the
> extra per-tgid taskstats struct).

But, this query-reply logic can be separated from that executed at
exit.

Thanks,
- jay

>
> So that could still benefit from such a thread group specific lock.
> Scope of change is a bit more of course
> so will need to take a closer look.
>
> --Shailabh