Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1511219 > unrolled thread

[PATCH 0/2] kthread: make struct kthread kmalloc'ed

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 0/2] kthread: make struct kthread kmalloc'ed Oleg Nesterov <oleg@redhat.com> - 2016-10-28 18:20 +0200
    [PATCH 1/2] kthread: make struct kthread kmalloc'ed Oleg Nesterov <oleg@redhat.com> - 2016-10-28 18:20 +0200
      Re: [PATCH 1/2] kthread: make struct kthread kmalloc'ed Thomas Gleixner <tglx@linutronix.de> - 2016-10-28 21:00 +0200
    Re: [PATCH 0/2] kthread: make struct kthread kmalloc'ed Thomas Gleixner <tglx@linutronix.de> - 2016-10-28 20:50 +0200
    [PATCH 0/2] kthread: kill to_live_kthread() Oleg Nesterov <oleg@redhat.com> - 2016-10-31 21:10 +0100
      [PATCH 1/2] kthread: don't use to_live_kthread() in kthread_stop() Oleg Nesterov <oleg@redhat.com> - 2016-10-31 21:10 +0100
        Re: [PATCH 1/2] kthread: don't use to_live_kthread() in  kthread_stop() Thomas Gleixner <tglx@linutronix.de> - 2016-11-09 09:10 +0100
      [PATCH 2/2] kthread: don't use to_live_kthread() in kthread_park()  and kthread_unpark() Oleg Nesterov <oleg@redhat.com> - 2016-10-31 21:10 +0100
        Re: [PATCH 2/2] kthread: don't use to_live_kthread() in kthread_park()  and kthread_unpark() Thomas Gleixner <tglx@linutronix.de> - 2016-11-09 09:50 +0100
          Re: [PATCH 2/2] kthread: don't use to_live_kthread() in         kthread_park() and kthread_unpark() Oleg Nesterov <oleg@redhat.com> - 2016-11-09 18:30 +0100
            [PATCH 1/1] kthread: don't abuse kthread_create_on_cpu() in  __kthread_create_worker() Oleg Nesterov <oleg@redhat.com> - 2016-11-10 18:30 +0100
            [PATCH 0/1] kthread: don't abuse kthread_create_on_cpu() in  __kthread_create_worker() Oleg Nesterov <oleg@redhat.com> - 2016-11-10 18:30 +0100
      Re: [PATCH 0/2] kthread: kill to_live_kthread() Andy Lutomirski <luto@amacapital.net> - 2016-11-07 19:40 +0100

#1511219 — [PATCH 0/2] kthread: make struct kthread kmalloc'ed

Sorry for delay, I was distracted...

On 10/26, Thomas Gleixner wrote:
>
> On Wed, 26 Oct 2016, Oleg Nesterov wrote:
> > On 10/26, Thomas Gleixner wrote:
> > >
> > > On Wed, 26 Oct 2016, Oleg Nesterov wrote:
> > > > +static inline void set_kthread_struct(void *kthread)
> > > > +{
> > > > +	/*
> > > > +	 * We abuse ->set_child_tid to avoid the new member and because it
> > > > +	 * can't be wrongly copied by copy_process(). We also rely on fact
> > > > +	 * that the caller can't exec, so PF_KTHREAD can't be cleared.
> > > > +	 */
> > > > +	current->set_child_tid = (__force void __user *)kthread;
> > >
> > > Can we pretty please avoid this type casting? We only have 5 places using
> > > set_child_tid. So we can really make it a proper union
> >
> > Yes, I thought about anonymous union too, the only problem is that
> > it will need more comments ;)
>
> Be careful with anonymous unions. There are a few pitfalls with older
> compilers. That's why I said make it a proper union and fixup the 5 usage
> sites.

Ah. Then I'd prefer to do this later or in a separate change, unless you
feel strongly. I certainly do not want to update other users at least right
now.

Yes, these 2 type casts do not look nice, but they are hidden in the trivial
helpers. And, for example, if something goes wrong we can trivially change
this code to use, say, sas_ss_sp. Just we need to update the comments to
explain why it is safe too.

Finally. I still hope we will kill struct kthread (I mean, unbloat it and
embed into task_struct), and this means that the proper union should touch
more members. Say, sas_* and/or vfork_done+set/clear_child_tid. I'd like
to do this only once if possible.


I'll try to kill to_live_kthread() tomorrow, didn't have time to do this
today.

Oleg.

[toc] | [next] | [standalone]

