[PATCH] kthread: Don't looking for a task in create_kthread() #2

[PATCH] kthread: Don't looking for a task in create_kthread() #2

[Vitaliy Gusev]

Remove the unnecessary find_task_by_pid_ns(). kthread() can just
use "current" to get the same result.

Signed-off-by: Vitaliy Gusev <vgusev@openvz.org>

---
 kernel/kthread.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 4fbc456..fd765f9 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -76,6 +76,7 @@ static int kthread(void *_create)
 
 	/* OK, tell user we're spawned, wait for stop or wakeup */
 	__set_current_state(TASK_UNINTERRUPTIBLE);
+	create->result = current;
 	complete(&create->started);
 	schedule();
 
@@ -101,9 +102,6 @@ static void create_kthread(struct kthread_create_info *create)
 	} else {
 		struct sched_param param = { .sched_priority = 0 };
 		wait_for_completion(&create->started);
-		read_lock(&tasklist_lock);
-		create->result = find_task_by_pid_ns(pid, &init_pid_ns);
-		read_unlock(&tasklist_lock);
 		/*
 		 * root may have changed our (kthreadd's) priority or CPU mask.
 		 * The kernel thread should not inherit these properties.

Re: [PATCH] kthread: Don't looking for a task in create_kthread() #2

[Oleg Nesterov]

On 01/20, Vitaliy Gusev wrote:
>
> Remove the unnecessary find_task_by_pid_ns(). kthread() can just
> use "current" to get the same result.

I think the patch is nice and correct.



BTW. Perhaps it also makes sense to move sched_setscheduler/etc code
from create_kthread() to kthread_create(), what do you think?

(of course, this needs another patch)

Oleg.

Re: [PATCH] kthread: Don't looking for a task in create_kthread() #2

[Vitaliy Gusev]

On 20 January 2009 15:06:42 Oleg Nesterov wrote:
> On 01/20, Vitaliy Gusev wrote:
> >
> > Remove the unnecessary find_task_by_pid_ns(). kthread() can just
> > use "current" to get the same result.
> 
> I think the patch is nice and correct.
> 
> 
> 
> BTW. Perhaps it also makes sense to move sched_setscheduler/etc code
> from create_kthread() to kthread_create(), what do you think?

I don't thinks so. create_kthread() is executed in kthreadd process and 
sched_setscheduler/etc preparation things are reflected to kthreadd's CPU usage.
In other words it is a kthreadd matter, start and setup a thread.

> 
> (of course, this needs another patch)
> 
> Oleg.
> 
> 



-- 
Thank,
Vitaliy Gusev

Re: [PATCH] kthread: Don't looking for a task in create_kthread() #2

[Oleg Nesterov]

On 01/20, Vitaliy Gusev wrote:
>
> On 20 January 2009 15:06:42 Oleg Nesterov wrote:
> >
> > BTW. Perhaps it also makes sense to move sched_setscheduler/etc code
> > from create_kthread() to kthread_create(), what do you think?
>
> I don't thinks so. create_kthread() is executed in kthreadd process and
> sched_setscheduler/etc preparation things are reflected to kthreadd's CPU usage.
> In other words it is a kthreadd matter, start and setup a thread.

can't understand...

Perhaps I missed something, but from the correctness pov it doesn't
matter who does sched_setscheduler/etc (except _nocheck() would be
better). However it does matter from the scalability pov, we should
move as much as possible from create_kthread() because we have a
single process with executes the "create" requests.

OK, please forget. This reminds me kthread.c needs a major rework
anyway, hopefully I'll try to do this soon.

Oleg.

Re: [PATCH] kthread: Don't looking for a task in create_kthread() #2

[Vitaliy Gusev]

On 20 January 2009 18:00:19 Oleg Nesterov wrote:
> On 01/20, Vitaliy Gusev wrote:
> >
> > On 20 January 2009 15:06:42 Oleg Nesterov wrote:
> > >
> > > BTW. Perhaps it also makes sense to move sched_setscheduler/etc code
> > > from create_kthread() to kthread_create(), what do you think?
> >
> > I don't thinks so. create_kthread() is executed in kthreadd process and
> > sched_setscheduler/etc preparation things are reflected to kthreadd's CPU usage.
> > In other words it is a kthreadd matter, start and setup a thread.
> 
> can't understand...
> 
> Perhaps I missed something, but from the correctness pov it doesn't
> matter who does sched_setscheduler/etc (except _nocheck() would be
> better). However it does matter from the scalability pov, we should
> move as much as possible from create_kthread() because we have a
> single process with executes the "create" requests.

Ok, I understand you. You want to decrease kthreadd latency. In this case
your suggestion is right.

> 
> OK, please forget. This reminds me kthread.c needs a major rework
> anyway, hopefully I'll try to do this soon.

I also have intent to make kthread_run/kthread_create more easy used for
threads that exit himself on error.
For example, we create some thread THR, which does something but exits on error.
So in normal case we need to call kthread_stop() to stop thread THR, but on
error we don't. This is a problem, because we can't simple (without race) 
determine that THR was exited.
Simple workaround - do a sleeping loop in thread THR and don't exit until kthread_should_stop.
But it isn't a good for code reading/writing.

So my suggestion is: Move workaround sleeping logic from THR to low level
functions:
   wait for a kthread_should_stop in kthread() if THR was exited.
Of course don't wait if nobody cares about thread THR (nobody calls kthread_stop()
for this thread).

That do you think about it?

> 
> Oleg.
> 


-- 
Thank,
Vitaliy Gusev

Re: [PATCH] kthread: Don't looking for a task in create_kthread() #2

[Oleg Nesterov]

On 01/20, Vitaliy Gusev wrote:
>
> Ok, I understand you. You want to decrease kthreadd latency. In this case
> your suggestion is right.

Please feel free to send the patch then.

> > OK, please forget. This reminds me kthread.c needs a major rework
> > anyway, hopefully I'll try to do this soon.
>
> I also have intent to make kthread_run/kthread_create more easy used for
> threads that exit himself on error.
> For example, we create some thread THR, which does something but exits on error.
> So in normal case we need to call kthread_stop() to stop thread THR, but on
> error we don't. This is a problem, because we can't simple (without race) 
> determine that THR was exited.

This is only one of the problems,

> Simple workaround - do a sleeping loop in thread THR and don't exit until kthread_should_stop.
> But it isn't a good for code reading/writing.
>
> So my suggestion is: Move workaround sleeping logic from THR to low level
> functions:
>    wait for a kthread_should_stop in kthread() if THR was exited.

I think it is better to improve kthread_stop() so it can be used even
if kthread has already exited, this is not that hard.

But there are other problems, I'll try to provide mor info later.

Oleg.