[PATCHSET] kthread_worker: reimplement flush_kthread_work() to allow freeing during execution

[PATCHSET] kthread_worker: reimplement flush_kthread_work() to allow freeing during execution

[Tejun Heo]

Hello,

kthread_worker was introduced together with concurrency managed
workqueue to serve workqueue users which need a special dedicated
worker - e.g. RT scheduling.  This is minimal queue / flush / flush
all iterface on top of kthread and each provided interface matches the
workqueue counterpart so that switching isn't difficult.

However, one noticeable difference was that kthread_worker doesn't
allow a work item to be freed while being executed.  The intention was
to keep the code simpler but it didn't really and the restriction is
subtle and does prevent some valid use cases.

This two-patch series reimplements flush_kthread_work() so that it
uses an extra work item for flushing.  While this takes a bit more
lines, this is easier to understand and removes the annoying
difference.

This patchset contains the following two patches.

 0001-kthread_worker-reorganize-to-prepare-for-flush_kthre.patch
 0002-kthread_worker-reimplement-flush_kthread_work-to-all.patch

The first one is a prep patch which makes no functional changes.  The
second reimplements flush_kthread_work().

All current kthread_worker users are cc'd.  If no one objects, I'll
push it through the workqueue branch.  This patchset is also available
in the following git branch.

 git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git review-kthread_worker-flush

diffstat follows.  Thanks.

 include/linux/kthread.h |    8 +---
 kernel/kthread.c        |   86 +++++++++++++++++++++++++++---------------------
 2 files changed, 52 insertions(+), 42 deletions(-)

-- 
tejun

[PATCH 1/2] kthread_worker: reorganize to prepare for flush_kthread_work() reimplementation

[Tejun Heo]

>From c9bba34243a86fb3ac82d1bdd0ce4bf796b79559 Mon Sep 17 00:00:00 2001
From: Tejun Heo <tj@kernel.org>
Date: Thu, 19 Jul 2012 13:52:53 -0700

Make the following two non-functional changes.

* Separate out insert_kthread_work() from queue_kthread_work().

* Relocate struct kthread_flush_work and kthread_flush_work_fn()
  definitions above flush_kthread_work().

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 kernel/kthread.c |   40 ++++++++++++++++++++++++----------------
 1 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 3d3de63..7b8a678 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -378,6 +378,17 @@ repeat:
 }
 EXPORT_SYMBOL_GPL(kthread_worker_fn);
 
+/* insert @work before @pos in @worker */
+static void insert_kthread_work(struct kthread_worker *worker,
+			       struct kthread_work *work,
+			       struct list_head *pos)
+{
+	list_add_tail(&work->node, pos);
+	work->queue_seq++;
+	if (likely(worker->task))
+		wake_up_process(worker->task);
+}
+
 /**
  * queue_kthread_work - queue a kthread_work
  * @worker: target kthread_worker
@@ -395,10 +406,7 @@ bool queue_kthread_work(struct kthread_worker *worker,
 
 	spin_lock_irqsave(&worker->lock, flags);
 	if (list_empty(&work->node)) {
-		list_add_tail(&work->node, &worker->work_list);
-		work->queue_seq++;
-		if (likely(worker->task))
-			wake_up_process(worker->task);
+		insert_kthread_work(worker, work, &worker->work_list);
 		ret = true;
 	}
 	spin_unlock_irqrestore(&worker->lock, flags);
@@ -406,6 +414,18 @@ bool queue_kthread_work(struct kthread_worker *worker,
 }
 EXPORT_SYMBOL_GPL(queue_kthread_work);
 
+struct kthread_flush_work {
+	struct kthread_work	work;
+	struct completion	done;
+};
+
+static void kthread_flush_work_fn(struct kthread_work *work)
+{
+	struct kthread_flush_work *fwork =
+		container_of(work, struct kthread_flush_work, work);
+	complete(&fwork->done);
+}
+
 /**
  * flush_kthread_work - flush a kthread_work
  * @work: work to flush
@@ -436,18 +456,6 @@ void flush_kthread_work(struct kthread_work *work)
 }
 EXPORT_SYMBOL_GPL(flush_kthread_work);
 
-struct kthread_flush_work {
-	struct kthread_work	work;
-	struct completion	done;
-};
-
-static void kthread_flush_work_fn(struct kthread_work *work)
-{
-	struct kthread_flush_work *fwork =
-		container_of(work, struct kthread_flush_work, work);
-	complete(&fwork->done);
-}
-
 /**
  * flush_kthread_worker - flush all current works on a kthread_worker
  * @worker: worker to flush
-- 
1.7.7.3

Re: [PATCH 1/2] kthread_worker: reorganize to prepare for flush_kthread_work() reimplementation

[Andy Walls]

On Thu, 2012-07-19 at 14:15 -0700, Tejun Heo wrote:
> From c9bba34243a86fb3ac82d1bdd0ce4bf796b79559 Mon Sep 17 00:00:00 2001
> From: Tejun Heo <tj@kernel.org>
> Date: Thu, 19 Jul 2012 13:52:53 -0700
> 
> Make the following two non-functional changes.
> 
> * Separate out insert_kthread_work() from queue_kthread_work().
> 
> * Relocate struct kthread_flush_work and kthread_flush_work_fn()
>   definitions above flush_kthread_work().
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  kernel/kthread.c |   40 ++++++++++++++++++++++++----------------
>  1 files changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index 3d3de63..7b8a678 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -378,6 +378,17 @@ repeat:
>  }
>  EXPORT_SYMBOL_GPL(kthread_worker_fn);
>  
> +/* insert @work before @pos in @worker */

