Messages in this thread | | | Date | Tue, 12 Mar 2013 19:18:35 +0100 (CET) | From | Thomas Gleixner <> | 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
| |