(Was: fput: task_work_add() can fail if the caller has passed exit_task_work())
Get series with b4
b4 am [email protected]Versions newest first
- v1 Latest3 patches 1 replyIn review
Patches in v1 No applies recorded yet
| # | Subject and files | Size | Replies | Applied |
|---|---|---|---|---|
| 1/3 | fput: turn "list_head delayed_fput_list" into llist_headfs/ include/ | +12 | 0 replies | |
| 2/3 | llist: fix/simplify llist_add() and llist_add_batch()include/ lib/ | +12 | 0 replies | |
| 3/3 | llist: llist_add() can use llist_add_batch()include/ | +4 | 0 replies |
Reviews trailers per patch
No Reviewed-by, Acked-by or Tested-by trailers for v1 yet.
Cover letter [PATCH 0/3] (Was: fput: task_work_add() can fail if the caller has passed exit_task_work())
Quoted 10 lines · Andrew Morton
Review
Quoted 4 lines · On Fri, 14 Jun 2013 21:09:47 +0200 Oleg Nesterov <[email protected]> wrote:
(quoted diff) +4
-0+ if (likely(!in_interrupt() && !(task->flags & PF_KTHREAD))) { + init_task_work(&file->f_u.fu_rcuhead, ____fput); + if (!task_work_add(task, &file->f_u.fu_rcuhead, true)) + return;A comment here would be useful, explaining the circumstances under which we fall through to the delayed fput.
Thanks!
Quoted 2 lines
This is particularly needed
becausekernel/task_work.cis such undocumented crap.
It seems that you are trying to force me to make the doc patch ;) OK, I'll try. task_work.c needs a couple of cosmetic cleanups anyway.
Quoted 7 lines
Review
Quoted 4 lines
(quoted diff) +4
-0+ spin_lock_irqsave(&delayed_fput_lock, flags); + list_add(&file->f_u.fu_list, &delayed_fput_list); + schedule_work(&delayed_fput_work); + spin_unlock_irqrestore(&delayed_fput_lock, flags);OT: I don't think that
schedule_work()needs to be inside the locked region. Scalability improvements beckon!
Yeees, I thought about this too.
Performance-wise this can't really help, this case is unlikely. But I think this change makes this code a bit simpler, so please see 1/3.
2/3 fixes the (theoretical) bug in llist_add() and imho cleanups the code.
3/3 comes as a separate change because I do not want to argue if someone dislike the non-inline llist_add(). But once again, we can make llist_add_batch() inline, and I believe it is never good to duplicate the code even if it is simple.
Oleg.