Hi Tejun,

Would a comment that the caller should be holding worker->lock be useful
here?  Anyway, comment or not:

Acked-by: Andy Walls <awall@md.metrocast.net>

Regards,
Andy

> +static void insert_kthread_work(struct kthread_worker *worker,
> +			       struct kthread_work *work,
> +			       struct list_head *pos)
> +{
> +	list_add_tail(&work->node, pos);
> +	work->queue_seq++;
> +	if (likely(worker->task))
> +		wake_up_process(worker->task);
> +}
> +
>  /**
>   * queue_kthread_work - queue a kthread_work
>   * @worker: target kthread_worker
> @@ -395,10 +406,7 @@ bool queue_kthread_work(struct kthread_worker *worker,
>  
>  	spin_lock_irqsave(&worker->lock, flags);
>  	if (list_empty(&work->node)) {
> -		list_add_tail(&work->node, &worker->work_list);
> -		work->queue_seq++;
> -		if (likely(worker->task))
> -			wake_up_process(worker->task);
> +		insert_kthread_work(worker, work, &worker->work_list);
>  		ret = true;
>  	}
>  	spin_unlock_irqrestore(&worker->lock, flags);
> @@ -406,6 +414,18 @@ bool queue_kthread_work(struct kthread_worker *worker,
>  }
>  EXPORT_SYMBOL_GPL(queue_kthread_work);
>  
> +struct kthread_flush_work {
> +	struct kthread_work	work;
> +	struct completion	done;
> +};
> +
> +static void kthread_flush_work_fn(struct kthread_work *work)
> +{
> +	struct kthread_flush_work *fwork =
> +		container_of(work, struct kthread_flush_work, work);
> +	complete(&fwork->done);
> +}
> +
>  /**
>   * flush_kthread_work - flush a kthread_work
>   * @work: work to flush
> @@ -436,18 +456,6 @@ void flush_kthread_work(struct kthread_work *work)
>  }
>  EXPORT_SYMBOL_GPL(flush_kthread_work);
>  
> -struct kthread_flush_work {
> -	struct kthread_work	work;
> -	struct completion	done;
> -};
> -
> -static void kthread_flush_work_fn(struct kthread_work *work)
> -{
> -	struct kthread_flush_work *fwork =
> -		container_of(work, struct kthread_flush_work, work);
> -	complete(&fwork->done);
> -}
> -
>  /**
>   * flush_kthread_worker - flush all current works on a kthread_worker
>   * @worker: worker to flush

Re: [PATCH 1/2] kthread_worker: reorganize to prepare for flush_kthread_work() reimplementation

[Tejun Heo]

Hello,

On Sat, Jul 21, 2012 at 01:13:27PM -0400, Andy Walls wrote:
> > +/* insert @work before @pos in @worker */
> 
> Hi Tejun,
> 
> Would a comment that the caller should be holding worker->lock be useful
> here?  Anyway, comment or not:
> 
> Acked-by: Andy Walls <awall@md.metrocast.net>

Will add lockdep_assert_held().  Thanks!

-- 
tejun

Re: [PATCH 1/2] kthread_worker: reorganize to prepare for flush_kthread_work() reimplementation

[Andy Walls]

On Sun, 2012-07-22 at 09:46 -0700, Tejun Heo wrote:
> Hello,
> 
> On Sat, Jul 21, 2012 at 01:13:27PM -0400, Andy Walls wrote:
> > > +/* insert @work before @pos in @worker */
> > 
> > Hi Tejun,
> > 
> > Would a comment that the caller should be holding worker->lock be useful
> > here?  Anyway, comment or not:
> > 
> > Acked-by: Andy Walls <awall@md.metrocast.net>
> 
> Will add lockdep_assert_held().  Thanks!
> 

Great!  Thank you.

Regards,
Andy 

[PATCH UPDATED 1/2] kthread_worker: reorganize to prepare for flush_kthread_work() reimplementation

[Tejun Heo]

>From 9a2e03d8ed518a61154f18d83d6466628e519f94 Mon Sep 17 00:00:00 2001
From: Tejun Heo <tj@kernel.org>
Date: Thu, 19 Jul 2012 13:52:53 -0700

Make the following two non-functional changes.

* Separate out insert_kthread_work() from queue_kthread_work().

* Relocate struct kthread_flush_work and kthread_flush_work_fn()
  definitions above flush_kthread_work().

v2: Added lockdep_assert_held() in insert_kthread_work() as suggested
    by Andy Walls.

Signed-off-by: Tejun Heo <tj@kernel.org>
Acked-by: Andy Walls <awalls@md.metrocast.net>
---
 kernel/kthread.c |   42 ++++++++++++++++++++++++++----------------
 1 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/kernel/kthread.c b/kernel/kthread.c
index 3d3de63..4bfbff3 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -378,6 +378,19 @@ repeat:
 }
 EXPORT_SYMBOL_GPL(kthread_worker_fn);
 
