Messages in this thread | | | Date | Sat, 15 Jun 2013 19:29:59 +0200 | From | Oleg Nesterov <> | Subject | [PATCH 0/3] (Was: fput: task_work_add() can fail if the caller has passed exit_task_work()) |
| |
On 06/14, Andrew Morton wrote: > > On Fri, 14 Jun 2013 21:09:47 +0200 Oleg Nesterov <oleg@redhat.com> wrote: > > > + 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!
> This is particularly needed > because kernel/task_work.c is 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.
> > + 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.
| |