Messages in this thread | | | Date | Tue, 12 Mar 2013 18:04:24 +0100 | From | Oleg Nesterov <> | Subject | Re: [PATCH 0/2] kthread: kill task_get_live_kthread() |
| |
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.
> > 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.
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.
Oleg.
| |