+/* insert @work before @pos in @worker */
+static void insert_kthread_work(struct kthread_worker *worker,
+			       struct kthread_work *work,
+			       struct list_head *pos)
+{
+	lockdep_assert_held(&worker->lock);
+
+	list_add_tail(&work->node, pos);
+	work->queue_seq++;
+	if (likely(worker->task))
+		wake_up_process(worker->task);
+}
+
 /**
  * queue_kthread_work - queue a kthread_work
  * @worker: target kthread_worker
@@ -395,10 +408,7 @@ bool queue_kthread_work(struct kthread_worker *worker,
 
 	spin_lock_irqsave(&worker->lock, flags);
 	if (list_empty(&work->node)) {
-		list_add_tail(&work->node, &worker->work_list);
-		work->queue_seq++;
-		if (likely(worker->task))
-			wake_up_process(worker->task);
+		insert_kthread_work(worker, work, &worker->work_list);
 		ret = true;
 	}
 	spin_unlock_irqrestore(&worker->lock, flags);
@@ -406,6 +416,18 @@ bool queue_kthread_work(struct kthread_worker *worker,
 }
 EXPORT_SYMBOL_GPL(queue_kthread_work);
 
+struct kthread_flush_work {
+	struct kthread_work	work;
+	struct completion	done;
+};
+
+static void kthread_flush_work_fn(struct kthread_work *work)
+{
+	struct kthread_flush_work *fwork =
+		container_of(work, struct kthread_flush_work, work);
+	complete(&fwork->done);
+}
+
 /**
  * flush_kthread_work - flush a kthread_work
  * @work: work to flush
@@ -436,18 +458,6 @@ void flush_kthread_work(struct kthread_work *work)
 }
 EXPORT_SYMBOL_GPL(flush_kthread_work);
 
-struct kthread_flush_work {
-	struct kthread_work	work;
-	struct completion	done;
-};
-
-static void kthread_flush_work_fn(struct kthread_work *work)
-{
-	struct kthread_flush_work *fwork =
-		container_of(work, struct kthread_flush_work, work);
-	complete(&fwork->done);
-}
-
 /**
  * flush_kthread_worker - flush all current works on a kthread_worker
  * @worker: worker to flush
-- 
1.7.7.3

[PATCH 2/2] kthread_worker: reimplement flush_kthread_work() to allow freeing the work item being executed

[Tejun Heo]

>From 06f9a06f4aeecdb9d07014713ab41b548ae219b5 Mon Sep 17 00:00:00 2001
From: Tejun Heo <tj@kernel.org>
Date: Thu, 19 Jul 2012 13:52:53 -0700

kthread_worker provides minimalistic workqueue-like interface for
users which need a dedicated worker thread (e.g. for realtime
priority).  It has basic queue, flush_work, flush_worker operations
which mostly match the workqueue counterparts; however, due to the way
flush_work() is implemented, it has a noticeable difference of not
allowing work items to be freed while being executed.

While the current users of kthread_worker are okay with the current
behavior, the restriction does impede some valid use cases.  Also,
removing this difference isn't difficult and actually makes the code
easier to understand.

This patch reimplements flush_kthread_work() such that it uses a
flush_work item instead of queue/done sequence numbers.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
 include/linux/kthread.h |    8 +-----
 kernel/kthread.c        |   48 ++++++++++++++++++++++++++--------------------
 2 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/include/linux/kthread.h b/include/linux/kthread.h
index 0714b24..22ccf9d 100644
--- a/include/linux/kthread.h
+++ b/include/linux/kthread.h
@@ -49,8 +49,6 @@ extern int tsk_fork_get_node(struct task_struct *tsk);
  * can be queued and flushed using queue/flush_kthread_work()
  * respectively.  Queued kthread_works are processed by a kthread
  * running kthread_worker_fn().
- *
- * A kthread_work can't be freed while it is executing.
  */
 struct kthread_work;
 typedef void (*kthread_work_func_t)(struct kthread_work *work);
@@ -59,15 +57,14 @@ struct kthread_worker {
 	spinlock_t		lock;
 	struct list_head	work_list;
 	struct task_struct	*task;
+	struct kthread_work	*current_work;
 };
 
 struct kthread_work {
 	struct list_head	node;
 	kthread_work_func_t	func;
 	wait_queue_head_t	done;
-	atomic_t		flushing;
-	int			queue_seq;
-	int			done_seq;
+	struct kthread_worker	*worker;
 };
 
 #define KTHREAD_WORKER_INIT(worker)	{				\
@@ -79,7 +76,6 @@ struct kthread_work {
 	.node = LIST_HEAD_INIT((work).node),				\
 	.func = (fn),							\
 	.done = __WAIT_QUEUE_HEAD_INITIALIZER((work).done),		\
-	.flushing = ATOMIC_INIT(0),					\
 	}
 
 #define DEFINE_KTHREAD_WORKER(worker)					\
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 7b8a678..4034b2b 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -360,16 +360,12 @@ repeat:
 					struct kthread_work, node);
 		list_del_init(&work->node);
 	}
+	worker->current_work = work;
 	spin_unlock_irq(&worker->lock);
 
 	if (work) {
 		__set_current_state(TASK_RUNNING);
 		work->func(work);
-		smp_wmb();	/* wmb worker-b0 paired with flush-b1 */
-		work->done_seq = work->queue_seq;
-		smp_mb();	/* mb worker-b1 paired with flush-b0 */
-		if (atomic_read(&work->flushing))
-			wake_up_all(&work->done);
 	} else if (!freezing(current))
 		schedule();
 
@@ -384,7 +380,7 @@ static void insert_kthread_work(struct kthread_worker *worker,
 			       struct list_head *pos)
 {
 	list_add_tail(&work->node, pos);
-	work->queue_seq++;
+	work->worker = worker;
 	if (likely(worker->task))
 		wake_up_process(worker->task);
 }
