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

My intention is to make it easier to manipulate and maintain kthreads.
Especially, I want to replace all the custom main cycles with a
generic one. Also I want to make the kthreads sleep in a consistent
state in a common place when there is no work.

My first attempt was with a brand new API (iterant kthread), see
http://thread.gmane.org/gmane.linux.kernel.api/11892 . But I was
directed to improve the existing kthread worker API. This is
the 3rd iteration of the new direction.


1st patch: add support to check if a timer callback is being called

2nd..12th patches: improve the existing kthread worker API

13th..18th, 20th, 22nd patches: convert several kthreads into
      the kthread worker API, namely: khugepaged, ring buffer
      benchmark, hung_task, kmemleak, ipmi, IB/fmr_pool,
      memstick/r592, intel_powerclamp
      
21st, 23rd patches: do some preparation steps; they usually do
      some clean up that makes sense even without the conversion.

  
Changes against v2:

  + used worker->lock to synchronize the operations with the work
    instead of the PENDING bit as suggested by Tejun Heo; it simplified
    the implementation in several ways

  + added timer_active(); used it together with del_timer_sync()
    to cancel the work a less tricky way

  + removed the controversial conversion of the RCU kthreads

  + added several other examples: hung_task, kmemleak, ipmi,
    IB/fmr_pool, memstick/r592, intel_powerclamp

  + the helper fixes for the ring buffer benchmark has been improved
    as suggested by Steven; they already are in the Linus tree now

  + fixed a possible race between the check for existing khugepaged
    worker and queuing the work
 

Changes against v1:

  + remove wrappers to manipulate the scheduling policy and priority

  + remove questionable wakeup_and_destroy_kthread_worker() variant

  + do not check for chained work when draining the queue

  + allocate struct kthread worker in create_kthread_work() and
    use more simple checks for running worker

  + add support for delayed kthread works and use them instead
    of waiting inside the works

  + rework the "unrelated" fixes for the ring buffer benchmark
    as discussed in the 1st RFC; also sent separately

  + convert also the consumer in the ring buffer benchmark


I have tested this patch set against the stable Linus tree
for 4.4-rc1.

Petr Mladek (22):
  timer: Allow to check when the timer callback has not finished yet
  kthread/smpboot: Do not park in kthread_create_on_cpu()
  kthread: Allow to call __kthread_create_on_node() with va_list args
  kthread: Add create_kthread_worker*()
  kthread: Add drain_kthread_worker()
  kthread: Add destroy_kthread_worker()
  kthread: Detect when a kthread work is used by more workers
  kthread: Initial support for delayed kthread work
  kthread: Allow to cancel kthread work
  kthread: Allow to modify delayed kthread work
  kthread: Better support freezable kthread workers
  kthread: Use try_lock_kthread_work() in flush_kthread_work()
  mm/huge_page: Convert khugepaged() into kthread worker API
  ring_buffer: Convert benchmark kthreads into kthread worker API
  hung_task: Convert hungtaskd into kthread worker API
  kmemleak: Convert kmemleak kthread into kthread worker API
  ipmi: Convert kipmi kthread into kthread worker API
  IB/fmr_pool: Convert the cleanup thread into kthread worker API
  memstick/r592: Better synchronize debug messages in r592_io kthread
  memstick/r592: convert r592_io kthread into kthread worker API
  thermal/intel_powerclamp: Remove duplicated code that starts the
    kthread
  thermal/intel_powerclamp: Convert the kthread to kthread worker API

 drivers/char/ipmi/ipmi_si_intf.c     | 116 ++++---
 drivers/infiniband/core/fmr_pool.c   |  54 ++-
 drivers/memstick/host/r592.c         |  61 ++--
 drivers/memstick/host/r592.h         |   5 +-
 drivers/thermal/intel_powerclamp.c   | 302 +++++++++--------
 include/linux/kthread.h              |  56 ++++
 include/linux/timer.h                |   2 +
 kernel/hung_task.c                   |  41 ++-
 kernel/kthread.c                     | 618 +++++++++++++++++++++++++++++++----
 kernel/smpboot.c                     |   5 +
 kernel/time/timer.c                  |  24 ++
 kernel/trace/ring_buffer_benchmark.c | 133 ++++----
 mm/huge_memory.c                     | 134 ++++----
 mm/kmemleak.c                        |  86 +++--
 14 files changed, 1142 insertions(+), 495 deletions(-)

