Re: keventd_create_kthread

Re: keventd_create_kthread

[Rusty Russell]

In message <20040218004648.7471bb37.akpm@osdl.org> you write:
> wait_task_inactive() will return due to the preemption?
> 
> perhaps wait_task_inactive() should wait until the target task leaves state
> TASK_RUNNING.

That's not enough: it can set that and then get preemted.  It really
want to return when the task is off the runqueue.  The original
wait_task_inactive() does an incredible complicated and AFAICT useless
dance wrt not locking and disabling preempt explicitly.  Ingo, how's
this replacement?  (And who wrote this code?)

/*
 * wait_task_inactive - wait for a thread to unschedule.
 *
 * The caller must ensure that the task *will* unschedule sometime soon,
 * else this function might spin for a *long* time. This function can't
 * be called with interrupts off, or it may introduce deadlock with
 * smp_call_function() if an IPI is sent by the same process we are
 * waiting to become inactive.
 */
void wait_task_inactive(task_t * p)
{
	unsigned long flags;
	runqueue_t *rq;

repeat:
	rq = task_rq_lock(p, &flags);
	/* Must be off runqueue entirely, not preempted. */
	if (unlikely(p->array)) {
		task_rq_unlock(rq, &flags);
		cpu_relax();
		/* If it's preempted: yield.  It could be a while. */
		if (!task_running(p))
			yield();
		goto repeat;
	}
	task_rq_unlock(rq, &flags);
}

Untested BTW.

Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: keventd_create_kthread

[Ingo Molnar]

On Thu, 19 Feb 2004, Rusty Russell wrote:

> That's not enough: it can set that and then get preemted.  It really
> want to return when the task is off the runqueue.  The original
> wait_task_inactive() does an incredible complicated and AFAICT useless
> dance wrt not locking and disabling preempt explicitly.  Ingo, how's
> this replacement?  (And who wrote this code?)

this is old code that morphed many times. Its main use was for exit.c's
purpose and in heavy clone/exit workloads it made quite a difference
whether the 'polling' for task exit was done under the runqueue lock or
not - hence the complexity. Task freeing is poll-free in 2.6 so
wait_task_inactive() doesnt get nearly as heavy use.

The current wait_task_inactive() code seems to be OK on x86.
Context-switching cannot be preempted. The goal of wait_task_inactive() is
to wait for the task to unschedule on a CPU. If that's due to preempt then
it's due to preempt.

i'd strongly advise against using wait_task_inactive() in
keventd_create_kthread() - it's _polling_. We must not do any polling like
that in any modern interface. Why does keventd_create_kthread() need
wait_task_inactive()?

	Ingo

Re: keventd_create_kthread

[Andrew Morton]

Ingo Molnar <mingo@redhat.com> wrote:
>
>  i'd strongly advise against using wait_task_inactive() in
>  keventd_create_kthread() - it's _polling_. We must not do any polling like
>  that in any modern interface. Why does keventd_create_kthread() need
>  wait_task_inactive()?

The way it's designed, we _have_ to wait until the new kthread has gone to
sleep, because we poke him again with wake_up_process().

However, if that wake_up_process() comes too early we'll just flip the new
thread out of TASK_INTERUPTIBLE into TASK_RUNNING and the schedule() in
kthread() will fall straight through.  So perhaps we can simply remove the
wait_task_inactive()?

Re: keventd_create_kthread

[Ingo Molnar]

On Thu, 19 Feb 2004, Andrew Morton wrote:

> >  i'd strongly advise against using wait_task_inactive() in
> >  keventd_create_kthread() - it's _polling_. We must not do any polling like
> >  that in any modern interface. Why does keventd_create_kthread() need
> >  wait_task_inactive()?
> 
> The way it's designed, we _have_ to wait until the new kthread has gone
> to sleep, because we poke him again with wake_up_process().
> 
> However, if that wake_up_process() comes too early we'll just flip the
> new thread out of TASK_INTERUPTIBLE into TASK_RUNNING and the schedule()
> in kthread() will fall straight through.  So perhaps we can simply
> remove the wait_task_inactive()?

