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

[Shailabh Nagar <nagar@watson.ibm.com>]

Jay Lan wrote:

> 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 

It doesn't need to....the per-tgid thing is allocated inside tsk->signal.
Let me send the patch out and we can discuss the design/implementation.

> and deoes not need to go through the loop on every exit.

Yes. Thats not needed anymore.

>
>>
>>>>> 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.

Yes, it already is.

Thanks,
Shailabh