Re: [RFC patch 1/5] kthread: Implement park/unpark facility

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, 14 Jun 2012, Namhyung Kim wrote:
> >  /**
> > + * kthread_create_on_cpu - Create a cpu bound kthread
> > + * @threadfn: the function to run until signal_pending(current).
> > + * @data: data ptr for @threadfn.
> > + * @node: memory node number.
> 
> Should be @cpu.

Yup. Copy and paste should be forbidden :)

> > +				   cpu);
> > +	if (IS_ERR(p))
> > +		return p;
> > +	/* Park the thread, mark it percpu and then bind it */
> > +	kthread_park(p);
> 
> Why park? AFAIK the p is not running after creation, so binding it
> doesn't need parking, right?
 
Parking is necessary to get it out of the UNINTERRUPTIBLE state,
otherwise the hung task detector would complain. Right the binding is
not strictly necessary. In fact we can delay it to unpark() where we
know that the target cpu is online already.
 
> > +void kthread_unpark(struct task_struct *k)
> > +{
> > +	struct kthread *kthread;
> > +
> > +	get_task_struct(k);
> > +
> > +	kthread = to_kthread(k);
> > +	barrier(); /* it might have exited */
> > +	if (k->vfork_done != NULL) {
> 
> Does is guarantee it's not exited? If so, as it is used a couple of
> times, wouldn't it be better making it up a (static?) helper (with
> comments, hopefully) something like a kthread_is_alive() ?

Good point.

> > +/**
> >   * kthread_stop - stop a thread created by kthread_create().
> >   * @k: thread created by kthread_create().
> >   *
> > @@ -259,7 +398,7 @@ int kthread_stop(struct task_struct *k)
> >  	kthread = to_kthread(k);
> >  	barrier(); /* it might have exited */
> >  	if (k->vfork_done != NULL) {
> > -		kthread->should_stop = 1;
> > +		set_bit(KTHREAD_SHOULD_STOP, &kthread->flags);
> 
> I wonder whether it also needs to unpark @k. Isn't it possible to stop a
> parked thread?

Right, it should be possible and it is. Clearing the park flag is not
strictly necessary, but yes we should do that.

Thanks for the review!

       tglx