#1511221 — [PATCH 1/2] kthread: make struct kthread kmalloc'ed

I still think we should kill struct kthread in its current form, but this
needs cleanups outside of kthread.c.

So make it kmalloc'ed for now to avoid the problems with stack corruption,
for example the crashed kthread will likely OOPS again because its .exited
was destroyed by rewind_stack_do_exit().

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/kthread.h |  1 +
 kernel/fork.c           |  2 ++
 kernel/kthread.c        | 58 ++++++++++++++++++++++++++++++++++++++-----------
 3 files changed, 48 insertions(+), 13 deletions(-)

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index a6e82a6..c1c3e63 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -48,6 +48,7 @@ struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
 	__k;								   \
 })
 
+void free_kthread_struct(struct task_struct *k);
 void kthread_bind(struct task_struct *k, unsigned int cpu);
 void kthread_bind_mask(struct task_struct *k, const struct cpumask *mask);
 int kthread_stop(struct task_struct *k);
diff --git a/kernel/fork.c b/kernel/fork.c
index 623259f..663c6a7 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -351,6 +351,8 @@ void free_task(struct task_struct *tsk)
 	ftrace_graph_exit_task(tsk);
 	put_seccomp_filter(tsk);
 	arch_release_task_struct(tsk);
+	if (tsk->flags & PF_KTHREAD)
+		free_kthread_struct(tsk);
 	free_task_struct(tsk);
 }
 EXPORT_SYMBOL(free_task);
diff --git a/kernel/kthread.c b/kernel/kthread.c
index be2cc1f..9d64b65 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -53,14 +53,38 @@ enum KTHREAD_BITS {
 	KTHREAD_IS_PARKED,
 };
 
-#define __to_kthread(vfork)	\
-	container_of(vfork, struct kthread, exited)
+static inline void set_kthread_struct(void *kthread)
+{
+	/*
+	 * We abuse ->set_child_tid to avoid the new member and because it
+	 * can't be wrongly copied by copy_process(). We also rely on fact
+	 * that the caller can't exec, so PF_KTHREAD can't be cleared.
+	 */
+	current->set_child_tid = (__force void __user *)kthread;
+}
 
 static inline struct kthread *to_kthread(struct task_struct *k)
 {
-	return __to_kthread(k->vfork_done);
+	WARN_ON(!(k->flags & PF_KTHREAD));
+	return (__force void *)k->set_child_tid;
+}
+
+void free_kthread_struct(struct task_struct *k)
+{
+	/*
+	 * Can be NULL if this kthread was created by kernel_thread()
+	 * or if kmalloc() in kthread() failed.
+	 */
+	kfree(to_kthread(k));
 }
 
+#define __to_kthread(vfork)	\
+	container_of(vfork, struct kthread, exited)
+
+/*
+ * TODO: kill it and use to_kthread(). But we still need the users
+ * like kthread_stop() which has to sync with the exiting kthread.
+ */
 static struct kthread *to_live_kthread(struct task_struct *k)
 {
 	struct completion *vfork = ACCESS_ONCE(k->vfork_done);
@@ -181,14 +205,11 @@ static int kthread(void *_create)
 	int (*threadfn)(void *data) = create->threadfn;
 	void *data = create->data;
 	struct completion *done;
-	struct kthread self;
+	struct kthread *self;
 	int ret;
 
-	self.flags = 0;
-	self.data = data;
-	init_completion(&self.exited);
-	init_completion(&self.parked);
-	current->vfork_done = &self.exited;
+	self = kmalloc(sizeof(*self), GFP_KERNEL);
+	set_kthread_struct(self);
 
 	/* If user was SIGKILLed, I release the structure. */
 	done = xchg(&create->done, NULL);
@@ -196,6 +217,19 @@ static int kthread(void *_create)
 		kfree(create);
 		do_exit(-EINTR);
 	}
+
+	if (!self) {
+		create->result = ERR_PTR(-ENOMEM);
+		complete(done);
+		do_exit(-ENOMEM);
+	}
+
+	self->flags = 0;
+	self->data = data;
+	init_completion(&self->exited);
+	init_completion(&self->parked);
+	current->vfork_done = &self->exited;
+
 	/* OK, tell user we're spawned, wait for stop or wakeup */
 	__set_current_state(TASK_UNINTERRUPTIBLE);
 	create->result = current;
@@ -203,12 +237,10 @@ static int kthread(void *_create)
 	schedule();
 
 	ret = -EINTR;