@@ -434,25 +430,35 @@ static void kthread_flush_work_fn(struct kthread_work *work)
  */
 void flush_kthread_work(struct kthread_work *work)
 {
-	int seq = work->queue_seq;
+	struct kthread_flush_work fwork = {
+		KTHREAD_WORK_INIT(fwork.work, kthread_flush_work_fn),
+		COMPLETION_INITIALIZER_ONSTACK(fwork.done),
+	};
+	struct kthread_worker *worker;
+	bool noop = false;
+
+retry:
+	worker = work->worker;
+	if (!worker)
+		return;
 
-	atomic_inc(&work->flushing);
+	spin_lock_irq(&worker->lock);
+	if (work->worker != worker) {
+		spin_unlock_irq(&worker->lock);
+		goto retry;
+	}
 
-	/*
-	 * mb flush-b0 paired with worker-b1, to make sure either
-	 * worker sees the above increment or we see done_seq update.
-	 */
-	smp_mb__after_atomic_inc();
+	if (!list_empty(&work->node))
+		insert_kthread_work(worker, &fwork.work, work->node.next);
+	else if (worker->current_work == work)
+		insert_kthread_work(worker, &fwork.work, worker->work_list.next);
+	else
+		noop = true;
 
-	/* A - B <= 0 tests whether B is in front of A regardless of overflow */
-	wait_event(work->done, seq - work->done_seq <= 0);
-	atomic_dec(&work->flushing);
+	spin_unlock_irq(&worker->lock);
 
-	/*
-	 * rmb flush-b1 paired with worker-b0, to make sure our caller
-	 * sees every change made by work->func().
-	 */
-	smp_mb__after_atomic_dec();
+	if (!noop)
+		wait_for_completion(&fwork.done);
 }
 EXPORT_SYMBOL_GPL(flush_kthread_work);
 
-- 
1.7.7.3

Re: [PATCH 2/2] kthread_worker: reimplement flush_kthread_work() to allow freeing the work item being executed

[Andy Walls]

On Thu, 2012-07-19 at 14:16 -0700, Tejun Heo wrote:
> From 06f9a06f4aeecdb9d07014713ab41b548ae219b5 Mon Sep 17 00:00:00 2001
> From: Tejun Heo <tj@kernel.org>
> Date: Thu, 19 Jul 2012 13:52:53 -0700
> 
> kthread_worker provides minimalistic workqueue-like interface for
> users which need a dedicated worker thread (e.g. for realtime
> priority).  It has basic queue, flush_work, flush_worker operations
> which mostly match the workqueue counterparts; however, due to the way
> flush_work() is implemented, it has a noticeable difference of not
> allowing work items to be freed while being executed.
> 
> While the current users of kthread_worker are okay with the current
> behavior, the restriction does impede some valid use cases.  Also,
> removing this difference isn't difficult and actually makes the code
> easier to understand.
> 
> This patch reimplements flush_kthread_work() such that it uses a
> flush_work item instead of queue/done sequence numbers.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
>  include/linux/kthread.h |    8 +-----
>  kernel/kthread.c        |   48 ++++++++++++++++++++++++++--------------------
>  2 files changed, 29 insertions(+), 27 deletions(-)

Hi Tejun,

I have a question and comment below.