CC: Catalin Marinas <catalin.marinas@arm.com>
CC: linux-watchdog@vger.kernel.org
CC: Corey Minyard <redacted>
CC: openipmi-developer@lists.sourceforge.net
CC: Doug Ledford <redacted>
CC: Sean Hefty <redacted>
CC: Hal Rosenstock <redacted>
CC: linux-rdma@vger.kernel.org
CC: Maxim Levitsky <maximlevitsky@gmail.com>
CC: Zhang Rui <rui.zhang@intel.com>
CC: Eduardo Valentin <edubezval@gmail.com>
CC: Jacob Pan <redacted>
CC: linux-pm@vger.kernel.org

-- 
1.8.5.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

timer_pending() checks whether the list of callbacks is empty.
Each callback is removed from the list before it is called,
see call_timer_fn() in __run_timers().

Sometimes we need to make sure that the callback has finished.
For example, if we want to free some resources that are accessed
by the callback.

For this purpose, this patch adds timer_active(). It checks both
the list of callbacks and the running_timer. It takes the base_lock
to see a consistent state.

I plan to use it to implement delayed works in kthread worker.
But I guess that it will have wider use. In fact, I wonder if
timer_pending() is misused in some situations.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 include/linux/timer.h |  2 ++
 kernel/time/timer.c   | 24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/include/linux/timer.h b/include/linux/timer.h
index 61aa61dc410c..237b7c3e2b4e 100644
--- a/include/linux/timer.h
+++ b/include/linux/timer.h

@@ -165,6 +165,8 @@ static inline int timer_pending(const struct timer_list * timer)
 	return timer->entry.pprev != NULL;
 }
 
+extern int timer_active(struct timer_list *timer);
+
 extern void add_timer_on(struct timer_list *timer, int cpu);
 extern int del_timer(struct timer_list * timer);
 extern int mod_timer(struct timer_list *timer, unsigned long expires);

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index bbc5d1114583..1c16f3230771 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c

@@ -778,6 +778,30 @@ static struct tvec_base *lock_timer_base(struct timer_list *timer,
 	}
 }
 
+/**
+ * timer_active - is a timer still in use?
+ * @timer: the timer in question
+ *
+ * timer_in_use() will tell whether the timer is pending or if the callback
+ * is curretly running.
+ *
+ * Use this function if you want to make sure that some resources
+ * will not longer get accessed by the timer callback. timer_pending()
+ * is not safe in this case.
+ */
+int timer_active(struct timer_list *timer)
+{
+	struct tvec_base *base;
+	unsigned long flags;
+	int ret;
+
+	base = lock_timer_base(timer, &flags);
+	ret = timer_pending(timer) || base->running_timer == timer;
+	spin_unlock_irqrestore(&base->lock, flags);
+
+	return ret;
+}
+
 static inline int
 __mod_timer(struct timer_list *timer, unsigned long expires,
 	    bool pending_only, int pinned)

-- 
1.8.5.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

On Wed, 18 Nov 2015, Petr Mladek wrote:

timer_pending() checks whether the list of callbacks is empty.
Each callback is removed from the list before it is called,
see call_timer_fn() in __run_timers().

Sometimes we need to make sure that the callback has finished.
For example, if we want to free some resources that are accessed
by the callback.

For this purpose, this patch adds timer_active(). It checks both
the list of callbacks and the running_timer. It takes the base_lock
to see a consistent state.

I plan to use it to implement delayed works in kthread worker.
But I guess that it will have wider use. In fact, I wonder if
timer_pending() is misused in some situations.

Well. That's nice and good. But how will that new function solve
anything? After you drop the lock the state is not longer valid.
 
Thanks,

	tglx

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

On Wed 2015-11-18 23:32:28, Thomas Gleixner wrote:

On Wed, 18 Nov 2015, Petr Mladek wrote:

quoted

timer_pending() checks whether the list of callbacks is empty.
Each callback is removed from the list before it is called,
see call_timer_fn() in __run_timers().

Sometimes we need to make sure that the callback has finished.
For example, if we want to free some resources that are accessed
by the callback.

For this purpose, this patch adds timer_active(). It checks both
the list of callbacks and the running_timer. It takes the base_lock
to see a consistent state.

I plan to use it to implement delayed works in kthread worker.
But I guess that it will have wider use. In fact, I wonder if
timer_pending() is misused in some situations.