-
-	if (!test_bit(KTHREAD_SHOULD_STOP, &self.flags)) {
-		__kthread_parkme(&self);
+	if (!test_bit(KTHREAD_SHOULD_STOP, &self->flags)) {
+		__kthread_parkme(self);
 		ret = threadfn(data);
 	}
-	/* we can't just return, we must preserve "self" on stack */
 	do_exit(ret);
 }
 
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1511355 — Re: [PATCH 1/2] kthread: make struct kthread kmalloc'ed

On Fri, 28 Oct 2016, Oleg Nesterov wrote:

> I still think we should kill struct kthread in its current form, but this
> needs cleanups outside of kthread.c.
> 
> So make it kmalloc'ed for now to avoid the problems with stack corruption,
> for example the crashed kthread will likely OOPS again because its .exited
> was destroyed by rewind_stack_do_exit().
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[toc] | [prev] | [next] | [standalone]

#1511348

On Fri, 28 Oct 2016, Oleg Nesterov wrote:
> On 10/26, Thomas Gleixner wrote:
> 
> > Be careful with anonymous unions. There are a few pitfalls with older
> > compilers. That's why I said make it a proper union and fixup the 5 usage
> > sites.
> 
> Ah. Then I'd prefer to do this later or in a separate change, unless you
> feel strongly. I certainly do not want to update other users at least right
> now.
> 
> Yes, these 2 type casts do not look nice, but they are hidden in the trivial
> helpers. And, for example, if something goes wrong we can trivially change
> this code to use, say, sas_ss_sp. Just we need to update the comments to
> explain why it is safe too.
> 
> Finally. I still hope we will kill struct kthread (I mean, unbloat it and
> embed into task_struct), and this means that the proper union should touch
> more members. Say, sas_* and/or vfork_done+set/clear_child_tid. I'd like
> to do this only once if possible.

Fair enough.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1512835 — [PATCH 0/2] kthread: kill to_live_kthread()

On 10/28, Oleg Nesterov wrote:
>
> I'll try to kill to_live_kthread() tomorrow, didn't have time to do this
> today.

Hmm. And this looks even simpler than I thought, or I am totally confused.
Thomas, could you please review this series too?

Chunming, Alex, I _think_ that drm/amdgpu should not use kthread_park(), but
lets discuss this separately. It would be nice if you can confirm that 2/2
can't break this code, even if I think "it obviously can't".

Oleg.

[toc] | [prev] | [next] | [standalone]

#1512838 — [PATCH 1/2] kthread: don't use to_live_kthread() in kthread_stop()

kthread_stop() had to use to_live_kthread() simply because it was not
possible to access kthread->exited after the exiting kthread clears
task_struct->vfork_done. Now that to_kthread() is always valid we can
do wake_up_process() + wait_for_completion() unconditionally, we don't
care if it has already passed complete_vfork_done() or even dead.

The exiting kthread can get the spurious wakeup after mm_release() but
this is possible without this change too and this is fine, do_task_dead()
ensures that this can't make any harm.

Note: we can even change this function to use task_work_add() and avoid
->vfork_done altogether, probably we will do this later.

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

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 7891a94..4dcbc8b 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -532,13 +532,11 @@ int kthread_stop(struct task_struct *k)
 	trace_sched_kthread_stop(k);
 
 	get_task_struct(k);
-	kthread = to_live_kthread(k);
-	if (kthread) {
-		set_bit(KTHREAD_SHOULD_STOP, &kthread->flags);
-		__kthread_unpark(k, kthread);
-		wake_up_process(k);
-		wait_for_completion(&kthread->exited);
-	}
+	kthread = to_kthread(k);
+	set_bit(KTHREAD_SHOULD_STOP, &kthread->flags);
+	__kthread_unpark(k, kthread);
+	wake_up_process(k);
+	wait_for_completion(&kthread->exited);
 	ret = k->exit_code;
 	put_task_struct(k);
 
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1517814 — Re: [PATCH 1/2] kthread: don't use to_live_kthread() in kthread_stop()

On Mon, 31 Oct 2016, Oleg Nesterov wrote:

> kthread_stop() had to use to_live_kthread() simply because it was not
> possible to access kthread->exited after the exiting kthread clears
> task_struct->vfork_done. Now that to_kthread() is always valid we can
> do wake_up_process() + wait_for_completion() unconditionally, we don't
> care if it has already passed complete_vfork_done() or even dead.
> 
> The exiting kthread can get the spurious wakeup after mm_release() but
> this is possible without this change too and this is fine, do_task_dead()
> ensures that this can't make any harm.
> 
> Note: we can even change this function to use task_work_add() and avoid
> ->vfork_done altogether, probably we will do this later.
> 
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[toc] | [prev] | [next] | [standalone]

#1512839 — [PATCH 2/2] kthread: don't use to_live_kthread() in kthread_park() and kthread_unpark()

Now that to_kthread() is always valid we can change kthread_park() and
kthread_unpark() to use it and kill to_live_kthread().

kthread_unpark() is trivial, if KTHREAD_IS_PARKED is set we know that this
kthread has called complete(&self->parked), we do not care if it exits after
that if we race with kthread_stop().

kthread_park() is more tricky, but only because its semantics is not well
defined. It returns -ENOSYS if the thread exited but this can never happen
and as Roman pointed out kthread_park() can obviously block forever if it
could race with the exiting kthread.

I think we need to unexport kthread_park/unpark, and either make it return
"void" or actually fix the race with kthred_stop/exit. This patch just adds
WARN_ON(PF_EXITING) for now.

The usage of kthread_park() in cpuhp code (cpu.c, smpboot.c, stop_machine.c)
is fine. It can never see an exiting/exited kthread, smpboot_destroy_threads()
clears *ht->store, smpboot_park_thread() checks it is not NULL under the same
smpboot_threads_lock. cpuhp_threads and cpu_stop_threads never exit, so other
callers are fine too.

But it has two more users:

- watchdog_park_threads() and it does not look nice. The code is actually
  correct, get_online_cpus() ensures that kthread_park() can't race with
  itself (note that kthread_park() can't handle this race correctly), but
  imo it should not use kthread_park() directly.

- drivers/gpu/drm/amd/scheduler/gpu_scheduler.c and I think it should not
  use kthread_park() too.

  But this patch should not break this code. kthread_park() must not be
  called after amd_sched_fini() which does kthread_stop(), otherwise even
  to_live_kthread() is not safe because task_struct can be already freed
  and sched->thread can point to nowhere.

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/kthread.c | 69 ++++++++++++++++++++------------------------------------
 1 file changed, 24 insertions(+), 45 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 4dcbc8b..01d2716 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -78,21 +78,6 @@ void free_kthread_struct(struct task_struct *k)
 	kfree(to_kthread(k));
 }
 
-#define __to_kthread(vfork)	\
-	container_of(vfork, struct kthread, exited)
-
-/*
- * TODO: kill it and use to_kthread(). But we still need the users
- * like kthread_stop() which has to sync with the exiting kthread.
- */
-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?
  *
@@ -441,8 +426,18 @@ struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
 	return p;
 }
 