> diff --git a/include/linux/kthread.h b/include/linux/kthread.h
> index 0714b24..22ccf9d 100644
> --- a/include/linux/kthread.h
> +++ b/include/linux/kthread.h
> @@ -49,8 +49,6 @@ extern int tsk_fork_get_node(struct task_struct *tsk);
>   * can be queued and flushed using queue/flush_kthread_work()
>   * respectively.  Queued kthread_works are processed by a kthread
>   * running kthread_worker_fn().
> - *
> - * A kthread_work can't be freed while it is executing.
>   */
>  struct kthread_work;
>  typedef void (*kthread_work_func_t)(struct kthread_work *work);
> @@ -59,15 +57,14 @@ struct kthread_worker {
>  	spinlock_t		lock;
>  	struct list_head	work_list;
>  	struct task_struct	*task;
> +	struct kthread_work	*current_work;
>  };
>  
>  struct kthread_work {
>  	struct list_head	node;
>  	kthread_work_func_t	func;
>  	wait_queue_head_t	done;
> -	atomic_t		flushing;
> -	int			queue_seq;
> -	int			done_seq;
> +	struct kthread_worker	*worker;
>  };
>  
>  #define KTHREAD_WORKER_INIT(worker)	{				\
> @@ -79,7 +76,6 @@ struct kthread_work {
>  	.node = LIST_HEAD_INIT((work).node),				\
>  	.func = (fn),							\
>  	.done = __WAIT_QUEUE_HEAD_INITIALIZER((work).done),		\
> -	.flushing = ATOMIC_INIT(0),					\
>  	}
>  
>  #define DEFINE_KTHREAD_WORKER(worker)					\
> diff --git a/kernel/kthread.c b/kernel/kthread.c
> index 7b8a678..4034b2b 100644
> --- a/kernel/kthread.c
> +++ b/kernel/kthread.c
> @@ -360,16 +360,12 @@ repeat:
>  					struct kthread_work, node);
>  		list_del_init(&work->node);
>  	}
> +	worker->current_work = work;
>  	spin_unlock_irq(&worker->lock);
>  
>  	if (work) {
>  		__set_current_state(TASK_RUNNING);
>  		work->func(work);

If the call to 'work->func(work);' frees the memory pointed to by
'work', 'worker->current_work' points to deallocated memory.
So 'worker->current_work' will only ever used as a unique 'work'
identifier to handle, correct?


> -		smp_wmb();	/* wmb worker-b0 paired with flush-b1 */
> -		work->done_seq = work->queue_seq;
> -		smp_mb();	/* mb worker-b1 paired with flush-b0 */
> -		if (atomic_read(&work->flushing))
> -			wake_up_all(&work->done);
>  	} else if (!freezing(current))
>  		schedule();
>  
> @@ -384,7 +380,7 @@ static void insert_kthread_work(struct kthread_worker *worker,
>  			       struct list_head *pos)
>  {
>  	list_add_tail(&work->node, pos);
> -	work->queue_seq++;
> +	work->worker = worker;
>  	if (likely(worker->task))
>  		wake_up_process(worker->task);
>  }
> @@ -434,25 +430,35 @@ static void kthread_flush_work_fn(struct kthread_work *work)
>   */
>  void flush_kthread_work(struct kthread_work *work)
>  {
> -	int seq = work->queue_seq;
> +	struct kthread_flush_work fwork = {
> +		KTHREAD_WORK_INIT(fwork.work, kthread_flush_work_fn),
> +		COMPLETION_INITIALIZER_ONSTACK(fwork.done),
> +	};
> +	struct kthread_worker *worker;
> +	bool noop = false;
> +

You might want a check for 'work == NULL' here, to gracefully handle
code like the following:

void driver_work_handler(struct kthread_work *work)
{
	...
	kfree(work);
}

struct kthread_work *driver_queue_batch(void)
{
	struct kthread_work *work = NULL;
	...
	while (driver_more_stuff_todo()) {
		work = kzalloc(sizeof(struct kthread work), GFP_WHATEVER);
		...
		queue_kthread_work(&driver_worker, work);
	}
	return work;
}

void driver_foobar(void)
{
	...
	flush_kthread_work(driver_queue_batch());
	...
}


Otherwise, things look OK to me.

Regards,
Andy

> +retry:
> +	worker = work->worker;
> +	if (!worker)
> +		return;
>  
> -	atomic_inc(&work->flushing);
> +	spin_lock_irq(&worker->lock);
> +	if (work->worker != worker) {
> +		spin_unlock_irq(&worker->lock);
> +		goto retry;
> +	}
>  
> -	/*
> -	 * mb flush-b0 paired with worker-b1, to make sure either
> -	 * worker sees the above increment or we see done_seq update.
> -	 */
> -	smp_mb__after_atomic_inc();
> +	if (!list_empty(&work->node))
> +		insert_kthread_work(worker, &fwork.work, work->node.next);
> +	else if (worker->current_work == work)
> +		insert_kthread_work(worker, &fwork.work, worker->work_list.next);
> +	else
> +		noop = true;
>  
> -	/* A - B <= 0 tests whether B is in front of A regardless of overflow */
> -	wait_event(work->done, seq - work->done_seq <= 0);
> -	atomic_dec(&work->flushing);
> +	spin_unlock_irq(&worker->lock);
>  
> -	/*
> -	 * rmb flush-b1 paired with worker-b0, to make sure our caller
> -	 * sees every change made by work->func().
> -	 */
> -	smp_mb__after_atomic_dec();
> +	if (!noop)
> +		wait_for_completion(&fwork.done);
>  }
>  EXPORT_SYMBOL_GPL(flush_kthread_work);
>  

Re: [PATCH 2/2] kthread_worker: reimplement flush_kthread_work() to allow freeing the work item being executed

[Tejun Heo]

Hello,

