[lkml]   [2015]   [Apr]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
Patch in this message
SubjectRe: [PATCH] exit: Use read lock for do_notify_parent() instead of write lock

I'll try to read this patch tomorrow, currently I am hopelessly buried
in user-space programming :/

But I have to admit that so far I dislike this patch very much. It adds
a lot of complications and for what?

Yes, yes, yes. tasklist_lock is another BKL and must die. We need the
per-process lock. Until then I do not think the hacks like this make
any sense, unless you have the "real" workload with before/after
performance numbers.

On 04/09, Kirill Tkhai wrote:
> I suggest to execute do_notify_parent() under read_lock(). It allows more tasks
> to use it in parallel. Read lock gives enough guarantees for us: child's parent
> won't change during the notification.

Well, write_unlock() + read_lock() is not nice too...

> include/asm-generic/qrwlock.h:
> static inline void queue_reduce_locked_write_to_read(struct qrwlock *lock)
> {
> smp_mb__before_atomic();
> atomic_add(_QR_BIAS - _QW_LOCKED, &lock->cnts);
> }

Yes, downgrade() will be better.

Still, this only removes do_notify_parent() from the write_lock'ed section.

(lets ignore kill_orphaned_pgrp(), we want to make will_become_orphaned_pgrp
lockless. Look at get_signal).

And this changes the rules: currently ->exit_state is stable under read_lock,
except -> EXIT_DEAD transition. OK, this is probably fine, but we need to
recheck. At least this was certainly wrong some time before iirc.

> @@ -594,7 +597,10 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
> write_lock_irq(&tasklist_lock);
> forget_original_parent(tsk, &dead);
> + tsk->exit_state = EXIT_NOTIFY;
> + write_unlock_irq(&tasklist_lock);

And unless I missed something this EXIT_NOTIFY turns the concurrent
do_wait() into the busy-wait loop.

Now suppose that CONFIG_SMP=n and the rt parent preempts the exiting
child right after it drops tasklist: deadlock?

> + read_lock(&tasklist_lock);
> if (group_dead)
> kill_orphaned_pgrp(tsk->group_leader, NULL);
> @@ -612,13 +618,14 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
> }
> tsk->exit_state = autoreap ? EXIT_DEAD : EXIT_ZOMBIE;

This needs WRITE_ONCE(). Otherwise gcc can do, say,

tsk->exit_state = EXIT_ZOMBIE;
if (autoreap)
tsk->exit_state = EXIT_DEAD;

which will lead to kernel crash (both parent and child can release this

> - if (tsk->exit_state == EXIT_DEAD)
> + smp_wmb(); /* Pairs with read_lock() in do_wait() */

Why? this barries looks unnecessary.

OTOH. We need to set EXIT_XXX before __wake_up_parent(). OK, OK, we do not
because of the busy-wait loop, but busy-wait is not an option.

> @@ -1317,6 +1324,13 @@ static int wait_consider_task(struct wait_opts *wo, int ptrace,
> return 0;
> }
> + if (unlikely(exit_state == EXIT_NOTIFY)) {
> + if (wo->wo_flags & WNOHANG)
> + return 0;
> + read_unlock(&tasklist_lock);
> + return -REPEAT_DOWAIT;
> + }

No, no, no. If you do something like this, please (ab)use wo->notask_error.
And wait_consider_task() should continue after that,


 \ /
  Last update: 2015-04-10 20:21    [W:0.077 / U:5.512 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site