Well. That's nice and good. But how will that new function solve
anything? After you drop the lock the state is not longer valid.

If we prevent anyone from setting up the timer and timer_pending()
returns false, we are sure that the timer will stay as is.

For example, I use it in the function try_to_cancel_kthread_work().
Any manipulation with the timer is protected by worker->lock.
If the timer is not pending but still active, I have to drop
the lock and busy wait for the timer callback. See
http://thread.gmane.org/gmane.linux.kernel.mm/141493/focus=141501


Also I wonder if the following usage in
drivers/infiniband/hw/nes/nes_cm.c is safe:

static int mini_cm_dealloc_core(struct nes_cm_core *cm_core)
{
	nes_debug(NES_DBG_CM, "De-Alloc CM Core (%p)\n", cm_core);

	if (!cm_core)
		return -EINVAL;

	barrier();

	if (timer_pending(&cm_core->tcp_timer))
		del_timer(&cm_core->tcp_timer);

	destroy_workqueue(cm_core->event_wq);
	destroy_workqueue(cm_core->disconn_wq);

We destroy the workqueue but the timer callback might still
be in progress and queue new work.


There are many more locations where I see the pattern:

      if (timer_pending())
		del_timer();
      clean_up_stuff();

IMHO, we should use:

      if (timer_active())
		del_timer_sync();
      /* really safe to free stuff */
      clean_up_stuff();

or just

   del_timer_sync();
   clean_up_stuff();


I wonder if timer_pending() is used in more racy scenarios. Or maybe,
I just miss something that makes it all safe.

Thanks,
Petr

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>
---
 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

On Wed, 18 Nov 2015, Petr Mladek wrote:

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-IBi9RG/b67k@public.gmane.org>

Reviewed-by: Thomas Gleixner <redacted>

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

kthread_create_on_node() implements a bunch of logic to create
the kthread. It is already called by kthread_create_on_cpu().

We are going to extend the kthread worker API and will
need to call kthread_create_on_node() with va_list args there.

This patch does only a refactoring and does not modify the existing
behavior.

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

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 1ffc11ec5546..bfe8742c4217 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c

@@ -244,33 +244,10 @@ static void create_kthread(struct kthread_create_info *create)
 	}
 }
 
