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

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

[Petr Mladek]

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 <minyard@acm.org>
CC: openipmi-developer@lists.sourceforge.net
CC: Doug Ledford <dledford@redhat.com>
CC: Sean Hefty <sean.hefty@intel.com>
CC: Hal Rosenstock <hal.rosenstock@gmail.com>
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 <jacob.jun.pan@linux.intel.com>
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>

[PATCH v3 18/22] IB/fmr_pool: Convert the cleanup thread into kthread worker API

[Petr Mladek]

Kthreads are currently implemented as an infinite loop. Each
has its own variant of checks for terminating, freezing,
awakening. In many cases it is unclear to say in which state
it is and sometimes it is done a wrong way.

The plan is to convert kthreads into kthread_worker or workqueues
API. It allows to split the functionality into separate operations.
It helps to make a better structure. Also it defines a clean state
where no locks are taken, IRQs blocked, the kthread might sleep
or even be safely migrated.

The kthread worker API is useful when we want to have a dedicated
single thread for the work. It helps to make sure that it is
available when needed. Also it allows a better control, e.g.
define a scheduling priority.

This patch converts the frm_pool kthread into the kthread worker
API because I am not sure how busy the thread is. It is well
possible that it does not need a dedicated kthread and workqueues
would be perfectly fine. Well, the conversion between kthread
worker API and workqueues is pretty trivial.

The patch moves one iteration from the kthread into the work function.
It preserves the check for a spurious queuing (wake up). Then it
processes one request. Finally, it re-queues itself if more requests
are pending.

Otherwise, wake_up_process() is replaced by queuing the work.

Important: The change is only compile tested. I did not find an easy
way how to check it in a real life.

Signed-off-by: Petr Mladek <pmladek@suse.com>
CC: Doug Ledford <dledford@redhat.com>
CC: Sean Hefty <sean.hefty@intel.com>
CC: Hal Rosenstock <hal.rosenstock@gmail.com>
CC: linux-rdma@vger.kernel.org
---
 drivers/infiniband/core/fmr_pool.c | 54 ++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 29 deletions(-)

diff --git a/drivers/infiniband/core/fmr_pool.c b/drivers/infiniband/core/fmr_pool.c
index 9f5ad7cc33c8..5f2b06bd14da 100644
--- a/drivers/infiniband/core/fmr_pool.c
+++ b/drivers/infiniband/core/fmr_pool.c
@@ -96,7 +96,8 @@ struct ib_fmr_pool {
 						   void *              arg);
 	void                     *flush_arg;
 
-	struct task_struct       *thread;
+	struct kthread_worker	  *worker;
+	struct kthread_work	  work;
 
 	atomic_t                  req_ser;
 	atomic_t                  flush_ser;
@@ -174,29 +175,26 @@ static void ib_fmr_batch_release(struct ib_fmr_pool *pool)
 	spin_unlock_irq(&pool->pool_lock);
 }
 
