linux-kernel - Re: [PATCH 0/2] kthread: kill task_get_live

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] [day] [month] [year] [list]

Message-ID: <alpine.LFD.2.02.1303121914010.22263@ionos>
Date:	Tue, 12 Mar 2013 19:18:35 +0100 (CET)
From:	Thomas Gleixner <tglx@...utronix.de>
To:	Oleg Nesterov <oleg@...hat.com>
cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Namhyung Kim <namhyung@...nel.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Rusty Russell <rusty@...tcorp.com.au>,
	"Srivatsa S. Bhat" <srivatsa.bhat@...ux.vnet.ibm.com>,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/2] kthread: kill task_get_live_kthread()

On Tue, 12 Mar 2013, Oleg Nesterov wrote:

> Hi Thomas,
> 
> On 03/11, Thomas Gleixner wrote:
> >
> > On Mon, 11 Mar 2013, Oleg Nesterov wrote:
> > >
> > > But the actual reason for this cleanup is that I do not understand
> > > why park/unpark abuse kthread.c.
> >
> > It's not abusing it :)
> 
> Yes, yes, I didn't mean the code looks bad or something like this.
> 
> Just I thought that, perhaps, it would be more clean to hide this
> park/unpark logic in kernel/smpboot.c and do not add the "special"
> new members into "struct kthread".
> 
> But let me repeat, mostly I simply wanted to ask the question. I
> just noticed this new code and I was curious if this park/unpark
> logic should be applied to every kthread (in future) or it is only
> for smpboot_register_percpu_thread/etc.

It was written to avoid the continous teardown/setup of per cpu
threads in the notifiers. That was a) racy and b) a total waste of
time.
 
> > > Thomas, can't we move kthread->parked/cpu to smpboot_thread_data
> > > and move all this code into kernel/smpboot.c? Just for example,
> > > why kthread() does __kthread_parkme() ? smpboot_thread_fn() can do
> > > this at the start.
> >
> > No objection. When I implemented this, I thought this would be the
> > correct place and I followed the conventions of kthread.c ...
> 
> OK, I'll try to think again if this change is actually possible _and_
> it can really make the things more clean/simple.
> 
> > What's the issue with that, other than some superflous task_get/put
> > calls ?
> 
> Do you mean this particular cleanup?
> 
> No issues, this is only cleanup. But every cleanup is subjective, so
> please tell me if you disagree.

No objections as long as it gets cleaner and simpler and works :)
 
> Firstly, to_kthread() + barrier() + "vfork_done != NULL" doesn't look
> very clear (cough, yes, this was written by me). And after 1/2

:)
 
> 	static struct kthread *task_get_live_kthread(struct task_struct *k)
> 	{
> 		get_task_struct(k);
> 		return to_live_kthread(k);
> 	}
> 
> looks confusing too because it mixes 2 different things and because
> its usage is not clear. I mean, it is not clear why the caller needs
> get_task_struct() and why it is safe if we do not have a reference.

True. And in the case of the smpboot threads we actually take a ref on
the task struct in the create function.

Thanks,

	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/