[PATCH 0/2] kthread: kill task_get_live_kthread()

[PATCH 0/2] kthread: kill task_get_live_kthread()

[Oleg Nesterov]

Hello.

Imho, task_get_live_kthread() is very confusing and unneeded.
2a1d4460 copied get_task_struct() + "vfork_done != NULL" from
kthread_stop(), but only kthread_stop() needs them both.

It needs get_task_struct() because kthread_stop() can be used
when the caller doesn't have a reference but we know that this
thread can't exit itself.

At the same time, if it can exit we do not need get_task_struct()
(the caller must have a reference) but we need to ensure we do not
use to_kthread(NULL) if it has exited.

I think that kthread_park/unpark can simply use to_kthread(), but
this series only removes get_task_struct() and keeps "alive" check.



But the actual reason for this cleanup is that I do not understand
why park/unpark abuse kthread.c.

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.

Or this would be wrong/undesirable by some reason?

Oleg.

[PATCH 1/2] kthread: introduce to_live_kthread()

[Oleg Nesterov]

"k->vfork_done != NULL" with a barrier() after to_kthread(k) in
task_get_live_kthread(k) looks unclear, and sub-optimal because
we load ->vfork_done twice.

All we need is to ensure that we do not return to_kthread(NULL).
Add a new trivial helper which loads/checks ->vfork_done once,
this also looks more understandable.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/kthread.c |   26 ++++++++++++++++----------
 1 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 691dc2e..fa4a013 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -52,8 +52,21 @@ enum KTHREAD_BITS {
 	KTHREAD_IS_PARKED,
 };
 
-#define to_kthread(tsk)	\
-	container_of((tsk)->vfork_done, struct kthread, exited)
+#define __to_kthread(vfork)	\
+	container_of(vfork, struct kthread, exited)
+
+static inline struct kthread *to_kthread(struct task_struct *k)
+{
+	return __to_kthread(k->vfork_done);
+}
+
+static struct kthread *to_live_kthread(struct task_struct *k)
+{
+	struct completion *vfork = ACCESS_ONCE(k->vfork_done);
+	if (likely(vfork))
+		return __to_kthread(vfork);
+	return NULL;
+}
 
 /**
  * kthread_should_stop - should this kthread return now?
@@ -313,15 +326,8 @@ struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
 
 static struct kthread *task_get_live_kthread(struct task_struct *k)
 {
-	struct kthread *kthread;
-
 	get_task_struct(k);
-	kthread = to_kthread(k);
-	/* It might have exited */
-	barrier();
-	if (k->vfork_done != NULL)
-		return kthread;
-	return NULL;
+	return to_live_kthread(k);
 }
 
 /**
-- 
1.5.5.1

[PATCH 2/2] kthread: kill task_get_live_kthread()

[Oleg Nesterov]

task_get_live_kthread() looks confusing and unneeded. It does
get_task_struct() but only kthread_stop() needs this, it can be
called even if the calller doesn't have a reference when we know
that this kthread can't exit until we do kthread_stop().

kthread_park() and kthread_unpark() do not need get_task_struct(),
the callers already have the reference. And it can not help if we
can race with the exiting kthread anyway, kthread_park() can hang
forever in this case.

Change kthread_park() and kthread_unpark() to use to_live_kthread(),
change kthread_stop() to do get_task_struct() by hand and remove
task_get_live_kthread().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/kthread.c |   20 +++++++-------------
 1 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index fa4a013..d345f4e 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -324,12 +324,6 @@ struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
 	return p;
 }
 
-static struct kthread *task_get_live_kthread(struct task_struct *k)
-{
-	get_task_struct(k);
-	return to_live_kthread(k);
-}
-
 /**
  * kthread_unpark - unpark a thread created by kthread_create().
  * @k:		thread created by kthread_create().
@@ -340,7 +334,7 @@ static struct kthread *task_get_live_kthread(struct task_struct *k)
  */
 void kthread_unpark(struct task_struct *k)
 {
-	struct kthread *kthread = task_get_live_kthread(k);
+	struct kthread *kthread = to_live_kthread(k);
 
 	if (kthread) {
 		clear_bit(KTHREAD_SHOULD_PARK, &kthread->flags);
@@ -356,7 +350,6 @@ void kthread_unpark(struct task_struct *k)
 			wake_up_process(k);
 		}
 	}
-	put_task_struct(k);
 }
 
 /**
@@ -373,7 +366,7 @@ void kthread_unpark(struct task_struct *k)
  */
 int kthread_park(struct task_struct *k)
 {
-	struct kthread *kthread = task_get_live_kthread(k);
+	struct kthread *kthread = to_live_kthread(k);
 	int ret = -ENOSYS;
 
 	if (kthread) {
@@ -386,7 +379,6 @@ int kthread_park(struct task_struct *k)
 		}
 		ret = 0;
 	}
-	put_task_struct(k);
 	return ret;
 }
 
@@ -407,10 +399,13 @@ int kthread_park(struct task_struct *k)
  */
 int kthread_stop(struct task_struct *k)
 {
-	struct kthread *kthread = task_get_live_kthread(k);
+	struct kthread *kthread;
 	int ret;
 
 	trace_sched_kthread_stop(k);
+
+	get_task_struct(k);
+	kthread = to_live_kthread(k);
 	if (kthread) {
 		set_bit(KTHREAD_SHOULD_STOP, &kthread->flags);
 		clear_bit(KTHREAD_SHOULD_PARK, &kthread->flags);
@@ -418,10 +413,9 @@ int kthread_stop(struct task_struct *k)
 		wait_for_completion(&kthread->exited);
 	}
 	ret = k->exit_code;
-
 	put_task_struct(k);
-	trace_sched_kthread_stop_ret(ret);
 
+	trace_sched_kthread_stop_ret(ret);
 	return ret;
 }
 EXPORT_SYMBOL(kthread_stop);
-- 
1.5.5.1

Re: [PATCH 0/2] kthread: kill task_get_live_kthread()

[Thomas Gleixner]

Oleg,

On Mon, 11 Mar 2013, Oleg Nesterov wrote:
> Imho, task_get_live_kthread() is very confusing and unneeded.
> 2a1d4460 copied get_task_struct() + "vfork_done != NULL" from
> kthread_stop(), but only kthread_stop() needs them both.
> 
> It needs get_task_struct() because kthread_stop() can be used
> when the caller doesn't have a reference but we know that this
> thread can't exit itself.
> 
> At the same time, if it can exit we do not need get_task_struct()
> (the caller must have a reference) but we need to ensure we do not
> use to_kthread(NULL) if it has exited.
> 
> I think that kthread_park/unpark can simply use to_kthread(), but
> this series only removes get_task_struct() and keeps "alive" check.
>
> 
> But the actual reason for this cleanup is that I do not understand
> why park/unpark abuse kthread.c.

It's not abusing it :)
 
> 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 ...
 
> Or this would be wrong/undesirable by some reason?

Need to think about it, but probably it was just either copy and paste
stupidity or some form of paranoia.

What's the issue with that, other than some superflous task_get/put
calls ?

Thanks,

	tglx

Re: [PATCH 0/2] kthread: kill task_get_live_kthread()

[Oleg Nesterov]

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.

Re: [PATCH 0/2] kthread: kill task_get_live_kthread()

[Thomas Gleixner]

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