[prog] Unlocking task_list read lock before performing a sleepy operation

[Ariane van der Steldt]

On 03/14/13 11:56, Pranami Bhattacharya wrote:
> Dear All,
>
>
> I was trying the following and had a doubt.
>
>
> >From my kernel module I want to iterate over task list and do some sleepy
> operation for all the process for which my task->flag is set .I know by
> holding read_lock(tasklist_lock)/rcu_read_lock I should not be performing
> any sleepy operation. So I have coded like below :
>
>
>
> 1.  Take the read lock
>
> 2.  Iterate over task (for_each_process)
>
> 3.  If my flag is set I unlock the tasklist_lock and get_task_struct
>
> 4.  Perform the sleepy operation
>
> 5.  Then put_task_struct
>
> 6.  Again take read_lock(&tasklist_lock);
>
> 7.  Loop continues
>
>
>
> The code snippet is as shown below :
>
>
> read_lock(&tasklist_lock);
>
>
>
>   for_each_process(c) {
>
>
>
>    if (c->my_flag) {
>
>
>
>            read_unlock(&tasklist_lock);

Once you unlock the corresponding mutex the tasklist can be changed by 
another thread.
This means the for_each_process() iterator may be invalid by the time 
you reacquire the lock on the list.

>             get_task_struct(c);
>
>
>                      ...
>
>
>                      perform_sleepy_operation();
>
>
>                       ...
>
>
>
>              put_task_struct(c);
>
>
>
>               read_lock(&tasklist_lock);
>
>                   }
>
>
>
>           }
>
>
>
> Please let me know if this approach is correct.

A couple of solutions exist to combat this problem, depending on the 
environment they may or may not apply.

[a] prevent 'c' from being removed from the list for the duration of 
your call (a reference counter or lock specific to 'c' would suffice.
[b] restart from the beginning once you unlock; this may result in the 
algorithm never terminating, if there is enough activity on the list and 
may result in your operation being done multiple times on objects, if 
the flag is not cleared at all or is set again between iterations
[c] as [b], restart from the beginning, but use a generation counter to 
prevent multiple updates
[d] while holding the lock, gather each 'c' for which you need to do 
this operation into a list, all the while holding the lock.  Once that 
list is populated, operate on the entries in this list while not holding 
the lock.  Disadvantage is that you may have to allocate sufficient 
memory up-front and that you need a way to inform the code that this 
object may not go away (for instance, increment a reference counter 
while the object is on the list).

If [b] or [c] are possible, they can make for nicer solutions than [d], 
in terms of overhead and complexity, but the code will be rather 
complicated (reference counting in C is messy at the best of times).
[d] is the approach which should always work and is easiest to verify, 
at the cost that you may have to return ENOMEM to indicate insufficient 
temporary memory was available.
[a] is possible if you have reference counters, but in that case, 
ideally the 'for_each_process' macro should have reference counting 
semantics or alternative locking strategies to prevent your iterator 'c' 
from going away.

> Thanks and Regards,
>
> Pranami
-- 
Ariane