On Sat, Jul 21, 2012 at 02:20:06PM -0400, Andy Walls wrote:
> > +	worker->current_work = work;
> >  	spin_unlock_irq(&worker->lock);
> >  
> >  	if (work) {
> >  		__set_current_state(TASK_RUNNING);
> >  		work->func(work);
> 
> If the call to 'work->func(work);' frees the memory pointed to by
> 'work', 'worker->current_work' points to deallocated memory.
> So 'worker->current_work' will only ever used as a unique 'work'
> identifier to handle, correct?

Yeah.  flush_kthread_work(@work) which can only be called if @work is
known to be alive looks at the pointer to determine whether it's the
current work item on the worker.

> >  void flush_kthread_work(struct kthread_work *work)
> >  {
> > -	int seq = work->queue_seq;
> > +	struct kthread_flush_work fwork = {
> > +		KTHREAD_WORK_INIT(fwork.work, kthread_flush_work_fn),
> > +		COMPLETION_INITIALIZER_ONSTACK(fwork.done),
> > +	};
> > +	struct kthread_worker *worker;
> > +	bool noop = false;
> > +
> 
> You might want a check for 'work == NULL' here, to gracefully handle
> code like the following:
> 
> void driver_work_handler(struct kthread_work *work)
> {
> 	...
> 	kfree(work);
> }
> 
> struct kthread_work *driver_queue_batch(void)
> {
> 	struct kthread_work *work = NULL;
> 	...
> 	while (driver_more_stuff_todo()) {
> 		work = kzalloc(sizeof(struct kthread work), GFP_WHATEVER);
> 		...
> 		queue_kthread_work(&driver_worker, work);
> 	}
> 	return work;
> }
> 
> void driver_foobar(void)
> {
> 	...
> 	flush_kthread_work(driver_queue_batch());
> 	...
> }

workqueue's flush_work() doesn't allow %NULL pointer.  I don't want to
make the behaviors deviate and don't see much point in changing
workqueue's behavior at this point.

Thanks.

-- 
tejun

Re: [PATCH 2/2] kthread_worker: reimplement flush_kthread_work() to allow freeing the work item being executed

[Andy Walls]

On Sun, 2012-07-22 at 09:49 -0700, Tejun Heo wrote:
> Hello,
> 
> On Sat, Jul 21, 2012 at 02:20:06PM -0400, Andy Walls wrote:
> > > +	worker->current_work = work;
> > >  	spin_unlock_irq(&worker->lock);
> > >  
> > >  	if (work) {
> > >  		__set_current_state(TASK_RUNNING);
> > >  		work->func(work);
> > 
> > If the call to 'work->func(work);' frees the memory pointed to by
> > 'work', 'worker->current_work' points to deallocated memory.
> > So 'worker->current_work' will only ever used as a unique 'work'
> > identifier to handle, correct?
> 
> Yeah.  flush_kthread_work(@work) which can only be called if @work is
> known to be alive looks at the pointer to determine whether it's the
> current work item on the worker.

OK.  Thanks.

Hmmm, I didn't know about the constraint about 'known to be alive' in
the other email I just sent.

That might make calling flush_kthread_work() hard for a user to use, if
the user lets the work get freed by another thread executing the work.


> > >  void flush_kthread_work(struct kthread_work *work)
> > >  {
> > > -	int seq = work->queue_seq;
> > > +	struct kthread_flush_work fwork = {
> > > +		KTHREAD_WORK_INIT(fwork.work, kthread_flush_work_fn),
> > > +		COMPLETION_INITIALIZER_ONSTACK(fwork.done),
> > > +	};
> > > +	struct kthread_worker *worker;
> > > +	bool noop = false;
> > > +
> > 
> > You might want a check for 'work == NULL' here, to gracefully handle
> > code like the following:

> workqueue's flush_work() doesn't allow %NULL pointer.  I don't want to
> make the behaviors deviate and don't see much point in changing
> workqueue's behavior at this point.

OK.  Fair enough.

Thanks.

Regards,
Andy

Re: [PATCH 2/2] kthread_worker: reimplement flush_kthread_work() to allow freeing the work item being executed

[Tejun Heo]

Hello,

On Sun, Jul 22, 2012 at 04:46:54PM -0400, Andy Walls wrote:
> Hmmm, I didn't know about the constraint about 'known to be alive' in
> the other email I just sent.
> 
> That might make calling flush_kthread_work() hard for a user to use, if
> the user lets the work get freed by another thread executing the work.

Umm... flushing a freed work item doesn't make any sense at all.  The
pointer itself loses the ability to identify anything.  What if it
gets recycled to another work item which happens to depend on the
flusher to make forward progress?  You now have a circular dependency
through a recycled memory area.  Good luck hunting that down.

For pretty much any API, allowing dangling pointers as argument is
insane.  If you want to flush self-freeing work items, flush the
kthread_worker.  That's how it is with workqueue and how it should be
with kthread_worker too.

Thanks.

-- 
tejun

Re: [PATCH 2/2] kthread_worker: reimplement flush_kthread_work() to allow freeing the work item being executed

[Andy Walls]

On Mon, 2012-07-23 at 10:12 -0700, Tejun Heo wrote:
> Hello,
> 
> On Sun, Jul 22, 2012 at 04:46:54PM -0400, Andy Walls wrote:
> > Hmmm, I didn't know about the constraint about 'known to be alive' in
> > the other email I just sent.
> > 
> > That might make calling flush_kthread_work() hard for a user to use, if
> > the user lets the work get freed by another thread executing the work.
> 
> Umm... flushing a freed work item doesn't make any sense at all.  The
> pointer itself loses the ability to identify anything.  What if it
> gets recycled to another work item which happens to depend on the
> flusher to make forward progress?  You now have a circular dependency
> through a recycled memory area.  Good luck hunting that down.
> 
> For pretty much any API, allowing dangling pointers as argument is
> insane.  If you want to flush self-freeing work items, flush the
> kthread_worker.  That's how it is with workqueue and how it should be
> with kthread_worker too.

Hi,

Ah.  My problem was that I mentally assigned the wrong rationale for why
you reworked flush_kthread_work().

Thank you for your patience and explanations.
Sorry for the noise.

For patch 2/2:

Reviewed-by: Andy Walls <awalls@md.metrocast.net>

Regards,
Andy

Re: [PATCH 2/2] kthread_worker: reimplement flush_kthread_work() to allow freeing the work item being executed

[Andy Walls]

Hi Tejun,

Thanks for responding to my previous questions.  I have one more.

On Sat, 2012-07-21 at 14:20 -0400, Andy Walls wrote:
> On Thu, 2012-07-19 at 14:16 -0700, Tejun Heo wrote:
> > From 06f9a06f4aeecdb9d07014713ab41b548ae219b5 Mon Sep 17 00:00:00 2001
> > From: Tejun Heo <tj@kernel.org>
> > Date: Thu, 19 Jul 2012 13:52:53 -0700
> > 
> > kthread_worker provides minimalistic workqueue-like interface for
> > users which need a dedicated worker thread (e.g. for realtime
> > priority).  It has basic queue, flush_work, flush_worker operations
> > which mostly match the workqueue counterparts; however, due to the way
> > flush_work() is implemented, it has a noticeable difference of not
> > allowing work items to be freed while being executed. 

[snip]

> > @@ -434,25 +430,35 @@ static void kthread_flush_work_fn(struct kthread_work *work)
> >   */
> >  void flush_kthread_work(struct kthread_work *work)
> >  {
> > -	int seq = work->queue_seq;
> > +	struct kthread_flush_work fwork = {
> > +		KTHREAD_WORK_INIT(fwork.work, kthread_flush_work_fn),
> > +		COMPLETION_INITIALIZER_ONSTACK(fwork.done),
> > +	};
> > +	struct kthread_worker *worker;
> > +	bool noop = false;
> > +
> 
> You might want a check for 'work == NULL' here, to gracefully handle
> code like the following:
> 
> void driver_work_handler(struct kthread_work *work)
> {
> 	...
> 	kfree(work);
> }
> 
> struct kthread_work *driver_queue_batch(void)
> {
> 	struct kthread_work *work = NULL;
> 	...
> 	while (driver_more_stuff_todo()) {
> 		work = kzalloc(sizeof(struct kthread work), GFP_WHATEVER);
> 		...
> 		queue_kthread_work(&driver_worker, work);
> 	}
> 	return work;
> }
> 
> void driver_foobar(void)
> {
> 	...
> 	flush_kthread_work(driver_queue_batch());
> 	...
> }

[snip]

> > +retry:
> > +	worker = work->worker;
> > +	if (!worker)
> > +		return;
> >  
> > -	atomic_inc(&work->flushing);
> > +	spin_lock_irq(&worker->lock);
> > +	if (work->worker != worker) {
> > +		spin_unlock_irq(&worker->lock);
> > +		goto retry;
> > +	}
> >  
> > -	/*
> > -	 * mb flush-b0 paired with worker-b1, to make sure either
> > -	 * worker sees the above increment or we see done_seq update.
> > -	 */
> > -	smp_mb__after_atomic_inc();
> > +	if (!list_empty(&work->node))
> > +		insert_kthread_work(worker, &fwork.work, work->node.next);
> > +	else if (worker->current_work == work)
> > +		insert_kthread_work(worker, &fwork.work, worker->work_list.next);
> > +	else
> > +		noop = true;

The objective is "allowing work items to be freed while being executed",
to me, it does not seem safe to me to allow flush_kthread_work() to
actually dereference the passed in work pointer.

flush_kthread_work() could theoretically be executed after the work
function was executed by the worker kthread which frees the 'work'
object, and that the memory 'work' points to could theoretically already
be reallocated for something else.  (I admit the above likely has very
low probability of occuring.)  

Is there a way to avoid dereferencing 'work' here?

Regards,
Andy

> >  
> > -	/* A - B <= 0 tests whether B is in front of A regardless of overflow */
> > -	wait_event(work->done, seq - work->done_seq <= 0);
> > -	atomic_dec(&work->flushing);
> > +	spin_unlock_irq(&worker->lock);
> >  
> > -	/*
> > -	 * rmb flush-b1 paired with worker-b0, to make sure our caller
> > -	 * sees every change made by work->func().
> > -	 */
> > -	smp_mb__after_atomic_dec();
> > +	if (!noop)
> > +		wait_for_completion(&fwork.done);
> >  }
> >  EXPORT_SYMBOL_GPL(flush_kthread_work);
> >  
> 

Re: [PATCHSET] kthread_worker: reimplement flush_kthread_work() to allow freeing during execution

[Colin Cross]

On Thu, Jul 19, 2012 at 2:15 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello,
>
> kthread_worker was introduced together with concurrency managed
> workqueue to serve workqueue users which need a special dedicated
> worker - e.g. RT scheduling.  This is minimal queue / flush / flush
> all iterface on top of kthread and each provided interface matches the
> workqueue counterpart so that switching isn't difficult.
>
> However, one noticeable difference was that kthread_worker doesn't
> allow a work item to be freed while being executed.  The intention was
> to keep the code simpler but it didn't really and the restriction is
> subtle and does prevent some valid use cases.
>
> This two-patch series reimplements flush_kthread_work() so that it
> uses an extra work item for flushing.  While this takes a bit more
> lines, this is easier to understand and removes the annoying
> difference.
>
> This patchset contains the following two patches.
>
>  0001-kthread_worker-reorganize-to-prepare-for-flush_kthre.patch
>  0002-kthread_worker-reimplement-flush_kthread_work-to-all.patch
>
> The first one is a prep patch which makes no functional changes.  The
> second reimplements flush_kthread_work().
>
> All current kthread_worker users are cc'd.  If no one objects, I'll
> push it through the workqueue branch.  This patchset is also available
> in the following git branch.
>
>  git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git review-kthread_worker-flush
>
> diffstat follows.  Thanks.
>
>  include/linux/kthread.h |    8 +---
>  kernel/kthread.c        |   86 +++++++++++++++++++++++++++---------------------
>  2 files changed, 52 insertions(+), 42 deletions(-)
>
> --
> tejun

This patch set fixes a reproducible crash I'm seeing on a 3.4.10
kernel.  flush_kthread_worker (which is different from
flush_kthread_work) is initializing a kthread_work and a completion on
the stack, then queuing it and calling wait_for_completion.  Once the
completion is signaled, flush_kthread_worker exits and the stack
region used by the kthread_work may be immediately reused by another
object on the stack, but kthread_worker_fn continues accessing its
work pointer:
                work->func(work);         <- calls complete,
effectively frees work
                smp_wmb();      /* wmb worker-b0 paired with flush-b1 */
                work->done_seq = work->queue_seq;   <- overwrites a
new stack object
                smp_mb();       /* mb worker-b1 paired with flush-b0 */
                if (atomic_read(&work->flushing))
                        wake_up_all(&work->done);  <- or crashes here

These patches fix the problem by not accessing work after work->func
is called, and should be backported to stable.  They apply cleanly to
3.4.10.  Upstream commits are 9a2e03d8ed518a61154f18d83d6466628e519f94
and 46f3d976213452350f9d10b0c2780c2681f7075b.

Re: [PATCHSET] kthread_worker: reimplement flush_kthread_work() to allow freeing during execution

[Tejun Heo]

On Fri, Sep 14, 2012 at 03:50:40PM -0700, Colin Cross wrote:
> This patch set fixes a reproducible crash I'm seeing on a 3.4.10
> kernel.  flush_kthread_worker (which is different from
> flush_kthread_work) is initializing a kthread_work and a completion on
> the stack, then queuing it and calling wait_for_completion.  Once the
> completion is signaled, flush_kthread_worker exits and the stack
> region used by the kthread_work may be immediately reused by another
> object on the stack, but kthread_worker_fn continues accessing its
> work pointer:
>                 work->func(work);         <- calls complete,
> effectively frees work
>                 smp_wmb();      /* wmb worker-b0 paired with flush-b1 */
>                 work->done_seq = work->queue_seq;   <- overwrites a
> new stack object
>                 smp_mb();       /* mb worker-b1 paired with flush-b0 */
>                 if (atomic_read(&work->flushing))
>                         wake_up_all(&work->done);  <- or crashes here
> 
> These patches fix the problem by not accessing work after work->func
> is called, and should be backported to stable.  They apply cleanly to
> 3.4.10.  Upstream commits are 9a2e03d8ed518a61154f18d83d6466628e519f94
> and 46f3d976213452350f9d10b0c2780c2681f7075b.

Yeah, you're right.  I wonder why this didn't come up before.  Greg,
can you please pick up these two commits?

Thanks.

-- 
tejun

Re: [PATCHSET] kthread_worker: reimplement flush_kthread_work() to allow freeing during execution

[Greg KH]

On Mon, Sep 17, 2012 at 12:40:16PM -0700, Tejun Heo wrote:
> On Fri, Sep 14, 2012 at 03:50:40PM -0700, Colin Cross wrote:
> > This patch set fixes a reproducible crash I'm seeing on a 3.4.10
> > kernel.  flush_kthread_worker (which is different from
> > flush_kthread_work) is initializing a kthread_work and a completion on
> > the stack, then queuing it and calling wait_for_completion.  Once the
> > completion is signaled, flush_kthread_worker exits and the stack
> > region used by the kthread_work may be immediately reused by another
> > object on the stack, but kthread_worker_fn continues accessing its
> > work pointer:
> >                 work->func(work);         <- calls complete,
> > effectively frees work
> >                 smp_wmb();      /* wmb worker-b0 paired with flush-b1 */
> >                 work->done_seq = work->queue_seq;   <- overwrites a
> > new stack object
> >                 smp_mb();       /* mb worker-b1 paired with flush-b0 */
> >                 if (atomic_read(&work->flushing))
> >                         wake_up_all(&work->done);  <- or crashes here
> > 
> > These patches fix the problem by not accessing work after work->func
> > is called, and should be backported to stable.  They apply cleanly to
> > 3.4.10.  Upstream commits are 9a2e03d8ed518a61154f18d83d6466628e519f94
> > and 46f3d976213452350f9d10b0c2780c2681f7075b.
> 
> Yeah, you're right.  I wonder why this didn't come up before.  Greg,
> can you please pick up these two commits?

Ok, will do, thanks for letting me know.

greg k-h

Re: [PATCHSET] kthread_worker: reimplement flush_kthread_work() to allow freeing during execution

[Greg KH]

On Mon, Sep 17, 2012 at 01:28:50PM -0700, Greg KH wrote:
> On Mon, Sep 17, 2012 at 12:40:16PM -0700, Tejun Heo wrote:
> > On Fri, Sep 14, 2012 at 03:50:40PM -0700, Colin Cross wrote:
> > > This patch set fixes a reproducible crash I'm seeing on a 3.4.10
> > > kernel.  flush_kthread_worker (which is different from
> > > flush_kthread_work) is initializing a kthread_work and a completion on
> > > the stack, then queuing it and calling wait_for_completion.  Once the
> > > completion is signaled, flush_kthread_worker exits and the stack
> > > region used by the kthread_work may be immediately reused by another
> > > object on the stack, but kthread_worker_fn continues accessing its
> > > work pointer:
> > >                 work->func(work);         <- calls complete,
> > > effectively frees work
> > >                 smp_wmb();      /* wmb worker-b0 paired with flush-b1 */
> > >                 work->done_seq = work->queue_seq;   <- overwrites a
> > > new stack object
> > >                 smp_mb();       /* mb worker-b1 paired with flush-b0 */
> > >                 if (atomic_read(&work->flushing))
> > >                         wake_up_all(&work->done);  <- or crashes here
> > > 
> > > These patches fix the problem by not accessing work after work->func
> > > is called, and should be backported to stable.  They apply cleanly to
> > > 3.4.10.  Upstream commits are 9a2e03d8ed518a61154f18d83d6466628e519f94
> > > and 46f3d976213452350f9d10b0c2780c2681f7075b.
> > 
> > Yeah, you're right.  I wonder why this didn't come up before.  Greg,
> > can you please pick up these two commits?
> 
> Ok, will do, thanks for letting me know.

Now applied, thanks.

greg k-h