yep. There's almost never any good reason to use wait_task_inactive().

The only excusable special case is ptrace: there are some inherent
assumptions in the ptrace framework that need the task to unschedule at
least once before the parent can modify the user state. (eg. on x86 the
lazy FPU state and the fs/gs selectors need to be saved before the parent
can read/write them, plus changed debug registers need a real
context-switch to take effect, etc.)

	Ingo

Re: keventd_create_kthread

[Srivatsa Vaddagiri]

On Thu, Feb 19, 2004 at 08:12:19AM +0000, Andrew Morton wrote:
> However, if that wake_up_process() comes too early we'll just flip the new
> thread out of TASK_INTERUPTIBLE into TASK_RUNNING and the schedule() in
> kthread() will fall straight through.  So perhaps we can simply remove the
> wait_task_inactive()?

If wake_up_process() comes too early (when the target task is still
in TASK_RUNNING state), then won't wake_up_process() be a no-op?
In which case, the target kthread will miss a wake-up event 
(kthread_start/kthread_stop)?

-- 


Thanks and Regards,
Srivatsa Vaddagiri,
Linux Technology Center,
IBM Software Labs,
Bangalore, INDIA - 560017

Re: keventd_create_kthread

[Andrew Morton]

Srivatsa Vaddagiri <vatsa@in.ibm.com> wrote:
>
> On Thu, Feb 19, 2004 at 08:12:19AM +0000, Andrew Morton wrote:
> > However, if that wake_up_process() comes too early we'll just flip the new
> > thread out of TASK_INTERUPTIBLE into TASK_RUNNING and the schedule() in
> > kthread() will fall straight through.  So perhaps we can simply remove the
> > wait_task_inactive()?
> 
> If wake_up_process() comes too early (when the target task is still
> in TASK_RUNNING state), then won't wake_up_process() be a no-op?
> In which case, the target kthread will miss a wake-up event 
> (kthread_start/kthread_stop)?

No, that's OK - the new kernel thread sets TASK_INTERRUPTIBLE before waking
the invoking thread via complete():

new thread:

	/* OK, tell user we're spawned, wait for stop or wakeup */
	__set_current_state(TASK_INTERRUPTIBLE);
	complete(&create->started);
	schedule();

invoker:

	wait_for_completion(&create->started);
	create->result = find_task_by_pid(pid);
	wait_task_inactive(create->result);

It's the window after the complete() and before the schedule() where
wait_task_inactive() is spinning.

Re: keventd_create_kthread

[Rusty Russell]

In message <Pine.LNX.4.58.0402190205040.16515@devserv.devel.redhat.com> you write:
> The current wait_task_inactive() code seems to be OK on x86.
> Context-switching cannot be preempted. The goal of wait_task_inactive() is
> to wait for the task to unschedule on a CPU. If that's due to preempt then
> it's due to preempt.

No, because it can come back at any time 8(

> that in any modern interface. Why does keventd_create_kthread() need
> wait_task_inactive()?

Um, the code was taken from sched.c to kthread:

2.6.3 migration_thread():
	set_current_state(TASK_UNINTERRUPTIBLE);
	schedule();

2.6.3 migration_call():
		kernel_thread(migration_thread, &startup, CLONE_KERNEL);
		wait_for_completion(&startup.startup_done);
		wait_task_inactive(startup.task);

		startup.task->thread_info->cpu = cpu;
		startup.task->cpus_allowed = cpumask_of_cpu(cpu);


So, if the migration thread has been preempted immediately before
schedule(), wait_task_inactive returns, but it can come back from
preempt while we're messing with startup.task->thread_info->cpu.

Now, the latter part is wrapped in kthread_bind(), which should really
be doing the wait_task_inactive itself (doing it in kthread_create is
overzealous).  But the race is still there.

Hope that clarifies,
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.