Messages in this thread | | | Date | Thu, 14 Jun 2012 10:35:27 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [RFC patch 1/5] kthread: Implement park/unpark facility |
| |
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
| |