-/**
- * kthread_create_on_node - create a kthread.
- * @threadfn: the function to run until signal_pending(current).
- * @data: data ptr for @threadfn.
- * @node: task and thread structures for the thread are allocated on this node
- * @namefmt: printf-style name for the thread.
- *
- * Description: This helper function creates and names a kernel
- * thread.  The thread will be stopped: use wake_up_process() to start
- * it.  See also kthread_run().  The new thread has SCHED_NORMAL policy and
- * is affine to all CPUs.
- *
- * If thread is going to be bound on a particular cpu, give its node
- * in @node, to get NUMA affinity for kthread stack, or else give NUMA_NO_NODE.
- * When woken, the thread will run @threadfn() with @data as its
- * argument. @threadfn() can either call do_exit() directly if it is a
- * standalone thread for which no one will call kthread_stop(), or
- * return when 'kthread_should_stop()' is true (which means
- * kthread_stop() has been called).  The return value should be zero
- * or a negative error number; it will be passed to kthread_stop().
- *
- * Returns a task_struct or ERR_PTR(-ENOMEM) or ERR_PTR(-EINTR).
- */
-struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
-					   void *data, int node,
-					   const char namefmt[],
-					   ...)
+static struct task_struct *__kthread_create_on_node(int (*threadfn)(void *data),
+						    void *data, int node,
+						    const char namefmt[],
+						    va_list args)
 {
 	DECLARE_COMPLETION_ONSTACK(done);
 	struct task_struct *task;

@@ -311,11 +288,8 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
 	task = create->result;
 	if (!IS_ERR(task)) {
 		static const struct sched_param param = { .sched_priority = 0 };
-		va_list args;
 
-		va_start(args, namefmt);
 		vsnprintf(task->comm, sizeof(task->comm), namefmt, args);
-		va_end(args);
 		/*
 		 * root may have changed our (kthreadd's) priority or CPU mask.
 		 * The kernel thread should not inherit these properties.

@@ -326,6 +300,44 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
 	kfree(create);
 	return task;
 }
+
+/**
+ * kthread_create_on_node - create a kthread.
+ * @threadfn: the function to run until signal_pending(current).
+ * @data: data ptr for @threadfn.
+ * @node: task and thread structures for the thread are allocated on this node
+ * @namefmt: printf-style name for the thread.
+ *
+ * Description: This helper function creates and names a kernel
+ * thread.  The thread will be stopped: use wake_up_process() to start
+ * it.  See also kthread_run().  The new thread has SCHED_NORMAL policy and
+ * is affine to all CPUs.
+ *
+ * If thread is going to be bound on a particular cpu, give its node
+ * in @node, to get NUMA affinity for kthread stack, or else give NUMA_NO_NODE.
+ * When woken, the thread will run @threadfn() with @data as its
+ * argument. @threadfn() can either call do_exit() directly if it is a
+ * standalone thread for which no one will call kthread_stop(), or
+ * return when 'kthread_should_stop()' is true (which means
+ * kthread_stop() has been called).  The return value should be zero
+ * or a negative error number; it will be passed to kthread_stop().
+ *
+ * Returns a task_struct or ERR_PTR(-ENOMEM) or ERR_PTR(-EINTR).
+ */
+struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
+					   void *data, int node,
+					   const char namefmt[],
+					   ...)
+{
+	struct task_struct *task;
+	va_list args;
+
+	va_start(args, namefmt);
+	task = __kthread_create_on_node(threadfn, data, node, namefmt, args);
+	va_end(args);
+
+	return task;
+}
 EXPORT_SYMBOL(kthread_create_on_node);
 
 static void __kthread_bind_mask(struct task_struct *p, const struct cpumask *mask, long state)

-- 
1.8.5.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

The current kthread worker users call flush() and stop() explicitly.
This function drains the worker, stops it, and frees the kthread_worker
struct in one call.

It is supposed to be used together with create_kthread_worker*() that
allocates struct kthread_worker.

Also note that drain() correctly handles self-queuing works in compare
with flush().

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

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 943900c7ce35..c4a95a3ba500 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h

@@ -136,4 +136,6 @@ bool queue_kthread_work(struct kthread_worker *worker,
 void flush_kthread_work(struct kthread_work *work);
 void flush_kthread_worker(struct kthread_worker *worker);
 
+void destroy_kthread_worker(struct kthread_worker *worker);
+
 #endif /* _LINUX_KTHREAD_H */

diff --git a/kernel/kthread.c b/kernel/kthread.c
index a18ad3b58f61..1d41e0faef2d 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c

@@ -838,3 +838,24 @@ void drain_kthread_worker(struct kthread_worker *worker)
 	spin_unlock_irq(&worker->lock);
 }
 EXPORT_SYMBOL(drain_kthread_worker);
+
+/**
+ * destroy_kthread_worker - destroy a kthread worker
+ * @worker: worker to be destroyed
+ *
+ * Drain and destroy @worker.  It has the same conditions
+ * for use as drain_kthread_worker(), see above.
+ */
+void destroy_kthread_worker(struct kthread_worker *worker)
+{
+	struct task_struct *task;
+
+	task = worker->task;
+	if (WARN_ON(!task))
+		return;
+
+	drain_kthread_worker(worker);
+	kthread_stop(task);
+	kfree(worker);
+}
+EXPORT_SYMBOL(destroy_kthread_worker);

-- 
1.8.5.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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.

Note that we need to clear the information about the current worker
when the work is not longer used. It is important when the worker
is destroyed and later created again. For example, this is
useful when a service might get disabled and enabled via sysfs.

Also note that kthread_work_pending() function will get more
complicated once we add support for a delayed kthread work and
allow to cancel works.

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 | 45 +++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 4 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 1d41e0faef2d..378d2203c8b0 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c

@@ -563,6 +563,18 @@ void __init_kthread_worker(struct kthread_worker *worker,
 }
 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
+ *
+ * This function must be called with locked work.
+ */
+static inline bool kthread_work_pending(const struct kthread_work *work)
+{
+	return !list_empty(&work->node);
+}
+
 /**
  * kthread_worker_fn - kthread function to process kthread_worker
  * @worker_ptr: pointer to initialized kthread_worker

@@ -574,6 +586,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)
 {

@@ -610,6 +625,12 @@ repeat:
 	if (work) {
 		__set_current_state(TASK_RUNNING);
 		work->func(work);
+
+		spin_lock_irq(&worker->lock);
+		/* Allow to queue the work into another worker */
+		if (!kthread_work_pending(work))
+			work->worker = NULL;
+		spin_unlock_irq(&worker->lock);
 	} else if (!freezing(current))
 		schedule();
 

