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

Groups > linux.kernel > #1316847 > unrolled thread

[PATCH v4 00/22] kthread: Use kthread worker API more widely

Back to article view | Back to linux.kernel

Contents

  [PATCH v4 00/22] kthread: Use kthread worker API more widely Petr Mladek <pmladek@suse.com> - 2016-01-25 16:50 +0100
    [PATCH v4 21/22] thermal/intel_powerclamp: Remove duplicated code that starts the kthread Petr Mladek <pmladek@suse.com> - 2016-01-25 16:50 +0100
      Re: [PATCH v4 21/22] thermal/intel_powerclamp: Remove duplicated  code that starts the kthread Jacob Pan <jacob.jun.pan@linux.intel.com> - 2016-01-25 17:30 +0100
    [PATCH v4 18/22] IB/fmr_pool: Convert the cleanup thread into kthread worker API Petr Mladek <pmladek@suse.com> - 2016-01-25 16:50 +0100
    [PATCH v4 03/22] kthread: Allow to call __kthread_create_on_node() with va_list args Petr Mladek <pmladek@suse.com> - 2016-01-25 16:50 +0100
    [PATCH v4 19/22] memstick/r592: Better synchronize debug messages in r592_io kthread Petr Mladek <pmladek@suse.com> - 2016-01-25 16:50 +0100
    [PATCH v4 12/22] kthread: Use try_lock_kthread_work() in flush_kthread_work() Petr Mladek <pmladek@suse.com> - 2016-01-25 16:50 +0100
    [PATCH v4 17/22] ipmi: Convert kipmi kthread into kthread worker API Petr Mladek <pmladek@suse.com> - 2016-01-25 16:50 +0100
    [PATCH v4 08/22] kthread: Initial support for delayed kthread work Petr Mladek <pmladek@suse.com> - 2016-01-25 16:50 +0100
      Re: [PATCH v4 08/22] kthread: Initial support for delayed kthread  work Tejun Heo <tj@kernel.org> - 2016-01-25 20:10 +0100
    [PATCH v4 01/22] timer: Allow to check when the timer callback has not finished yet Petr Mladek <pmladek@suse.com> - 2016-01-25 16:50 +0100
      Re: [PATCH v4 01/22] timer: Allow to check when the timer callback  has not finished yet Tejun Heo <tj@kernel.org> - 2016-01-25 19:50 +0100
    [PATCH v4 06/22] kthread: Add destroy_kthread_worker() Petr Mladek <pmladek@suse.com> - 2016-01-25 16:50 +0100
    [PATCH v4 22/22] thermal/intel_powerclamp: Convert the kthread to kthread worker API Petr Mladek <pmladek@suse.com> - 2016-01-25 16:50 +0100
      Re: [PATCH v4 22/22] thermal/intel_powerclamp: Convert the kthread  to kthread worker API Jacob Pan <jacob.jun.pan@linux.intel.com> - 2016-01-25 17:40 +0100
    [PATCH v4 20/22] memstick/r592: convert r592_io kthread into kthread worker API Petr Mladek <pmladek@suse.com> - 2016-01-25 17:00 +0100
    [PATCH v4 15/22] hung_task: Convert hungtaskd into kthread worker API Petr Mladek <pmladek@suse.com> - 2016-01-25 17:00 +0100
    [PATCH v4 16/22] kmemleak: Convert kmemleak kthread into kthread worker API Petr Mladek <pmladek@suse.com> - 2016-01-25 17:00 +0100
    [PATCH v4 13/22] mm/huge_page: Convert khugepaged() into kthread worker API Petr Mladek <pmladek@suse.com> - 2016-01-25 17:00 +0100
    [PATCH v4 14/22] ring_buffer: Convert benchmark kthreads into kthread worker API Petr Mladek <pmladek@suse.com> - 2016-01-25 17:00 +0100
    [PATCH v4 11/22] kthread: Better support freezable kthread workers Petr Mladek <pmladek@suse.com> - 2016-01-25 17:10 +0100
      Re: [PATCH v4 11/22] kthread: Better support freezable kthread  workers Tejun Heo <tj@kernel.org> - 2016-01-25 20:30 +0100
    [PATCH v4 09/22] kthread: Allow to cancel kthread work Petr Mladek <pmladek@suse.com> - 2016-01-25 17:20 +0100
      Re: [PATCH v4 09/22] kthread: Allow to cancel kthread work Tejun Heo <tj@kernel.org> - 2016-01-25 20:20 +0100
    [PATCH v4 02/22] kthread/smpboot: Do not park in kthread_create_on_cpu() Petr Mladek <pmladek@suse.com> - 2016-01-25 17:20 +0100
    [PATCH v4 07/22] kthread: Detect when a kthread work is used by more workers Petr Mladek <pmladek@suse.com> - 2016-01-25 17:20 +0100
      Re: [PATCH v4 07/22] kthread: Detect when a kthread work is used by  more workers Tejun Heo <tj@kernel.org> - 2016-01-25 20:00 +0100
    [PATCH v4 04/22] kthread: Add create_kthread_worker*() Petr Mladek <pmladek@suse.com> - 2016-01-25 17:20 +0100
      Re: [PATCH v4 04/22] kthread: Add create_kthread_worker*() Tejun Heo <tj@kernel.org> - 2016-01-25 20:00 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1316925 — [PATCH v4 11/22] kthread: Better support freezable kthread workers