-static int ib_fmr_cleanup_thread(void *pool_ptr)
+static void ib_fmr_cleanup_func(struct kthread_work *work)
 {
-	struct ib_fmr_pool *pool = pool_ptr;
+	struct ib_fmr_pool *pool = container_of(work, struct ib_fmr_pool, work);
 
-	do {
-		if (atomic_read(&pool->flush_ser) - atomic_read(&pool->req_ser) < 0) {
-			ib_fmr_batch_release(pool);
-
-			atomic_inc(&pool->flush_ser);
-			wake_up_interruptible(&pool->force_wait);
+	/*
+	 * The same request might be queued twice when it appears and
+	 * by re-queuing from this work.
+	 */
+	if (atomic_read(&pool->flush_ser) - atomic_read(&pool->req_ser) >= 0)
+		return;
 
-			if (pool->flush_function)
-				pool->flush_function(pool, pool->flush_arg);
-		}
+	ib_fmr_batch_release(pool);
+	atomic_inc(&pool->flush_ser);
+	wake_up_interruptible(&pool->force_wait);
 
-		set_current_state(TASK_INTERRUPTIBLE);
-		if (atomic_read(&pool->flush_ser) - atomic_read(&pool->req_ser) >= 0 &&
-		    !kthread_should_stop())
-			schedule();
-		__set_current_state(TASK_RUNNING);
-	} while (!kthread_should_stop());
+	if (pool->flush_function)
+		pool->flush_function(pool, pool->flush_arg);
 
-	return 0;
+	if (atomic_read(&pool->flush_ser) - atomic_read(&pool->req_ser) < 0)
+		queue_kthread_work(pool->worker, &pool->work);
 }
 
 /**
@@ -286,15 +284,13 @@ struct ib_fmr_pool *ib_create_fmr_pool(struct ib_pd             *pd,
 	atomic_set(&pool->flush_ser, 0);
 	init_waitqueue_head(&pool->force_wait);
 
-	pool->thread = kthread_run(ib_fmr_cleanup_thread,
-				   pool,
-				   "ib_fmr(%s)",
-				   device->name);
-	if (IS_ERR(pool->thread)) {
-		printk(KERN_WARNING PFX "couldn't start cleanup thread\n");
-		ret = PTR_ERR(pool->thread);
+	pool->worker = create_kthread_worker(0, "ib_fmr(%s)", device->name);
+	if (IS_ERR(pool->worker)) {
+		pr_warn(PFX "couldn't start cleanup kthread worker\n");
+		ret = PTR_ERR(pool->worker);
 		goto out_free_pool;
 	}
+	init_kthread_work(&pool->work, ib_fmr_cleanup_func);
 
 	{
 		struct ib_pool_fmr *fmr;
@@ -362,7 +358,7 @@ void ib_destroy_fmr_pool(struct ib_fmr_pool *pool)
 	LIST_HEAD(fmr_list);
 	int                 i;
 
-	kthread_stop(pool->thread);
+	destroy_kthread_worker(pool->worker);
 	ib_fmr_batch_release(pool);
 
 	i = 0;
@@ -412,7 +408,7 @@ int ib_flush_fmr_pool(struct ib_fmr_pool *pool)
 	spin_unlock_irq(&pool->pool_lock);
 
 	serial = atomic_inc_return(&pool->req_ser);
-	wake_up_process(pool->thread);
+	queue_kthread_work(pool->worker, &pool->work);
 
 	if (wait_event_interruptible(pool->force_wait,
 				     atomic_read(&pool->flush_ser) - serial >= 0))
@@ -526,7 +522,7 @@ int ib_fmr_pool_unmap(struct ib_pool_fmr *fmr)
 			list_add_tail(&fmr->list, &pool->dirty_list);
 			if (++pool->dirty_len >= pool->dirty_watermark) {
 				atomic_inc(&pool->req_ser);
-				wake_up_process(pool->thread);
+				queue_kthread_work(pool->worker, &pool->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>

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

[Paul E. McKenney]

On Wed, Nov 18, 2015 at 02:25:05PM +0100, Petr Mladek wrote:
> 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

Thank you!  ;-)

							Thanx, Paul

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

Re: [PATCH v3 18/22] IB/fmr_pool: Convert the cleanup thread into kthread worker API

[Yuval Shaia]

On Wed, Nov 18, 2015 at 02:25:23PM +0100, Petr Mladek wrote:
> Kthreads are currently implemented as an infinite loop. Each
> has its own variant of checks for terminating, freezing,
> awakening. In many cases it is unclear to say in which state
> it is and sometimes it is done a wrong way.
> 
> The plan is to convert kthreads into kthread_worker or workqueues
> API. It allows to split the functionality into separate operations.
> It helps to make a better structure. Also it defines a clean state
> where no locks are taken, IRQs blocked, the kthread might sleep
> or even be safely migrated.
> 
> The kthread worker API is useful when we want to have a dedicated
> single thread for the work. It helps to make sure that it is
> available when needed. Also it allows a better control, e.g.
> define a scheduling priority.
> 
> This patch converts the frm_pool kthread into the kthread worker
s/frm/fmr
> API because I am not sure how busy the thread is. It is well
> possible that it does not need a dedicated kthread and workqueues
> would be perfectly fine. Well, the conversion between kthread
> worker API and workqueues is pretty trivial.
> 
> The patch moves one iteration from the kthread into the work function.
> It preserves the check for a spurious queuing (wake up). Then it
> processes one request. Finally, it re-queues itself if more requests
> are pending.
> 
> Otherwise, wake_up_process() is replaced by queuing the work.
> 
> Important: The change is only compile tested. I did not find an easy
> way how to check it in a real life.
What are the expectations?
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> CC: Doug Ledford <dledford@redhat.com>
> CC: Sean Hefty <sean.hefty@intel.com>
> CC: Hal Rosenstock <hal.rosenstock@gmail.com>
> CC: linux-rdma@vger.kernel.org
> ---
>  drivers/infiniband/core/fmr_pool.c | 54 ++++++++++++++++++--------------------
>  1 file changed, 25 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/infiniband/core/fmr_pool.c b/drivers/infiniband/core/fmr_pool.c
> index 9f5ad7cc33c8..5f2b06bd14da 100644
> --- a/drivers/infiniband/core/fmr_pool.c
> +++ b/drivers/infiniband/core/fmr_pool.c
> @@ -96,7 +96,8 @@ struct ib_fmr_pool {
>  						   void *              arg);
>  	void                     *flush_arg;
>  
> -	struct task_struct       *thread;
> +	struct kthread_worker	  *worker;
> +	struct kthread_work	  work;
>  
>  	atomic_t                  req_ser;
>  	atomic_t                  flush_ser;
> @@ -174,29 +175,26 @@ static void ib_fmr_batch_release(struct ib_fmr_pool *pool)
>  	spin_unlock_irq(&pool->pool_lock);
>  }
>  
> -static int ib_fmr_cleanup_thread(void *pool_ptr)
> +static void ib_fmr_cleanup_func(struct kthread_work *work)
>  {
> -	struct ib_fmr_pool *pool = pool_ptr;
> +	struct ib_fmr_pool *pool = container_of(work, struct ib_fmr_pool, work);
>  
> -	do {
> -		if (atomic_read(&pool->flush_ser) - atomic_read(&pool->req_ser) < 0) {
> -			ib_fmr_batch_release(pool);
> -
> -			atomic_inc(&pool->flush_ser);
> -			wake_up_interruptible(&pool->force_wait);
> +	/*
> +	 * The same request might be queued twice when it appears and
> +	 * by re-queuing from this work.
> +	 */
> +	if (atomic_read(&pool->flush_ser) - atomic_read(&pool->req_ser) >= 0)
> +		return;
>  
> -			if (pool->flush_function)
> -				pool->flush_function(pool, pool->flush_arg);
> -		}
> +	ib_fmr_batch_release(pool);
> +	atomic_inc(&pool->flush_ser);
> +	wake_up_interruptible(&pool->force_wait);
>  
> -		set_current_state(TASK_INTERRUPTIBLE);
> -		if (atomic_read(&pool->flush_ser) - atomic_read(&pool->req_ser) >= 0 &&
> -		    !kthread_should_stop())
> -			schedule();
> -		__set_current_state(TASK_RUNNING);
> -	} while (!kthread_should_stop());
> +	if (pool->flush_function)
> +		pool->flush_function(pool, pool->flush_arg);
>  
> -	return 0;
> +	if (atomic_read(&pool->flush_ser) - atomic_read(&pool->req_ser) < 0)
> +		queue_kthread_work(pool->worker, &pool->work);
>  }
>  
>  /**
> @@ -286,15 +284,13 @@ struct ib_fmr_pool *ib_create_fmr_pool(struct ib_pd             *pd,
>  	atomic_set(&pool->flush_ser, 0);
>  	init_waitqueue_head(&pool->force_wait);
>  
> -	pool->thread = kthread_run(ib_fmr_cleanup_thread,
> -				   pool,
> -				   "ib_fmr(%s)",
> -				   device->name);
> -	if (IS_ERR(pool->thread)) {
> -		printk(KERN_WARNING PFX "couldn't start cleanup thread\n");
> -		ret = PTR_ERR(pool->thread);
> +	pool->worker = create_kthread_worker(0, "ib_fmr(%s)", device->name);
Is this patch depends on some other patch?
> +	if (IS_ERR(pool->worker)) {
> +		pr_warn(PFX "couldn't start cleanup kthread worker\n");
> +		ret = PTR_ERR(pool->worker);
>  		goto out_free_pool;
>  	}
> +	init_kthread_work(&pool->work, ib_fmr_cleanup_func);
>  
>  	{
>  		struct ib_pool_fmr *fmr;
> @@ -362,7 +358,7 @@ void ib_destroy_fmr_pool(struct ib_fmr_pool *pool)
>  	LIST_HEAD(fmr_list);
>  	int                 i;
>  
> -	kthread_stop(pool->thread);
> +	destroy_kthread_worker(pool->worker);
>  	ib_fmr_batch_release(pool);
>  
>  	i = 0;
> @@ -412,7 +408,7 @@ int ib_flush_fmr_pool(struct ib_fmr_pool *pool)
>  	spin_unlock_irq(&pool->pool_lock);
>  
>  	serial = atomic_inc_return(&pool->req_ser);
> -	wake_up_process(pool->thread);
> +	queue_kthread_work(pool->worker, &pool->work);
>  
>  	if (wait_event_interruptible(pool->force_wait,
>  				     atomic_read(&pool->flush_ser) - serial >= 0))
> @@ -526,7 +522,7 @@ int ib_fmr_pool_unmap(struct ib_pool_fmr *fmr)
>  			list_add_tail(&fmr->list, &pool->dirty_list);
>  			if (++pool->dirty_len >= pool->dirty_watermark) {
>  				atomic_inc(&pool->req_ser);
> -				wake_up_process(pool->thread);
> +				queue_kthread_work(pool->worker, &pool->work);
>  			}
>  		}
>  	}
> -- 
> 1.8.5.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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