Re: [Patch 4/7] Add sysctl for delay accounting

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

Arjan van de Ven wrote:

>On Mon, 2006-02-27 at 03:38 -0500, Shailabh Nagar wrote:
>  
>
>>Arjan van de Ven wrote:
>>
>>    
>>
>>>>+/* Allocate task_delay_info for all tasks without one */
>>>>+static int alloc_delays(void)
>>>>   
>>>>
>>>>        
>>>>
>>>I'm sorry but this function seems to be highly horrible
>>> 
>>>
>>>      
>>>
>>Could you be more specific ? Is it the way its coded or the design 
>>(preallocate, then assign)
>>itself ?
>>
>>The function needs to allocate task_delay_info structs for all tasks 
>>that might
>>have been forked since the last time delay accounting was turned off.
>>Either we have to count how many such tasks there are, or preallocate
>>nr_tasks (as an upper bound) and then use as many as needed.
>>    
>>
>
>it generally feels really fragile, especially with the task enumeration
>going to RCU soon. (eg you'd lose the ability to lock out new task
>creation)
>  
>
>On first sight it looks a lot better to allocate these things on demand,
>but I'm not sure how the sleeping-allocation would interact with the
>places it'd need to be called...
>  
>
Yes, thats  the reason why we didn't do the on-demand allocation...the 
next time a task is checked
could be in any of the places where the timestamping is done. Doing the 
allocation there (and incurring
the extra cost of the check even when sysctl hasn't been used) didn't 
seem worthwhile, esp. when we
have a point (sysctl handler) where we can catch most of the allocs needed.

But if task enumeration is going to get more difficult, we'll need to 
keep the on-demand allocation (on
next use) as a backup for tasks that weren't caught during the sysctl 
change.


>
>  
>