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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 14 Jun 2012 10:35:27 +0200 (CEST)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Namhyung Kim <namhyung@...nel.org>
cc:	LKML <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Ingo Molnar <mingo@...nel.org>,
	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>,
	Rusty Russell <rusty@...tcorp.com.au>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Tejun Heo <tj@...nel.org>
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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/