@@ -696,12 +717,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 +748,12 @@ 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.
  */
 bool queue_kthread_work(struct kthread_worker *worker,
 			struct kthread_work *work)

@@ -725,7 +762,7 @@ bool queue_kthread_work(struct kthread_worker *worker,
 	unsigned long flags;
 
 	spin_lock_irqsave(&worker->lock, flags);
-	if (list_empty(&work->node)) {
+	if (!kthread_work_pending(work)) {
 		insert_kthread_work(worker, work, &worker->work_list);
 		ret = true;
 	}

-- 
1.8.5.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Hello,

On Wed, Nov 18, 2015 at 02:25:12PM +0100, Petr Mladek wrote:

Doesn't this mean that the work item can't be freed from its callback?
That pattern tends to happen regularly.

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

On Mon 2015-11-23 17:27:03, Tejun Heo wrote:

Hello,

On Wed, Nov 18, 2015 at 02:25:12PM +0100, Petr Mladek wrote:

quoted

@@ -610,6 +625,12 @@ repeat:
 	if (work) {
 		__set_current_state(TASK_RUNNING);
 		work->func(work);
+
+		spin_lock_irq(&worker->lock);
+		/* Allow to queue the work into another worker */
+		if (!kthread_work_pending(work))
+			work->worker = NULL;
+		spin_unlock_irq(&worker->lock);

Doesn't this mean that the work item can't be freed from its callback?
That pattern tends to happen regularly.

I am not sure if I understand your question. Do you mean switching
work->func during the life time of the struct kthread_work? This
should not be affected by the above code.

The above code allows to queue an _unused_ kthread_work into any
kthread_worker. For example, it is needed for khugepaged,
see http://marc.info/?l=linux-kernel&m=144785344924871&w=2
The work is static but the worker can be started/stopped
(allocated/freed) repeatedly. It means that the work need
to be usable with many workers. But it is associated only
with one worker when being used.

If the work is in use (pending or being proceed), we must not
touch work->worker. Otherwise there might be a race. Because
all the operations with the work are synchronized using
work->worker->lock.

I hope that it makes sense.

Thanks a lot for feedback,
Petr

Hello, Petr.

On Tue, Nov 24, 2015 at 11:06:50AM +0100, Petr Mladek wrote:

quoted

quoted

@@ -610,6 +625,12 @@ repeat:
 	if (work) {
 		__set_current_state(TASK_RUNNING);
 		work->func(work);
+
+		spin_lock_irq(&worker->lock);
+		/* Allow to queue the work into another worker */
+		if (!kthread_work_pending(work))
+			work->worker = NULL;
+		spin_unlock_irq(&worker->lock);

Doesn't this mean that the work item can't be freed from its callback?
That pattern tends to happen regularly.

I am not sure if I understand your question. Do you mean switching
work->func during the life time of the struct kthread_work? This
should not be affected by the above code.

So, something like the following.

void my_work_fn(work)
{
	struct my_struct *s = container_of(work, ...);

	do something with s;
	kfree(s);
}

and the queuer does

	struct my_struct *s = kmalloc(sizeof(*s));

	init s and s->work;
	queue(&s->work);

expecting s to be freed on completion.  IOW, you can't expect the work
item to remain accessible once the work function starts executing.

The above code allows to queue an _unused_ kthread_work into any
kthread_worker. For example, it is needed for khugepaged,
see http://marc.info/?l=linux-kernel&m=144785344924871&w=2
The work is static but the worker can be started/stopped
(allocated/freed) repeatedly. It means that the work need
to be usable with many workers. But it is associated only
with one worker when being used.

It can just re-init work items when it restarts workers, right?

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

On Tue 2015-11-24 09:49:42, Tejun Heo wrote:

Hello, Petr.

On Tue, Nov 24, 2015 at 11:06:50AM +0100, Petr Mladek wrote:

quoted

quoted

quoted

@@ -610,6 +625,12 @@ repeat:
 	if (work) {
 		__set_current_state(TASK_RUNNING);
 		work->func(work);
+
+		spin_lock_irq(&worker->lock);
+		/* Allow to queue the work into another worker */
+		if (!kthread_work_pending(work))
+			work->worker = NULL;
+		spin_unlock_irq(&worker->lock);

Doesn't this mean that the work item can't be freed from its callback?
That pattern tends to happen regularly.

I am not sure if I understand your question. Do you mean switching
work->func during the life time of the struct kthread_work? This
should not be affected by the above code.

IOW, you can't expect the work
item to remain accessible once the work function starts executing.

I see, I was not aware of this pattern.

quoted

The above code allows to queue an _unused_ kthread_work into any
kthread_worker. For example, it is needed for khugepaged,
see http://marc.info/?l=linux-kernel&m=144785344924871&w=2
The work is static but the worker can be started/stopped
(allocated/freed) repeatedly. It means that the work need
to be usable with many workers. But it is associated only
with one worker when being used.

It can just re-init work items when it restarts workers, right?

Yes, this would work. It might be slightly inconvenient but
it looks like a good compromise. It helps to keep the API
implementation rather simple and rather secure.

Alternatively, we could allow to queue the work on another worker
if it is not pending. But then we would need to check the pending
status without the worker->lock because work->worker might point
to an already freed worker. We need to check the pending
status in many situations. It might open a can of worms that
I probably do not want to catch.

Thank you and PeterZ for explanation,
Petr

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

On Tue, Nov 24, 2015 at 11:06:50AM +0100, Petr Mladek wrote:

On Mon 2015-11-23 17:27:03, Tejun Heo wrote:

quoted

Hello,

On Wed, Nov 18, 2015 at 02:25:12PM +0100, Petr Mladek wrote:

quoted

@@ -610,6 +625,12 @@ repeat:
 	if (work) {
 		__set_current_state(TASK_RUNNING);
 		work->func(work);
+
+		spin_lock_irq(&worker->lock);
+		/* Allow to queue the work into another worker */
+		if (!kthread_work_pending(work))
+			work->worker = NULL;
+		spin_unlock_irq(&worker->lock);

Doesn't this mean that the work item can't be freed from its callback?
That pattern tends to happen regularly.

I am not sure if I understand your question. Do you mean switching
work->func during the life time of the struct kthread_work? This
should not be affected by the above code.

No, work->func(work) doing: kfree(work).

That is indeed something quite frequently done, and since you now have
references to work after calling func, things would go *boom* rather
quickly.

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. A problem is when
the timer callback is running at the same time. In this case, we need
to release the lock to avoid a deadlock and start from the beginning.

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        | 142 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 146 insertions(+)

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 0f4b348c2c7e..d12aa91cc44d 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));
 }
 

@@ -950,6 +952,146 @@ 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
+ *  -EAGAIN	if the lock was dropped. The caller is supposed to
+ *		take the lock again and repeat the operation.
+ */
+static int
+try_to_cancel_kthread_work(struct kthread_work *work,
+				   spinlock_t *lock,
+				   unsigned long *flags)
+{
+	int ret = 0;
+
+	if (work->timer) {
+		/* Try to cancel the timer if pending. */
+		if (del_timer(work->timer)) {
+			ret = 1;
+			goto out;
+		}
+
+		/* Are we racing with the timer callback? */
+		if (timer_active(work->timer)) {
+			/* Bad luck, need to avoid a deadlock. */
+			spin_unlock_irqrestore(lock, *flags);
+			del_timer_sync(work->timer);
+			ret = -EAGAIN;
+			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;
+
+try_again:
+	local_irq_save(flags);
+	if (!try_lock_kthread_work(work)) {
+		local_irq_restore(flags);
+		ret = 0;
+		goto out;
+	}
+	worker = work->worker;
+
+	ret = try_to_cancel_kthread_work(work, &worker->lock, &flags);
+	if (ret == -EAGAIN)
+		goto try_again;
+
+	if (worker->current_work != work)
+		goto out_fast;
+
+	/*
+	 * Need to wait until the work finished. Block queueing
+	 * in the meantime.
+	 */
+	work->canceling++;
+	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);
+	work->canceling--;
+
+out_fast:
+	/*
+	 * Allow to queue the work into another worker if there is no other
+	 * pending operation.
+	 */
+	if (!work->canceling)
+		work->worker = NULL;
+	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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Hello,

On Wed, Nov 18, 2015 at 02:25:14PM +0100, Petr Mladek wrote:

+static int
+try_to_cancel_kthread_work(struct kthread_work *work,
+				   spinlock_t *lock,
+				   unsigned long *flags)
+{
+	int ret = 0;
+
+	if (work->timer) {
+		/* Try to cancel the timer if pending. */
+		if (del_timer(work->timer)) {
+			ret = 1;
+			goto out;
+		}
+
+		/* Are we racing with the timer callback? */
+		if (timer_active(work->timer)) {
+			/* Bad luck, need to avoid a deadlock. */
+			spin_unlock_irqrestore(lock, *flags);
+			del_timer_sync(work->timer);
+			ret = -EAGAIN;
+			goto out;
+		}

As the timer side is already kinda trylocking anyway, can't the cancel
path be made simpler?  Sth like

	lock(worker);
	work->canceling = true;
	del_timer_sync(work->timer);
	unlock(worker);

And the timer can do (ignoring the multiple worker support, do we even
need that?)

	while (!trylock(worker)) {
		if (work->canceling)
			return;
		cpu_relax();
	}
	queue;
	unlock(worker);

Thanks.

-- 
tejun

On Mon 2015-11-23 17:58:23, Tejun Heo wrote:

Hello,

On Wed, Nov 18, 2015 at 02:25:14PM +0100, Petr Mladek wrote:

quoted

+static int
+try_to_cancel_kthread_work(struct kthread_work *work,
+				   spinlock_t *lock,
+				   unsigned long *flags)
+{
+	int ret = 0;
+
+	if (work->timer) {
+		/* Try to cancel the timer if pending. */
+		if (del_timer(work->timer)) {
+			ret = 1;
+			goto out;
+		}
+
+		/* Are we racing with the timer callback? */
+		if (timer_active(work->timer)) {
+			/* Bad luck, need to avoid a deadlock. */
+			spin_unlock_irqrestore(lock, *flags);
+			del_timer_sync(work->timer);
+			ret = -EAGAIN;
+			goto out;
+		}

As the timer side is already kinda trylocking anyway, can't the cancel
path be made simpler?  Sth like

	lock(worker);
	work->canceling = true;
	del_timer_sync(work->timer);
	unlock(worker);

And the timer can do (ignoring the multiple worker support, do we even
need that?)

	while (!trylock(worker)) {
		if (work->canceling)
			return;
		cpu_relax();
	}
	queue;
	unlock(worker);

Why did I not find out this myself ?:-)

Thanks for hint,
Petr

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

On Mon, Nov 23, 2015 at 2:58 PM, Tejun Heo [off-list ref] wrote:

And the timer can do (ignoring the multiple worker support, do we even
need that?)

        while (!trylock(worker)) {
                if (work->canceling)
                        return;
                cpu_relax();
        }

No no no!

People, you need to learn that code like the above is *not*
acceptable. It's busy-looping on a spinlock, and constantly trying to
*write* to the spinlock.

It will literally crater performance on a multi-socket SMP system if
it ever triggers. We're talking 10x slowdowns, and absolutely
unacceptable cache coherency traffic.

These kinds of loops absolutely *have* to have the read-only part. The
"cpu_relax()" above needs to be a loop that just tests the lock state
by *reading* it, so the cpu_relax() needs to be replaced with
something like

        while (spin_is_locked(lock)) cpu_relax();

instead (possibly just "spin_unlock_wait()" - but the explicit loop
might be worth it if you then want to check the "canceling" flag
independently of the lock state too).

In general, it's very dangerous to try to cook up your own locking
rules. People *always* get it wrong.

                      Linus

Hello,

On Tue, Nov 24, 2015 at 12:23:53PM -0800, Linus Torvalds wrote:

instead (possibly just "spin_unlock_wait()" - but the explicit loop

I see.  Wasn't thinking about cache traffic.  Yeah, spin_unlock_wait()
seems a lot better.

might be worth it if you then want to check the "canceling" flag
independently of the lock state too).

In general, it's very dangerous to try to cook up your own locking
rules. People *always* get it wrong.

It's either trylock on timer side or timer active spinning trick on
canceling side, so this seems the lesser of the two evils.

Thanks.

-- 
tejun

On Tue, Nov 24, 2015 at 12:28 PM, Tejun Heo [off-list ref] wrote:

quoted

In general, it's very dangerous to try to cook up your own locking
rules. People *always* get it wrong.

It's either trylock on timer side or timer active spinning trick on
canceling side, so this seems the lesser of the two evils.

I'm not saying the approach is wrong.

I'm saying that people need to realize that locking is harder than
they think, and not cook up their own lock primitives using things
like trylock without really thinking about it a *lot*.

Basically, "trylock()" on its own should never be used in a loop. The
main use for trylock should be one of:

 - thing that you can just not do at all if you can't get the lock

 - avoiding ABBA deadlocks: if you have a A->B locking order, but you
already hold B, instead of "drop B, then take A and B in the right
order", you may decide to first "trylock(A)" - and if that fails you
then fall back on the "drop and relock in the right order".

but if what you want to create is a "get lock using trylock", you need
to be very aware of the cache coherency traffic issue at least.

It is possible that we should think about trying to introduce a new
primitive for that "loop_try_lock()" thing. But it's probably not
common enough to be worth it - we've had this issue before, but I
think it's a "once every couple of years" kind of thing rather than
anything that we need to worry about.

The "locking is hard" issue is very real, though. We've traditionally
had a *lot* of code that tried to do its own locking, and not getting
the memory ordering right etc. Things that happen to work on x86 but
don't on other architectures etc.

                       Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

There are situations when we need to modify the delay of a delayed kthread
work. For example, when the work depends on an event and the initial delay
means a timeout. Then we want to queue the work immediately when the event
happens.

This patch implements mod_delayed_kthread_work() as inspired workqueues.
It tries to cancel the pending work and queue it again with the
given timeout.

A very special case is when the work is being canceled at the same time.
cancel_*kthread_work_sync() operation blocks queuing until the running
work finishes. Therefore we do nothing and let cancel() win. This should
not normally happen as the caller is supposed to synchronize these
operations a reasonable way.

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

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index dd2a587a2bd7..f501dfeaa0e3 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h

@@ -168,6 +168,10 @@ bool queue_delayed_kthread_work(struct kthread_worker *worker,
 				struct delayed_kthread_work *dwork,
 				unsigned long delay);
 
+bool mod_delayed_kthread_work(struct kthread_worker *worker,
+			      struct delayed_kthread_work *dwork,
+			      unsigned long delay);
+
 void flush_kthread_work(struct kthread_work *work);
 void flush_kthread_worker(struct kthread_worker *worker);
 

diff --git a/kernel/kthread.c b/kernel/kthread.c
index d12aa91cc44d..4c3b845c719e 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c

@@ -1003,6 +1003,56 @@ out:
 	return ret;
 }
 
+/**
+ * mod_delayed_kthread_work - modify delay of or queue a delayed kthread work
+ * @worker: kthread worker to use
+ * @dwork: delayed kthread work to queue
+ * @delay: number of jiffies to wait before queuing
+ *
+ * If @dwork is idle, equivalent to queue_delayed_kthread work(). Otherwise,
+ * modify @dwork's timer so that it expires after @delay. If @delay is zero,
+ * @work is guaranteed to be queued immediately.
+ *
+ * Return: %false if @dwork was idle and queued. Return %true if @dwork was
+ * pending and its timer was modified.
+ *
+ * A special case is when cancel_work_sync() is running in parallel.
+ * It blocks further queuing. We let the cancel() win and return %false.
+ * The caller is supposed to synchronize these operations a reasonable way.
+ *
+ * This function is safe to call from any context including IRQ handler.
+ * See try_to_grab_pending_kthread_work() for details.
+ */
+bool mod_delayed_kthread_work(struct kthread_worker *worker,
+			      struct delayed_kthread_work *dwork,
+			      unsigned long delay)
+{
+	struct kthread_work *work = &dwork->work;
+	unsigned long flags;
+	int ret = 0;
+
+try_again:
+	spin_lock_irqsave(&worker->lock, flags);
+	WARN_ON_ONCE(work->worker && work->worker != worker);
+
+	if (work->canceling)
+		goto out;
+
+	ret = try_to_cancel_kthread_work(work, &worker->lock, &flags);
+	if (ret == -EAGAIN)
+		goto try_again;
+
+	if (work->canceling)
+		ret = 0;
+	else
+		__queue_delayed_kthread_work(worker, dwork, delay);
+
+out:
+	spin_unlock_irqrestore(&worker->lock, flags);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(mod_delayed_kthread_work);
+
 static bool __cancel_kthread_work_sync(struct kthread_work *work)
 {
 	struct kthread_worker *worker;

-- 
1.8.5.6

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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);