-static void __kthread_unpark(struct task_struct *k, struct kthread *kthread)
+/**
+ * kthread_unpark - unpark a thread created by kthread_create().
+ * @k:		thread created by kthread_create().
+ *
+ * Sets kthread_should_park() for @k to return false, wakes it, and
+ * waits for it to return. If the thread is marked percpu then its
+ * bound to the cpu again.
+ */
+void kthread_unpark(struct task_struct *k)
 {
+	struct kthread *kthread = to_kthread(k);
+
 	clear_bit(KTHREAD_SHOULD_PARK, &kthread->flags);
 	/*
 	 * We clear the IS_PARKED bit here as we don't wait
@@ -460,22 +455,6 @@ static void __kthread_unpark(struct task_struct *k, struct kthread *kthread)
 		wake_up_state(k, TASK_PARKED);
 	}
 }
-
-/**
- * kthread_unpark - unpark a thread created by kthread_create().
- * @k:		thread created by kthread_create().
- *
- * Sets kthread_should_park() for @k to return false, wakes it, and
- * waits for it to return. If the thread is marked percpu then its
- * bound to the cpu again.
- */
-void kthread_unpark(struct task_struct *k)
-{
-	struct kthread *kthread = to_live_kthread(k);
-
-	if (kthread)
-		__kthread_unpark(k, kthread);
-}
 EXPORT_SYMBOL_GPL(kthread_unpark);
 
 /**
@@ -492,20 +471,20 @@ EXPORT_SYMBOL_GPL(kthread_unpark);
  */
 int kthread_park(struct task_struct *k)
 {
-	struct kthread *kthread = to_live_kthread(k);
-	int ret = -ENOSYS;
-
-	if (kthread) {
-		if (!test_bit(KTHREAD_IS_PARKED, &kthread->flags)) {
-			set_bit(KTHREAD_SHOULD_PARK, &kthread->flags);
-			if (k != current) {
-				wake_up_process(k);
-				wait_for_completion(&kthread->parked);
-			}
+	struct kthread *kthread = to_kthread(k);
+
+	if (WARN_ON(k->flags & PF_EXITING))
+		return -ENOSYS;
+
+	if (!test_bit(KTHREAD_IS_PARKED, &kthread->flags)) {
+		set_bit(KTHREAD_SHOULD_PARK, &kthread->flags);
+		if (k != current) {
+			wake_up_process(k);
+			wait_for_completion(&kthread->parked);
 		}
-		ret = 0;
 	}
-	return ret;
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(kthread_park);
 
@@ -534,7 +513,7 @@ int kthread_stop(struct task_struct *k)
 	get_task_struct(k);
 	kthread = to_kthread(k);
 	set_bit(KTHREAD_SHOULD_STOP, &kthread->flags);
-	__kthread_unpark(k, kthread);
+	kthread_unpark(k);
 	wake_up_process(k);
 	wait_for_completion(&kthread->exited);
 	ret = k->exit_code;
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1517827 — Re: [PATCH 2/2] kthread: don't use to_live_kthread() in kthread_park() and kthread_unpark()

On Mon, 31 Oct 2016, Oleg Nesterov wrote:
> I think we need to unexport kthread_park/unpark, and either make it return
> "void" or actually fix the race with kthred_stop/exit. This patch just adds
> WARN_ON(PF_EXITING) for now.

I'll have a look.
 
> The usage of kthread_park() in cpuhp code (cpu.c, smpboot.c, stop_machine.c)
> is fine. It can never see an exiting/exited kthread, smpboot_destroy_threads()
> clears *ht->store, smpboot_park_thread() checks it is not NULL under the same
> smpboot_threads_lock. cpuhp_threads and cpu_stop_threads never exit, so other
> callers are fine too.
> 
> But it has two more users:
> 
> - watchdog_park_threads() and it does not look nice. The code is actually
>   correct, get_online_cpus() ensures that kthread_park() can't race with
>   itself (note that kthread_park() can't handle this race correctly), but
>   imo it should not use kthread_park() directly.

Should we provide an interface through the smpboot thread infrastructure for
this?

> - drivers/gpu/drm/amd/scheduler/gpu_scheduler.c and I think it should not
>   use kthread_park() too.
> 
>   But this patch should not break this code. kthread_park() must not be
>   called after amd_sched_fini() which does kthread_stop(), otherwise even
>   to_live_kthread() is not safe because task_struct can be already freed
>   and sched->thread can point to nowhere.

Right. That's why the smpboot thread code holds a task ref which is only
released after the thread has been stopped.

I can see why that gpu driver wants to use the park mechanism and I guess
there are other legitimate use cases as well. I prefer to implement a
park/unpark variant which is safe to use on arbitrary kthreads over forcing
driver writers to come up with even more broken open coded implementations
of that.

> Signed-off-by: Oleg Nesterov <oleg@redhat.com>

Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

[toc] | [prev] | [next] | [standalone]

#1518332 — Re: [PATCH 2/2] kthread: don't use to_live_kthread() in kthread_park() and kthread_unpark()

On 11/09, Thomas Gleixner wrote:
>
> > - watchdog_park_threads() and it does not look nice. The code is actually
> >   correct, get_online_cpus() ensures that kthread_park() can't race with
> >   itself (note that kthread_park() can't handle this race correctly), but
> >   imo it should not use kthread_park() directly.
>
> Should we provide an interface through the smpboot thread infrastructure for
> this?

IMHO yes, I'll write another email.

> I can see why that gpu driver wants to use the park mechanism and I guess
> there are other legitimate use cases as well. I prefer to implement a
> park/unpark variant which is safe to use on arbitrary kthreads

Yes, agreed. Again, I'll write another email. Perhaps we should even keep
park/unpark exported and change them to avoid the races with exit/itself,
I dunno.

My real point was, imo the KTHREAD_IS_PER_CPU/__kthread_bind(kthread->cpu)
logic in kthread_unpark() should be private to smpboot.c/cpu.c.

I'll send another patch tomorrow. kthread_create_worker_on_cpu() ab-uses
this logic too for no reason, but this is trivial.

> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>

Thanks!

Probably I should re-send these 2 short series to Ingo with your acks applied.

Oleg.

[toc] | [prev] | [next] | [standalone]

#1519168 — [PATCH 1/1] kthread: don't abuse kthread_create_on_cpu() in __kthread_create_worker()

kthread_create_on_cpu() sets KTHREAD_IS_PER_CPU and kthread->cpu, this
only makes sense if this kthread can be parked/unparked by cpuhp code.
kthread workers never call kthread_parkme() so this has no effect.

Change __kthread_create_worker() to simply call kthread_bind(task, cpu).
The very fact that kthread_create_on_cpu() doesn't accept a generic fmt
shows that it should not be used outside of smpboot.c.

Now, the only reason we can not unexport this helper and move it into
smpboot.c is that it sets kthread->cpu and struct kthread is not exported.
And the only reason we can not kill kthread->cpu is that kthread_unpark()
is used by drivers/gpu/drm/amd/scheduler/gpu_scheduler.c and thus we can
not turn _unpark into kthread_unpark(struct smp_hotplug_thread *, cpu).

Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 kernel/kthread.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 01d2716..956495f 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -641,6 +641,7 @@ __kthread_create_worker(int cpu, unsigned int flags,
 {
 	struct kthread_worker *worker;
 	struct task_struct *task;
+	int node = -1;
 
 	worker = kzalloc(sizeof(*worker), GFP_KERNEL);
 	if (!worker)
@@ -648,25 +649,17 @@ __kthread_create_worker(int cpu, unsigned int flags,
 
 	kthread_init_worker(worker);
 
-	if (cpu >= 0) {
-		char name[TASK_COMM_LEN];
-
-		/*
-		 * kthread_create_worker_on_cpu() allows to pass a generic
-		 * namefmt in compare with kthread_create_on_cpu. We need
-		 * to format it here.
-		 */
-		vsnprintf(name, sizeof(name), namefmt, args);
-		task = kthread_create_on_cpu(kthread_worker_fn, worker,
-					     cpu, name);
-	} else {
-		task = __kthread_create_on_node(kthread_worker_fn, worker,
-						-1, namefmt, args);
-	}
+	if (cpu >= 0)
+		node = cpu_to_node(cpu);
 
+	task = __kthread_create_on_node(kthread_worker_fn, worker,
+						node, namefmt, args);
 	if (IS_ERR(task))
 		goto fail_task;
 
+	if (cpu >= 0)
+		kthread_bind(task, cpu);
+
 	worker->flags = flags;
 	worker->task = task;
 	wake_up_process(task);
-- 
2.5.0

[toc] | [prev] | [next] | [standalone]

#1519169 — [PATCH 0/1] kthread: don't abuse kthread_create_on_cpu() in __kthread_create_worker()

On 11/09, Oleg Nesterov wrote:
>
> Yes, agreed. Again, I'll write another email. Perhaps we should even keep
> park/unpark exported and change them to avoid the races with exit/itself,
> I dunno.
>
> My real point was, imo the KTHREAD_IS_PER_CPU/__kthread_bind(kthread->cpu)
> logic in kthread_unpark() should be private to smpboot.c/cpu.c.
>
> I'll send another patch tomorrow. kthread_create_worker_on_cpu() ab-uses
> this logic too for no reason, but this is trivial.

After this change we are almost ready to kill kthread->cpu and KTHREAD_IS_PER_CPU.
(but the change itself doesn't depend on the previous patches).

Petr, why do we need kthread_create_worker_on_cpu() ? It has no users and
I can not imagine any "real" use-case for it. Perhaps it can be removed?

Oleg.

[toc] | [prev] | [next] | [standalone]

#1516448 — Re: [PATCH 0/2] kthread: kill to_live_kthread()

Ping?

What's the current status of this?  Do we need this for 4.9?

On Mon, Oct 31, 2016 at 1:07 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 10/28, Oleg Nesterov wrote:
>>
>> I'll try to kill to_live_kthread() tomorrow, didn't have time to do this
>> today.
>
> Hmm. And this looks even simpler than I thought, or I am totally confused.
> Thomas, could you please review this series too?
>
> Chunming, Alex, I _think_ that drm/amdgpu should not use kthread_park(), but
> lets discuss this separately. It would be nice if you can confirm that 2/2
> can't break this code, even if I think "it obviously can't".
>
> Oleg.
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web