This patch allows to make kthread worker freezable via a new @flags
parameter. It will allow to avoid an init work in some kthreads.

It currently does not affect the function of kthread_worker_fn()
but it might help to do some optimization or fixes eventually.

I currently do not know about any other use for the @flags
parameter but I believe that we will want more flags
in the future.

Finally, I hope that it will not cause confusion with @flags member
in struct kthread. Well, I guess that we will want to rework the
basic kthreads implementation once all kthreads are converted into
kthread workers or workqueues. It is possible that we will merge
the two structures.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 include/linux/kthread.h | 11 ++++++++---
 kernel/kthread.c        | 17 ++++++++++++-----
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index f501dfeaa0e3..2dad7020047f 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -65,7 +65,12 @@ struct kthread_work;
 typedef void (*kthread_work_func_t)(struct kthread_work *work);
 void delayed_kthread_work_timer_fn(unsigned long __data);
 
+enum {
+	KTW_FREEZABLE		= 1 << 2,	/* freeze during suspend */
+};
+
 struct kthread_worker {
+	unsigned int		flags;
 	spinlock_t		lock;
 	struct list_head	work_list;
 	struct task_struct	*task;
@@ -154,12 +159,12 @@ extern void __init_kthread_worker(struct kthread_worker *worker,
 
 int kthread_worker_fn(void *worker_ptr);
 
-__printf(1, 2)
+__printf(2, 3)
 struct kthread_worker *
-create_kthread_worker(const char namefmt[], ...);
+create_kthread_worker(unsigned int flags, const char namefmt[], ...);
 
 struct kthread_worker *
-create_kthread_worker_on_cpu(int cpu, const char namefmt[]);
+create_kthread_worker_on_cpu(unsigned int flags, int cpu, const char namefmt[]);
 
 bool queue_kthread_work(struct kthread_worker *worker,
 			struct kthread_work *work);
diff --git a/kernel/kthread.c b/kernel/kthread.c
index ebb91848685f..53c4d5a7c723 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -556,6 +556,7 @@ void __init_kthread_worker(struct kthread_worker *worker,
 				const char *name,
 				struct lock_class_key *key)
 {
+	worker->flags = 0;
 	spin_lock_init(&worker->lock);
 	lockdep_set_class_and_name(&worker->lock, key, name);
 	INIT_LIST_HEAD(&worker->work_list);
@@ -605,6 +606,10 @@ int kthread_worker_fn(void *worker_ptr)
 	 */
 	WARN_ON(worker->task && worker->task != current);
 	worker->task = current;
+
+	if (worker->flags & KTW_FREEZABLE)
+		set_freezable();
+
 repeat:
 	set_current_state(TASK_INTERRUPTIBLE);	/* mb paired w/ kthread_stop */
 
@@ -638,7 +643,8 @@ repeat:
 EXPORT_SYMBOL_GPL(kthread_worker_fn);
 
 static struct kthread_worker *
-__create_kthread_worker(int cpu, const char namefmt[], va_list args)
+__create_kthread_worker(unsigned int flags, int cpu,
+			const char namefmt[], va_list args)
 {
 	struct kthread_worker *worker;
 	struct task_struct *task;
@@ -658,6 +664,7 @@ __create_kthread_worker(int cpu, const char namefmt[], va_list args)
 	if (IS_ERR(task))
 		goto fail_task;
 
+	worker->flags = flags;
 	worker->task = task;
 	wake_up_process(task);
 	return worker;
@@ -676,13 +683,13 @@ fail_task:
  * the worker was SIGKILLed.
  */
 struct kthread_worker *
-create_kthread_worker(const char namefmt[], ...)
+create_kthread_worker(unsigned int flags, const char namefmt[], ...)
 {
 	struct kthread_worker *worker;
 	va_list args;
 
 	va_start(args, namefmt);
-	worker = __create_kthread_worker(-1, namefmt, args);
+	worker = __create_kthread_worker(flags, -1, namefmt, args);
 	va_end(args);
 
 	return worker;
@@ -706,12 +713,12 @@ EXPORT_SYMBOL(create_kthread_worker);
  * ERR_PTR(-EINVAL) on invalid @cpu.
  */
 struct kthread_worker *
-create_kthread_worker_on_cpu(int cpu, const char namefmt[])
+create_kthread_worker_on_cpu(unsigned int flags, int cpu, const char namefmt[])
 {
 	if (cpu < 0 || cpu > num_possible_cpus())
 		return ERR_PTR(-EINVAL);
 
-	return __create_kthread_worker(cpu, namefmt, NULL);
+	return __create_kthread_worker(flags, cpu, namefmt, NULL);
 }
 EXPORT_SYMBOL(create_kthread_worker_on_cpu);
 
-- 
1.8.5.6

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

#1317231 — Re: [PATCH v4 11/22] kthread: Better support freezable kthread workers

On Mon, Jan 25, 2016 at 04:45:00PM +0100, Petr Mladek wrote:
> @@ -556,6 +556,7 @@ void __init_kthread_worker(struct kthread_worker *worker,
>  				const char *name,
>  				struct lock_class_key *key)
>  {
> +	worker->flags = 0;
>  	spin_lock_init(&worker->lock);
>  	lockdep_set_class_and_name(&worker->lock, key, name);
>  	INIT_LIST_HEAD(&worker->work_list);

Maybe memset the thing and drop 0, NULL inits?

> @@ -638,7 +643,8 @@ repeat:
>  EXPORT_SYMBOL_GPL(kthread_worker_fn);
>  
>  static struct kthread_worker *
> -__create_kthread_worker(int cpu, const char namefmt[], va_list args)
> +__create_kthread_worker(unsigned int flags, int cpu,
> +			const char namefmt[], va_list args)

Wouldn't @cpu, @flags be less confusing?  You would end up with, (A,
B, C) and (B, C) instead of (A, B, C) and (A, C).

Thanks.

-- 
tejun

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

#1316939 — [PATCH v4 09/22] kthread: Allow to cancel kthread work

We are going to use kthread workers more widely and sometimes we will need
to make sure that the work is neither pending nor running.

This patch implements cancel_*_sync() operations as inspired by
workqueues. Well, we are synchronized against the other operations
via the worker lock, we use del_timer_sync() and a counter to count
parallel cancel operations. Therefore the implementation might be easier.

First, we try to lock the work. If it does not work, it means that
no worker is assigned and that we are done.

Second, we try to cancel the timer when it exists. The timer is deleted
synchronously to make sure that the timer call back is not running.
We hold the worker->lock at this point. To avoid a possible race,
the timer callback need to use a modified variant of try_lock_kthread_work()
and give up spinning when the canceling flag is set.

Third, we try to remove the work from the worker list.

Fourth, if the work is running, we call flush_kthread_work(). It might
take an arbitrary time. In the meantime, queuing of the work is blocked
by the new canceling counter.

As already mentioned, the check for a pending kthread work is done under
a lock. In compare with workqueues, we do not need to fight for a single
PENDING bit to block other operations. Therefore do not suffer from
the thundering storm problem and all parallel canceling jobs might use
kthread_work_flush(). Any queuing is blocked until the counter is zero.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 include/linux/kthread.h |   4 ++
 kernel/kthread.c        | 154 ++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 154 insertions(+), 4 deletions(-)

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 1a5738dcdf8d..dd2a587a2bd7 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -77,6 +77,7 @@ struct kthread_work {
 	kthread_work_func_t	func;
 	struct kthread_worker	*worker;
 	struct timer_list	*timer;
+	int			canceling;
 };
 
 struct delayed_kthread_work {
@@ -170,6 +171,9 @@ bool queue_delayed_kthread_work(struct kthread_worker *worker,
 void flush_kthread_work(struct kthread_work *work);
 void flush_kthread_worker(struct kthread_worker *worker);
 
+bool cancel_kthread_work_sync(struct kthread_work *work);
+bool cancel_delayed_kthread_work_sync(struct delayed_kthread_work *work);
+
 void destroy_kthread_worker(struct kthread_worker *worker);
 
 #endif /* _LINUX_KTHREAD_H */
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 6933d90c0fec..6e2eeca08d5f 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -567,6 +567,7 @@ EXPORT_SYMBOL_GPL(__init_kthread_worker);
  * Returns true when there is a pending operation for this work.
  * In particular, it checks if the work is:
  *	- queued
+ *	- being cancelled
  *	- a timer is running to queue this delayed work
  *
  * This function must be called with locked work.
@@ -574,6 +575,7 @@ EXPORT_SYMBOL_GPL(__init_kthread_worker);
 static inline bool kthread_work_pending(const struct kthread_work *work)
 {
 	return !list_empty(&work->node) ||
+	       work->canceling ||
 	       (work->timer && timer_active(work->timer));
 }
 
@@ -779,7 +781,13 @@ bool queue_kthread_work(struct kthread_worker *worker,
 }
 EXPORT_SYMBOL_GPL(queue_kthread_work);
 
-static bool try_lock_kthread_work(struct kthread_work *work)
+/*
+ * Get the worker lock if any worker is associated with the work.
+ * Depending on @check_canceling, it might need to give up the busy
+ * wait when work->canceling gets set.
+ */
+static bool try_lock_kthread_work(struct kthread_work *work,
+				  bool check_canceling)
 {
 	struct kthread_worker *worker;
 	int ret = false;
@@ -790,7 +798,24 @@ try_again:
 	if (!worker)
 		goto out;
 
-	spin_lock(&worker->lock);
+	if (check_canceling) {
+		if (!spin_trylock(&worker->lock)) {
+			/*
+			 * Busy wait with spin_is_locked() to avoid
+			 * cache bouncing. Break when canceling
+			 * is set to avoid a deadlock.
+			 */
+			do {
+				if (READ_ONCE(work->canceling))
+					goto out;
+				cpu_relax();
+			} while (spin_is_locked(&worker->lock));
+			goto try_again;
+		}
+	} else {
+		spin_lock(&worker->lock);
+	}
+
 	if (worker != work->worker) {
 		spin_unlock(&worker->lock);
 		goto try_again;
@@ -820,10 +845,13 @@ void delayed_kthread_work_timer_fn(unsigned long __data)
 		(struct delayed_kthread_work *)__data;
 	struct kthread_work *work = &dwork->work;
 
-	if (!try_lock_kthread_work(work))
+	/* Give up when the work is being canceled. */
+	if (!try_lock_kthread_work(work, true))
 		return;
 
-	__queue_kthread_work(work->worker, work);
+	if (!work->canceling)
+		__queue_kthread_work(work->worker, work);
+
 	unlock_kthread_work(work);
 }
 EXPORT_SYMBOL(delayed_kthread_work_timer_fn);
@@ -947,6 +975,124 @@ retry:
 EXPORT_SYMBOL_GPL(flush_kthread_work);
 
 /**
+ * try_to_cancel_kthread_work - Try to cancel kthread work.
+ * @work: work item to cancel
+ * @lock: lock used to protect the work
+ * @flags: flags stored when the lock was taken
+ *
+ * This function tries to cancel the given kthread work by deleting
+ * the timer and by removing the work from the queue.
+ *
+ * If the timer callback is in progress, it waits until it finishes
+ * but it has to drop the lock to avoid a deadlock.
+ *
+ * Return:
+ *  1		if @work was pending and successfully canceled
+ *  0		if @work was not pending
+ */
+static int
+try_to_cancel_kthread_work(struct kthread_work *work,
+				   spinlock_t *lock,
+				   unsigned long *flags)
+{
+	int ret = 0;
+
+	/* Try to cancel the timer if pending. */
+	if (work->timer && del_timer_sync(work->timer)) {
+		ret = 1;
+		goto out;
+	}
+
+	/* Try to remove queued work before it is being executed. */
+	if (!list_empty(&work->node)) {
+		list_del_init(&work->node);
+		ret = 1;
+	}
+
+out:
+	return ret;
+}
+
+static bool __cancel_kthread_work_sync(struct kthread_work *work)
+{
+	struct kthread_worker *worker;
+	unsigned long flags;
+	int ret;
+
+	local_irq_save(flags);
+	if (!try_lock_kthread_work(work, false)) {
+		local_irq_restore(flags);
+		ret = 0;
+		goto out;
+	}
+	worker = work->worker;
+
+	/*
+	 * Block further queueing. It must be set before trying to cancel
+	 * the kthread work. It avoids a possible deadlock between
+	 * del_timer_sync() and the timer callback.
+	 */
+	work->canceling++;
+	ret = try_to_cancel_kthread_work(work, &worker->lock, &flags);
+
+	if (worker->current_work != work)
+		goto out_fast;
+
+	spin_unlock_irqrestore(&worker->lock, flags);
+	flush_kthread_work(work);
+	/*
+	 * Nobody is allowed to switch the worker or queue the work
+	 * when .canceling is set.
+	 */
+	spin_lock_irqsave(&worker->lock, flags);
+
+out_fast:
+	work->canceling--;
+	spin_unlock_irqrestore(&worker->lock, flags);
+out:
+	return ret;
+}
+
+/**
+ * cancel_kthread_work_sync - cancel a kthread work and wait for it to finish
+ * @work: the kthread work to cancel
+ *
+ * Cancel @work and wait for its execution to finish.  This function
+ * can be used even if the work re-queues itself. On return from this
+ * function, @work is guaranteed to be not pending or executing on any CPU.
+ *
+ * The caller must ensure that the worker on which @work was last
+ * queued can't be destroyed before this function returns.
+ *
+ * Return:
+ * %true if @work was pending, %false otherwise.
+ */
+bool cancel_kthread_work_sync(struct kthread_work *work)
+{
+	/* Rather use cancel_delayed_kthread_work() for delayed works. */
+	WARN_ON_ONCE(work->timer);
+
+	return __cancel_kthread_work_sync(work);
+}
+EXPORT_SYMBOL_GPL(cancel_kthread_work_sync);
+
+/**
+ * cancel_delayed_kthread_work_sync - cancel a delayed kthread work and
+ *	wait for it to finish.
+ * @dwork: the delayed kthread work to cancel
+ *
+ * This is cancel_kthread_work_sync() for delayed works.
+ *
+ * Return:
+ * %true if @dwork was pending, %false otherwise.
+ */
+bool cancel_delayed_kthread_work_sync(struct delayed_kthread_work *dwork)
+{
+	return __cancel_kthread_work_sync(&dwork->work);
+}
+EXPORT_SYMBOL_GPL(cancel_delayed_kthread_work_sync);
+
+/**
  * flush_kthread_worker - flush all current works on a kthread_worker
  * @worker: worker to flush
  *
-- 
1.8.5.6

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

#1317227 — Re: [PATCH v4 09/22] kthread: Allow to cancel kthread work

Hello,

On Mon, Jan 25, 2016 at 04:44:58PM +0100, Petr Mladek wrote:
> @@ -574,6 +575,7 @@ EXPORT_SYMBOL_GPL(__init_kthread_worker);
>  static inline bool kthread_work_pending(const struct kthread_work *work)
>  {
>  	return !list_empty(&work->node) ||
> +	       work->canceling ||
>  	       (work->timer && timer_active(work->timer));
>  }

So, the reason ->canceling test is necessary is to ensure that
self-requeueing work items can be canceled reliably.  It's not to
block "further queueing" in general.  It's probably worthwhile to
clear that up in the description and comment.

> +/*
> + * Get the worker lock if any worker is associated with the work.
> + * Depending on @check_canceling, it might need to give up the busy
> + * wait when work->canceling gets set.
> + */

While mentioning @check_canceling, the above doesn't actually explain
what it does.

> +static bool try_lock_kthread_work(struct kthread_work *work,
> +				  bool check_canceling)
>  {
>  	struct kthread_worker *worker;
>  	int ret = false;
> @@ -790,7 +798,24 @@ try_again:
>  	if (!worker)
>  		goto out;
>  
> -	spin_lock(&worker->lock);
> +	if (check_canceling) {
> +		if (!spin_trylock(&worker->lock)) {
> +			/*
> +			 * Busy wait with spin_is_locked() to avoid
> +			 * cache bouncing. Break when canceling
> +			 * is set to avoid a deadlock.
> +			 */
> +			do {
> +				if (READ_ONCE(work->canceling))
> +					goto out;

Why READ_ONCE?

> +				cpu_relax();
> +			} while (spin_is_locked(&worker->lock));
> +			goto try_again;
> +		}
> +	} else {
> +		spin_lock(&worker->lock);
> +	}
> +
>  	if (worker != work->worker) {
>  		spin_unlock(&worker->lock);
>  		goto try_again;
> @@ -820,10 +845,13 @@ void delayed_kthread_work_timer_fn(unsigned long __data)
>  		(struct delayed_kthread_work *)__data;
>  	struct kthread_work *work = &dwork->work;
>  
> -	if (!try_lock_kthread_work(work))
> +	/* Give up when the work is being canceled. */
> +	if (!try_lock_kthread_work(work, true))

Again, this is the trickest part of the whole thing.  Please add a
comment explaining why this is necessary.

>  		return;
>  
> -	__queue_kthread_work(work->worker, work);
> +	if (!work->canceling)
> +		__queue_kthread_work(work->worker, work);
> +
...
> +static int
> +try_to_cancel_kthread_work(struct kthread_work *work,
> +				   spinlock_t *lock,
> +				   unsigned long *flags)

bool?

> +{
> +	int ret = 0;
> +
> +	/* Try to cancel the timer if pending. */
> +	if (work->timer && del_timer_sync(work->timer)) {
> +		ret = 1;
> +		goto out;
> +	}
> +
> +	/* Try to remove queued work before it is being executed. */
> +	if (!list_empty(&work->node)) {
> +		list_del_init(&work->node);
> +		ret = 1;
> +	}
> +
> +out:
> +	return ret;

Again, what's up with unnecessary goto exits?

> +static bool __cancel_kthread_work_sync(struct kthread_work *work)
> +{
> +	struct kthread_worker *worker;
> +	unsigned long flags;
> +	int ret;
> +
> +	local_irq_save(flags);
> +	if (!try_lock_kthread_work(work, false)) {
> +		local_irq_restore(flags);

Can't try_lock_kthread_work() take &flags?

> +		ret = 0;
> +		goto out;
> +	}
> +	worker = work->worker;
> +
> +	/*
> +	 * Block further queueing. It must be set before trying to cancel
> +	 * the kthread work. It avoids a possible deadlock between
> +	 * del_timer_sync() and the timer callback.
> +	 */

So, "blocking further queueing" and "a possible deadlock between
del_timer_sync() and the timer callback" don't have anything to do
with each other, do they?  Those are two separate things.  You need
the former to guarantee cancelation of self-requeueing work items and
the latter for deadlock avoidance, no?

> +	work->canceling++;
> +	ret = try_to_cancel_kthread_work(work, &worker->lock, &flags);
> +
> +	if (worker->current_work != work)
> +		goto out_fast;

If there are two racing cancellers, wouldn't this allow the losing one
to return while the work item is still running?

> +	spin_unlock_irqrestore(&worker->lock, flags);
> +	flush_kthread_work(work);
> +	/*
> +	 * Nobody is allowed to switch the worker or queue the work
> +	 * when .canceling is set.
> +	 */
> +	spin_lock_irqsave(&worker->lock, flags);
> +
> +out_fast:
> +	work->canceling--;
> +	spin_unlock_irqrestore(&worker->lock, flags);
> +out:
> +	return ret;
> +}

Thanks.

-- 
tejun

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

#1316943 — [PATCH v4 02/22] kthread/smpboot: Do not park in kthread_create_on_cpu()

kthread_create_on_cpu() was added by the commit 2a1d446019f9a5983e
("kthread: Implement park/unpark facility"). It is currently used
only when enabling new CPU. For this purpose, the newly created
kthread has to be parked.

The CPU binding is a bit tricky. The kthread is parked when the CPU
has not been allowed yet. And the CPU is bound when the kthread
is unparked.

The function would be useful for more per-CPU kthreads, e.g.
bnx2fc_thread, fcoethread. For this purpose, the newly created
kthread should stay in the uninterruptible state.

This patch moves the parking into smpboot. It binds the thread
already when created. Then the function might be used universally.
Also the behavior is consistent with kthread_create() and
kthread_create_on_node().

Signed-off-by: Petr Mladek <pmladek@suse.com>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/kthread.c | 8 ++++++--
 kernel/smpboot.c | 5 +++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 9ff173dca1ae..1ffc11ec5546 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -390,10 +390,10 @@ struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
 				   cpu);
 	if (IS_ERR(p))
 		return p;
+	kthread_bind(p, cpu);
+	/* CPU hotplug need to bind once again when unparking the thread. */
 	set_bit(KTHREAD_IS_PER_CPU, &to_kthread(p)->flags);
 	to_kthread(p)->cpu = cpu;
-	/* Park the thread to get it out of TASK_UNINTERRUPTIBLE state */
-	kthread_park(p);
 	return p;
 }
 
@@ -407,6 +407,10 @@ static void __kthread_unpark(struct task_struct *k, struct kthread *kthread)
 	 * which might be about to be cleared.
 	 */
 	if (test_and_clear_bit(KTHREAD_IS_PARKED, &kthread->flags)) {
+		/*
+		 * Newly created kthread was parked when the CPU was offline.
+		 * The binding was lost and we need to set it again.
+		 */
 		if (test_bit(KTHREAD_IS_PER_CPU, &kthread->flags))
 			__kthread_bind(k, kthread->cpu, TASK_PARKED);
 		wake_up_state(k, TASK_PARKED);
diff --git a/kernel/smpboot.c b/kernel/smpboot.c
index d264f59bff56..79f07014be6e 100644
--- a/kernel/smpboot.c
+++ b/kernel/smpboot.c
@@ -186,6 +186,11 @@ __smpboot_create_thread(struct smp_hotplug_thread *ht, unsigned int cpu)
 		kfree(td);
 		return PTR_ERR(tsk);
 	}
+	/*
+	 * Park the thread so that it could start right on the CPU
+	 * when it is available.
+	 */
+	kthread_park(tsk);
 	get_task_struct(tsk);
 	*per_cpu_ptr(ht->store, cpu) = tsk;
 	if (ht->create) {
-- 
1.8.5.6

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

#1316946 — [PATCH v4 07/22] kthread: Detect when a kthread work is used by more workers

Nothing currently prevents a work from queuing for a kthread worker
when it is already running on another one. This means that the work
might run in parallel on more workers. Also some operations, e.g.
flush or drain are not reliable.

This problem will be even more visible after we add cancel_kthread_work()
function. It will only have "work" as the parameter and will use
worker->lock to synchronize with others.

Well, normally this is not a problem because the API users are sane.
But bugs might happen and users also might be crazy.

This patch adds a warning when we try to insert the work for another
worker. It does not fully prevent the misuse because it would make the
code much more complicated without a big benefit.

A side effect is that one needs to explicitely reinitialize the work
if it must be queued into another worker. This is needed, for example,
when the worker is stopped and started again. It is a bit inconvenient.
But it looks like a good compromise between the stability and complexity.

Just for completeness, the patch adds a check for disabled interrupts
and an empty queue.

The patch also puts all the checks into a separate function. It will
be reused when implementing delayed works.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/kthread.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 1d41e0faef2d..e12576bc0e39 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -574,6 +574,9 @@ EXPORT_SYMBOL_GPL(__init_kthread_worker);
  * The works are not allowed to keep any locks, disable preemption or interrupts
  * when they finish. There is defined a safe point for freezing when one work
  * finishes and before a new one is started.
+ *
+ * Also the works must not be handled by more workers at the same time, see also
+ * queue_kthread_work().
  */
 int kthread_worker_fn(void *worker_ptr)
 {
@@ -696,12 +699,22 @@ create_kthread_worker_on_cpu(int cpu, const char namefmt[])
 }
 EXPORT_SYMBOL(create_kthread_worker_on_cpu);
 
+static void insert_kthread_work_sanity_check(struct kthread_worker *worker,
+					       struct kthread_work *work)
+{
+	lockdep_assert_held(&worker->lock);
+	WARN_ON_ONCE(!irqs_disabled());
+	WARN_ON_ONCE(!list_empty(&work->node));
+	/* Do not use a work with more workers, see queue_kthread_work() */
+	WARN_ON_ONCE(work->worker && work->worker != worker);
+}
+
 /* insert @work before @pos in @worker */
 static void insert_kthread_work(struct kthread_worker *worker,
-			       struct kthread_work *work,
-			       struct list_head *pos)
+				struct kthread_work *work,
+				struct list_head *pos)
 {
-	lockdep_assert_held(&worker->lock);
+	insert_kthread_work_sanity_check(worker, work);
 
 	list_add_tail(&work->node, pos);
 	work->worker = worker;
@@ -717,6 +730,15 @@ static void insert_kthread_work(struct kthread_worker *worker,
  * Queue @work to work processor @task for async execution.  @task
  * must have been created with kthread_worker_create().  Returns %true
  * if @work was successfully queued, %false if it was already pending.
+ *
+ * Never queue a work into a worker when it is being processed by another
+ * one. Otherwise, some operations, e.g. cancel or flush, will not work
+ * correctly or the work might run in parallel. This is not enforced
+ * because it would make the code too complex. There are only warnings
+ * printed when such a situation is detected.
+ *
+ * Reinitialize the work if it needs to be used by another worker.
+ * For example, when the worker was stopped and started again.
  */
 bool queue_kthread_work(struct kthread_worker *worker,
 			struct kthread_work *work)
-- 
1.8.5.6

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

#1317210 — Re: [PATCH v4 07/22] kthread: Detect when a kthread work is used by more workers

On Mon, Jan 25, 2016 at 04:44:56PM +0100, Petr Mladek wrote:
> +static void insert_kthread_work_sanity_check(struct kthread_worker *worker,
> +					       struct kthread_work *work)
> +{
> +	lockdep_assert_held(&worker->lock);
> +	WARN_ON_ONCE(!irqs_disabled());

Isn't worker->lock gonna be a irq-safe lock?  If so, why would this
need to be tested separately?

> +	WARN_ON_ONCE(!list_empty(&work->node));
> +	/* Do not use a work with more workers, see queue_kthread_work() */
> +	WARN_ON_ONCE(work->worker && work->worker != worker);
> +}

Is this sanity check function gonna be used from multiple places?

>  /* insert @work before @pos in @worker */
>  static void insert_kthread_work(struct kthread_worker *worker,
> -			       struct kthread_work *work,
> -			       struct list_head *pos)
> +				struct kthread_work *work,
> +				struct list_head *pos)
>  {
> -	lockdep_assert_held(&worker->lock);
> +	insert_kthread_work_sanity_check(worker, work);
>  
>  	list_add_tail(&work->node, pos);
>  	work->worker = worker;
> @@ -717,6 +730,15 @@ static void insert_kthread_work(struct kthread_worker *worker,
>   * Queue @work to work processor @task for async execution.  @task
>   * must have been created with kthread_worker_create().  Returns %true
>   * if @work was successfully queued, %false if it was already pending.
> + *
> + * Never queue a work into a worker when it is being processed by another
> + * one. Otherwise, some operations, e.g. cancel or flush, will not work
> + * correctly or the work might run in parallel. This is not enforced
> + * because it would make the code too complex. There are only warnings
> + * printed when such a situation is detected.

I'm not sure the above paragraph adds much.  It isn't that accurate to
begin with as what's being disallowed is larger scope than the above.
Isn't the paragraph below enough?

> + * Reinitialize the work if it needs to be used by another worker.
> + * For example, when the worker was stopped and started again.
>   */
>  bool queue_kthread_work(struct kthread_worker *worker,
>  			struct kthread_work *work)
> -- 
> 1.8.5.6
> 

-- 
tejun

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

#1316950 — [PATCH v4 04/22] kthread: Add create_kthread_worker*()

Kthread workers are currently created using the classic kthread API,
namely kthread_run(). kthread_worker_fn() is passed as the @threadfn
parameter.

This patch defines create_kthread_worker() and
create_kthread_worker_on_cpu() functions that hide implementation details.

They enforce using kthread_worker_fn() for the main thread. But I doubt
that there are any plans to create any alternative. In fact, I think
that we do not want any alternative main thread because it would be
hard to support consistency with the rest of the kthread worker API.

The naming and function is inspired by the workqueues API like the rest
of the kthread worker API.

Note that we need to bind per-CPU kthread workers already when they are
created. It makes the life easier. kthread_bind() could not be used later
for an already running worker.

This patch does _not_ convert existing kthread workers. The kthread worker
API need more improvements first, e.g. a function to destroy the worker.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 include/linux/kthread.h |  7 ++++
 kernel/kthread.c        | 99 ++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 96 insertions(+), 10 deletions(-)

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index e691b6a23f72..943900c7ce35 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -124,6 +124,13 @@ extern void __init_kthread_worker(struct kthread_worker *worker,
 
 int kthread_worker_fn(void *worker_ptr);
 
+__printf(1, 2)
+struct kthread_worker *
+create_kthread_worker(const char namefmt[], ...);
+
+struct kthread_worker *
+create_kthread_worker_on_cpu(int cpu, const char namefmt[]);
+
 bool queue_kthread_work(struct kthread_worker *worker,
 			struct kthread_work *work);
 void flush_kthread_work(struct kthread_work *work);
diff --git a/kernel/kthread.c b/kernel/kthread.c
index bfe8742c4217..df402e18bb5a 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -567,23 +567,24 @@ EXPORT_SYMBOL_GPL(__init_kthread_worker);
  * kthread_worker_fn - kthread function to process kthread_worker
  * @worker_ptr: pointer to initialized kthread_worker
  *
- * This function can be used as @threadfn to kthread_create() or
- * kthread_run() with @worker_ptr argument pointing to an initialized
- * kthread_worker.  The started kthread will process work_list until
- * the it is stopped with kthread_stop().  A kthread can also call
- * this function directly after extra initialization.
+ * This function implements the main cycle of kthread worker. It processes
+ * work_list until it is stopped with kthread_stop(). It sleeps when the queue
+ * is empty.
  *
- * Different kthreads can be used for the same kthread_worker as long
- * as there's only one kthread attached to it at any given time.  A
- * kthread_worker without an attached kthread simply collects queued
- * kthread_works.
+ * The works are not allowed to keep any locks, disable preemption or interrupts
+ * when they finish. There is defined a safe point for freezing when one work
+ * finishes and before a new one is started.
  */
 int kthread_worker_fn(void *worker_ptr)
 {
 	struct kthread_worker *worker = worker_ptr;
 	struct kthread_work *work;
 
-	WARN_ON(worker->task);
+	/*
+	 * FIXME: Update the check and remove the assignment when all kthread
+	 * worker users are created using create_kthread_worker*() functions.
+	 */
+	WARN_ON(worker->task && worker->task != current);
 	worker->task = current;
 repeat:
 	set_current_state(TASK_INTERRUPTIBLE);	/* mb paired w/ kthread_stop */
@@ -617,6 +618,84 @@ repeat:
 }
 EXPORT_SYMBOL_GPL(kthread_worker_fn);
 
+static struct kthread_worker *
+__create_kthread_worker(int cpu, const char namefmt[], va_list args)
+{
+	struct kthread_worker *worker;
+	struct task_struct *task;
+
+	worker = kzalloc(sizeof(*worker), GFP_KERNEL);
+	if (!worker)
+		return ERR_PTR(-ENOMEM);
+
+	init_kthread_worker(worker);
+
+	if (cpu >= 0)
+		task = kthread_create_on_cpu(kthread_worker_fn, worker,
+					     cpu, namefmt);
+	else
+		task = __kthread_create_on_node(kthread_worker_fn, worker,
+						-1, namefmt, args);
+	if (IS_ERR(task))
+		goto fail_task;
+
+	worker->task = task;
+	wake_up_process(task);
+	return worker;
+
+fail_task:
+	kfree(worker);
+	return ERR_CAST(task);
+}
+
+/**
+ * create_kthread_worker - create a kthread worker
+ * @namefmt: printf-style name for the kthread worker (task).
+ *
+ * Returns pointer to an allocated worker on success, ERR_PTR(-ENOMEM) when
+ * the needed structures could not get allocated, and ERR_PTR(-EINTR) when
+ * the worker was SIGKILLed.
+ */
+struct kthread_worker *
+create_kthread_worker(const char namefmt[], ...)
+{
+	struct kthread_worker *worker;
+	va_list args;
+
+	va_start(args, namefmt);
+	worker = __create_kthread_worker(-1, namefmt, args);
+	va_end(args);
+
+	return worker;
+}
+EXPORT_SYMBOL(create_kthread_worker);
+
+/**
+ * create_kthread_worker_on_cpu - create a kthread worker and bind it
+ *	it to a given CPU and the associated NUMA node.
+ * @cpu: CPU number
+ * @namefmt: printf-style name for the kthread worker (task).
+ *
+ * Use a valid CPU number if you want to bind the kthread worker
+ * to the given CPU and the associated NUMA node.
+ *
+ * @namefmt might include one "%d" that will get replaced by CPU number.
+ *
+ * Returns pointer to allocated worker on success, ERR_PTR when the CPU
+ * number is not valid, ERR_PTR(-ENOMEM) when the needed structures could
+ * not get allocated, ERR_PTR(-EINTR) when the worker was SIGKILLed, and
+ * ERR_PTR(-EINVAL) on invalid @cpu.
+ */
+struct kthread_worker *
+create_kthread_worker_on_cpu(int cpu, const char namefmt[])
+{
+	if (cpu < 0 || cpu > num_possible_cpus())
+		return ERR_PTR(-EINVAL);
+
+	return __create_kthread_worker(cpu, namefmt, NULL);
+}
+EXPORT_SYMBOL(create_kthread_worker_on_cpu);
+
 /* insert @work before @pos in @worker */
 static void insert_kthread_work(struct kthread_worker *worker,
 			       struct kthread_work *work,
-- 
1.8.5.6

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

#1317196 — Re: [PATCH v4 04/22] kthread: Add create_kthread_worker*()

On Mon, Jan 25, 2016 at 04:44:53PM +0100, Petr Mladek wrote:
> +struct kthread_worker *
> +create_kthread_worker_on_cpu(int cpu, const char namefmt[])
> +{
> +	if (cpu < 0 || cpu > num_possible_cpus())
> +		return ERR_PTR(-EINVAL);

Comparing cpu ID to num_possible_cpus() doesn't make any sense.  It
should either be testing against cpu_possible_mask or testing against
nr_cpu_ids.  Does this test need to be in this function at all?

Thanks.

-- 
tejun

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web