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

Groups > linux.kernel > #1272151 > unrolled thread

[PATCH v3 17/22] ipmi: Convert kipmi kthread into kthread worker API

Back to article view | Back to linux.kernel

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

Contents

  [PATCH v3 17/22] ipmi: Convert kipmi kthread into kthread worker API Petr Mladek <pmladek@suse.com> - 2015-11-18 14:30 +0100
    Re: [PATCH v3 17/22] ipmi: Convert kipmi kthread into kthread worker  API Corey Minyard <minyard@acm.org> - 2015-11-23 20:40 +0100
      Re: [PATCH v3 17/22] ipmi: Convert kipmi kthread into kthread worker  API Petr Mladek <pmladek@suse.com> - 2015-11-24 13:20 +0100
        Re: [PATCH v3 17/22] ipmi: Convert kipmi kthread into kthread worker  API Corey Minyard <minyard@acm.org> - 2015-11-24 14:40 +0100

#1272151 — [PATCH v3 17/22] ipmi: Convert kipmi kthread into kthread worker API

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 kipmi kthread into the kthread worker API because
it modifies the scheduling priority. The change is quite straightforward.

First, we move the per-thread variable "busy_until" into the per-thread
structure struct smi_info. As a side effect, we could omit one parameter
in ipmi_thread_busy_wait(). On the other hand, the structure could not
longer be passed with the const qualifier.

The value of "busy_until" is initialized when the kthread is created.
Also the scheduling priority is set there. This helps to avoid an extra
init work.

One iteration of the kthread cycle is moved to a delayed work function.
The different delays between the cycles are solved the following way:

  + immediate cycle (nope) is converted into goto within the same work

  + immediate cycle with a possible reschedule is converted into
    re-queuing with a zero delay

  + schedule_timeout() is converted into re-queuing with the given
    delay

  + interruptible sleep is converted into nothing; The work
    will get queued again from the check_start_timer_thread().
    By other words the external wakeup_up_process() will get
    replaced by queuing with a zero delay.

Probably the most tricky change is when the worker is being stopped.
We need to explicitly cancel the work to prevent it from re-queuing.

Signed-off-by: Petr Mladek <pmladek@suse.com>
CC: Corey Minyard <minyard@acm.org>
CC: openipmi-developer@lists.sourceforge.net
---
 drivers/char/ipmi/ipmi_si_intf.c | 116 ++++++++++++++++++++++-----------------
 1 file changed, 66 insertions(+), 50 deletions(-)

diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
index 654f6f36a071..fdb97eaded4b 100644
--- a/drivers/char/ipmi/ipmi_si_intf.c
+++ b/drivers/char/ipmi/ipmi_si_intf.c
@@ -302,7 +302,9 @@ struct smi_info {
 	/* Counters and things for the proc filesystem. */
 	atomic_t stats[SI_NUM_STATS];
 
-	struct task_struct *thread;
+	struct kthread_worker *worker;
+	struct delayed_kthread_work work;
+	struct timespec64 busy_until;
 
 	struct list_head link;
 	union ipmi_smi_info_union addr_info;
@@ -929,8 +931,9 @@ static void check_start_timer_thread(struct smi_info *smi_info)
 	if (smi_info->si_state == SI_NORMAL && smi_info->curr_msg == NULL) {
 		smi_mod_timer(smi_info, jiffies + SI_TIMEOUT_JIFFIES);
 
-		if (smi_info->thread)
-			wake_up_process(smi_info->thread);
+		if (smi_info->worker)
+			mod_delayed_kthread_work(smi_info->worker,
+						 &smi_info->work, 0);
 
 		start_next_msg(smi_info);
 		smi_event_handler(smi_info, 0);
@@ -1008,10 +1011,10 @@ static inline int ipmi_si_is_busy(struct timespec64 *ts)
 }
 
 static inline int ipmi_thread_busy_wait(enum si_sm_result smi_result,
-					const struct smi_info *smi_info,
-					struct timespec64 *busy_until)
+					struct smi_info *smi_info)
 {
 	unsigned int max_busy_us = 0;
+	struct timespec64 *busy_until = &smi_info->busy_until;
 
 	if (smi_info->intf_num < num_max_busy_us)
 		max_busy_us = kipmid_max_busy_us[smi_info->intf_num];
@@ -1042,53 +1045,49 @@ static inline int ipmi_thread_busy_wait(enum si_sm_result smi_result,
  * (if that is enabled).  See the paragraph on kimid_max_busy_us in
  * Documentation/IPMI.txt for details.
  */
-static int ipmi_thread(void *data)
+static void ipmi_func(struct kthread_work *work)
 {
-	struct smi_info *smi_info = data;
+	struct smi_info *smi_info = container_of(work, struct smi_info,
+						 work.work);
 	unsigned long flags;
 	enum si_sm_result smi_result;
-	struct timespec64 busy_until;
+	int busy_wait;
 
-	ipmi_si_set_not_busy(&busy_until);
-	set_user_nice(current, MAX_NICE);
-	while (!kthread_should_stop()) {
-		int busy_wait;
+next:
+	spin_lock_irqsave(&(smi_info->si_lock), flags);
+	smi_result = smi_event_handler(smi_info, 0);
 
-		spin_lock_irqsave(&(smi_info->si_lock), flags);
-		smi_result = smi_event_handler(smi_info, 0);
+	/*
+	 * If the driver is doing something, there is a possible
+	 * race with the timer.  If the timer handler see idle,
+	 * and the thread here sees something else, the timer
+	 * handler won't restart the timer even though it is
+	 * required.  So start it here if necessary.
+	 */
+	if (smi_result != SI_SM_IDLE && !smi_info->timer_running)
+		smi_mod_timer(smi_info, jiffies + SI_TIMEOUT_JIFFIES);
 
-		/*
-		 * If the driver is doing something, there is a possible
-		 * race with the timer.  If the timer handler see idle,
-		 * and the thread here sees something else, the timer
-		 * handler won't restart the timer even though it is
-		 * required.  So start it here if necessary.
-		 */
-		if (smi_result != SI_SM_IDLE && !smi_info->timer_running)
-			smi_mod_timer(smi_info, jiffies + SI_TIMEOUT_JIFFIES);
-
-		spin_unlock_irqrestore(&(smi_info->si_lock), flags);
-		busy_wait = ipmi_thread_busy_wait(smi_result, smi_info,
-						  &busy_until);
-		if (smi_result == SI_SM_CALL_WITHOUT_DELAY)
-			; /* do nothing */
-		else if (smi_result == SI_SM_CALL_WITH_DELAY && busy_wait)
-			schedule();
-		else if (smi_result == SI_SM_IDLE) {
-			if (atomic_read(&smi_info->need_watch)) {
-				schedule_timeout_interruptible(100);
-			} else {
-				/* Wait to be woken up when we are needed. */
-				__set_current_state(TASK_INTERRUPTIBLE);
-				schedule();
-			}
-		} else
-			schedule_timeout_interruptible(1);
+	spin_unlock_irqrestore(&(smi_info->si_lock), flags);
+	busy_wait = ipmi_thread_busy_wait(smi_result, smi_info);
+
+	if (smi_result == SI_SM_CALL_WITHOUT_DELAY)
+		goto next;
+	if (smi_result == SI_SM_CALL_WITH_DELAY && busy_wait) {
+		queue_delayed_kthread_work(smi_info->worker,
+					   &smi_info->work, 0);
+	} else if (smi_result == SI_SM_IDLE) {
+		if (atomic_read(&smi_info->need_watch)) {
+			queue_delayed_kthread_work(smi_info->worker,
+						   &smi_info->work, 100);
+		} else {
+			/* Nope. Wait to be queued when we are needed. */
+		}
+	} else {
+		queue_delayed_kthread_work(smi_info->worker,
+					   &smi_info->work, 1);
 	}
-	return 0;
 }
 
-
 static void poll(void *send_info)
 {
 	struct smi_info *smi_info = send_info;
@@ -1229,17 +1228,29 @@ static int smi_start_processing(void       *send_info,
 		enable = 1;
 
 	if (enable) {
-		new_smi->thread = kthread_run(ipmi_thread, new_smi,
-					      "kipmi%d", new_smi->intf_num);
-		if (IS_ERR(new_smi->thread)) {
+		struct kthread_worker *worker;
+
+		worker = create_kthread_worker(0, "kipmi%d",
+					       new_smi->intf_num);
+
+		if (IS_ERR(worker)) {
 			dev_notice(new_smi->dev, "Could not start"
 				   " kernel thread due to error %ld, only using"
 				   " timers to drive the interface\n",
-				   PTR_ERR(new_smi->thread));
-			new_smi->thread = NULL;
+				   PTR_ERR(worker));
+			goto out;
 		}
+
+		ipmi_si_set_not_busy(&new_smi->busy_until);
+		set_user_nice(worker->task, MAX_NICE);
+
+		init_delayed_kthread_work(&new_smi->work, ipmi_func);
+		queue_delayed_kthread_work(worker, &new_smi->work, 0);
+
+		new_smi->worker = worker;
 	}
 
+out:
 	return 0;
 }
 
@@ -3414,8 +3425,13 @@ static void check_for_broken_irqs(struct smi_info *smi_info)
 
 static inline void wait_for_timer_and_thread(struct smi_info *smi_info)
 {
-	if (smi_info->thread != NULL)
-		kthread_stop(smi_info->thread);
+	if (smi_info->worker != NULL) {
+		struct kthread_worker *worker = smi_info->worker;
+
+		smi_info->worker = NULL;
+		cancel_delayed_kthread_work_sync(&smi_info->work);
+		destroy_kthread_worker(worker);
+	}
 	if (smi_info->timer_running)
 		del_timer_sync(&smi_info->si_timer);
 }
-- 
1.8.5.6

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1275776 — Re: [PATCH v3 17/22] ipmi: Convert kipmi kthread into kthread worker API

On 11/18/2015 07:25 AM, 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 kipmi kthread into the kthread worker API because
> it modifies the scheduling priority. The change is quite straightforward.

I think this is correct.  That code was hard to get right, but I don't
see where any
logic is actually changed.

This also doesn't really look any simpler (you end up with more LOC than
you did before :) ),
though it will make things more consistent and reduce errors and that's
a good thing.

My only comment is I would like the worker function named ipmi_worker,
not ipmi_func.

Reviewed-by: Corey Minyard <cminyard@mvista.com>

> First, we move the per-thread variable "busy_until" into the per-thread
> structure struct smi_info. As a side effect, we could omit one parameter
> in ipmi_thread_busy_wait(). On the other hand, the structure could not
> longer be passed with the const qualifier.
>
> The value of "busy_until" is initialized when the kthread is created.
> Also the scheduling priority is set there. This helps to avoid an extra
> init work.
>
> One iteration of the kthread cycle is moved to a delayed work function.
> The different delays between the cycles are solved the following way:
>
>   + immediate cycle (nope) is converted into goto within the same work
>
>   + immediate cycle with a possible reschedule is converted into
>     re-queuing with a zero delay
>
>   + schedule_timeout() is converted into re-queuing with the given
>     delay
>
>   + interruptible sleep is converted into nothing; The work
>     will get queued again from the check_start_timer_thread().
>     By other words the external wakeup_up_process() will get
>     replaced by queuing with a zero delay.
>
> Probably the most tricky change is when the worker is being stopped.
> We need to explicitly cancel the work to prevent it from re-queuing.
>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> CC: Corey Minyard <minyard@acm.org>
> CC: openipmi-developer@lists.sourceforge.net
> ---
>  drivers/char/ipmi/ipmi_si_intf.c | 116 ++++++++++++++++++++++-----------------
>  1 file changed, 66 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c
> index 654f6f36a071..fdb97eaded4b 100644
> --- a/drivers/char/ipmi/ipmi_si_intf.c
> +++ b/drivers/char/ipmi/ipmi_si_intf.c
> @@ -302,7 +302,9 @@ struct smi_info {
>  	/* Counters and things for the proc filesystem. */
>  	atomic_t stats[SI_NUM_STATS];
>  
> -	struct task_struct *thread;
> +	struct kthread_worker *worker;
> +	struct delayed_kthread_work work;
> +	struct timespec64 busy_until;
>  
>  	struct list_head link;
>  	union ipmi_smi_info_union addr_info;
> @@ -929,8 +931,9 @@ static void check_start_timer_thread(struct smi_info *smi_info)
>  	if (smi_info->si_state == SI_NORMAL && smi_info->curr_msg == NULL) {
>  		smi_mod_timer(smi_info, jiffies + SI_TIMEOUT_JIFFIES);
>  
> -		if (smi_info->thread)
> -			wake_up_process(smi_info->thread);
> +		if (smi_info->worker)
> +			mod_delayed_kthread_work(smi_info->worker,
> +						 &smi_info->work, 0);
>  
>  		start_next_msg(smi_info);
>  		smi_event_handler(smi_info, 0);
> @@ -1008,10 +1011,10 @@ static inline int ipmi_si_is_busy(struct timespec64 *ts)
>  }
>  
>  static inline int ipmi_thread_busy_wait(enum si_sm_result smi_result,
> -					const struct smi_info *smi_info,
> -					struct timespec64 *busy_until)
> +					struct smi_info *smi_info)
>  {
>  	unsigned int max_busy_us = 0;
> +	struct timespec64 *busy_until = &smi_info->busy_until;
>  
>  	if (smi_info->intf_num < num_max_busy_us)
>  		max_busy_us = kipmid_max_busy_us[smi_info->intf_num];
> @@ -1042,53 +1045,49 @@ static inline int ipmi_thread_busy_wait(enum si_sm_result smi_result,
>   * (if that is enabled).  See the paragraph on kimid_max_busy_us in
>   * Documentation/IPMI.txt for details.
>   */
> -static int ipmi_thread(void *data)
> +static void ipmi_func(struct kthread_work *work)
>  {
> -	struct smi_info *smi_info = data;
> +	struct smi_info *smi_info = container_of(work, struct smi_info,
> +						 work.work);
>  	unsigned long flags;
>  	enum si_sm_result smi_result;
> -	struct timespec64 busy_until;
> +	int busy_wait;
>  
> -	ipmi_si_set_not_busy(&busy_until);
> -	set_user_nice(current, MAX_NICE);
> -	while (!kthread_should_stop()) {
> -		int busy_wait;
> +next:
> +	spin_lock_irqsave(&(smi_info->si_lock), flags);
> +	smi_result = smi_event_handler(smi_info, 0);
>  
> -		spin_lock_irqsave(&(smi_info->si_lock), flags);
> -		smi_result = smi_event_handler(smi_info, 0);
> +	/*
> +	 * If the driver is doing something, there is a possible
> +	 * race with the timer.  If the timer handler see idle,
> +	 * and the thread here sees something else, the timer
> +	 * handler won't restart the timer even though it is
> +	 * required.  So start it here if necessary.
> +	 */
> +	if (smi_result != SI_SM_IDLE && !smi_info->timer_running)
> +		smi_mod_timer(smi_info, jiffies + SI_TIMEOUT_JIFFIES);
>  
> -		/*
> -		 * If the driver is doing something, there is a possible
> -		 * race with the timer.  If the timer handler see idle,
> -		 * and the thread here sees something else, the timer
> -		 * handler won't restart the timer even though it is
> -		 * required.  So start it here if necessary.
> -		 */
> -		if (smi_result != SI_SM_IDLE && !smi_info->timer_running)
> -			smi_mod_timer(smi_info, jiffies + SI_TIMEOUT_JIFFIES);
> -
> -		spin_unlock_irqrestore(&(smi_info->si_lock), flags);
> -		busy_wait = ipmi_thread_busy_wait(smi_result, smi_info,
> -						  &busy_until);
> -		if (smi_result == SI_SM_CALL_WITHOUT_DELAY)
> -			; /* do nothing */
> -		else if (smi_result == SI_SM_CALL_WITH_DELAY && busy_wait)
> -			schedule();
> -		else if (smi_result == SI_SM_IDLE) {
> -			if (atomic_read(&smi_info->need_watch)) {
> -				schedule_timeout_interruptible(100);
> -			} else {
> -				/* Wait to be woken up when we are needed. */
> -				__set_current_state(TASK_INTERRUPTIBLE);
> -				schedule();
> -			}
> -		} else
> -			schedule_timeout_interruptible(1);
> +	spin_unlock_irqrestore(&(smi_info->si_lock), flags);
> +	busy_wait = ipmi_thread_busy_wait(smi_result, smi_info);
> +
> +	if (smi_result == SI_SM_CALL_WITHOUT_DELAY)
> +		goto next;
> +	if (smi_result == SI_SM_CALL_WITH_DELAY && busy_wait) {
> +		queue_delayed_kthread_work(smi_info->worker,
> +					   &smi_info->work, 0);
> +	} else if (smi_result == SI_SM_IDLE) {
> +		if (atomic_read(&smi_info->need_watch)) {
> +			queue_delayed_kthread_work(smi_info->worker,
> +						   &smi_info->work, 100);
> +		} else {
> +			/* Nope. Wait to be queued when we are needed. */
> +		}
> +	} else {
> +		queue_delayed_kthread_work(smi_info->worker,
> +					   &smi_info->work, 1);
>  	}
> -	return 0;
>  }
>  
> -
>  static void poll(void *send_info)
>  {
>  	struct smi_info *smi_info = send_info;
> @@ -1229,17 +1228,29 @@ static int smi_start_processing(void       *send_info,
>  		enable = 1;
>  
>  	if (enable) {
> -		new_smi->thread = kthread_run(ipmi_thread, new_smi,
> -					      "kipmi%d", new_smi->intf_num);
> -		if (IS_ERR(new_smi->thread)) {
> +		struct kthread_worker *worker;
> +
> +		worker = create_kthread_worker(0, "kipmi%d",
> +					       new_smi->intf_num);
> +
> +		if (IS_ERR(worker)) {
>  			dev_notice(new_smi->dev, "Could not start"
>  				   " kernel thread due to error %ld, only using"
>  				   " timers to drive the interface\n",
> -				   PTR_ERR(new_smi->thread));
> -			new_smi->thread = NULL;
> +				   PTR_ERR(worker));
> +			goto out;
>  		}
> +
> +		ipmi_si_set_not_busy(&new_smi->busy_until);
> +		set_user_nice(worker->task, MAX_NICE);
> +
> +		init_delayed_kthread_work(&new_smi->work, ipmi_func);
> +		queue_delayed_kthread_work(worker, &new_smi->work, 0);
> +
> +		new_smi->worker = worker;
>  	}
>  
> +out:
>  	return 0;
>  }
>  
> @@ -3414,8 +3425,13 @@ static void check_for_broken_irqs(struct smi_info *smi_info)
>  
>  static inline void wait_for_timer_and_thread(struct smi_info *smi_info)
>  {
> -	if (smi_info->thread != NULL)
> -		kthread_stop(smi_info->thread);
> +	if (smi_info->worker != NULL) {
> +		struct kthread_worker *worker = smi_info->worker;
> +
> +		smi_info->worker = NULL;
> +		cancel_delayed_kthread_work_sync(&smi_info->work);
> +		destroy_kthread_worker(worker);
> +	}
>  	if (smi_info->timer_running)
>  		del_timer_sync(&smi_info->si_timer);
>  }

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1276378 — Re: [PATCH v3 17/22] ipmi: Convert kipmi kthread into kthread worker API

On Mon 2015-11-23 13:36:06, Corey Minyard wrote:
> 
> 
> On 11/18/2015 07:25 AM, 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 kipmi kthread into the kthread worker API because
> > it modifies the scheduling priority. The change is quite straightforward.
> 
> I think this is correct.  That code was hard to get right, but I don't
> see where any
> logic is actually changed.

I believe that it was hard to make it working.


> This also doesn't really look any simpler (you end up with more LOC than
> you did before :) ),
> though it will make things more consistent and reduce errors and that's
> a good thing.

I have just realized that the original code actually looks racy. For
example, it does:

	__set_current_state(TASK_INTERRUPTIBLE);
	schedule();

without rechecking the state in between. There might already be a new
message and it might miss the wake_up_process(). Similar problem is
with the schedule_timeout_interruptible(100); I mean:


CPU 0					CPU 1


ipmi_thread()
  spin_lock_irqsave();
  smi_result = smi_event_handler();
  spin_unlock_irqrestore();

  [...]
  else if (smi_result == SI_SM_IDLE)
    /* true */
    if (atomic_read(need_watch)) {
      /* true */

					sender()
					  spin_lock_irqsave()
					  check_start_timer_thread()
					    wake_up_process()

					    /*
					     * NOPE because kthread
					     * is not sleeping
					     */

     schedule_timeout_interruptible(100);

     /*
      * We sleep 100 jiffies but
      * there is a pending message.
      */


This is not a problem with the kthread worker API because

	mod_delayed_kthread_work(smi_info->worker,
				 &smi_info->work, 0);

would queue the work to be done immediately and

	queue_delayed_kthread_work(smi_info->worker,
				   &smi_info->work, 100);

would do nothing in this case.


> My only comment is I would like the worker function named ipmi_worker,
> not ipmi_func.

You probably want it because the original name was ipmi_thread. But
it might cause confusion with new_smi->worker. The function gets
assigned to work->func, see struct kthread_work. Therefore I think that
_func suffix makes more sense.

> Reviewed-by: Corey Minyard <cminyard@mvista.com>


Thanks a lot for review,
Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1276464 — Re: [PATCH v3 17/22] ipmi: Convert kipmi kthread into kthread worker API

On 11/24/2015 06:12 AM, Petr Mladek wrote:
> On Mon 2015-11-23 13:36:06, Corey Minyard wrote:
>>
>> On 11/18/2015 07:25 AM, 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 kipmi kthread into the kthread worker API because
>>> it modifies the scheduling priority. The change is quite straightforward.
>> I think this is correct.  That code was hard to get right, but I don't
>> see where any
>> logic is actually changed.
> I believe that it was hard to make it working.
>
>
>> This also doesn't really look any simpler (you end up with more LOC than
>> you did before :) ),
>> though it will make things more consistent and reduce errors and that's
>> a good thing.
> I have just realized that the original code actually looks racy. For
> example, it does:
>
> 	__set_current_state(TASK_INTERRUPTIBLE);
> 	schedule();
>
> without rechecking the state in between. There might already be a new
> message and it might miss the wake_up_process(). Similar problem is
> with the schedule_timeout_interruptible(100); I mean:
>
>
> CPU 0					CPU 1
>
>
> ipmi_thread()
>   spin_lock_irqsave();
>   smi_result = smi_event_handler();
>   spin_unlock_irqrestore();
>
>   [...]
>   else if (smi_result == SI_SM_IDLE)
>     /* true */
>     if (atomic_read(need_watch)) {
>       /* true */
>
> 					sender()
> 					  spin_lock_irqsave()
> 					  check_start_timer_thread()
> 					    wake_up_process()
>
> 					    /*
> 					     * NOPE because kthread
> 					     * is not sleeping
> 					     */
>
>      schedule_timeout_interruptible(100);
>
>      /*
>       * We sleep 100 jiffies but
>       * there is a pending message.
>       */

Yes, I knew the code was racy, but this is a performance optimization and
it wasn't that important to get it perfect.  The thread wouldn't actually
wait 100 jiffies, it would just be run by timer interrupts for that time.

>
> This is not a problem with the kthread worker API because
>
> 	mod_delayed_kthread_work(smi_info->worker,
> 				 &smi_info->work, 0);
>
> would queue the work to be done immediately and
>
> 	queue_delayed_kthread_work(smi_info->worker,
> 				   &smi_info->work, 100);
>
> would do nothing in this case.

And indeed this is a lot better.

>
>> My only comment is I would like the worker function named ipmi_worker,
>> not ipmi_func.
> You probably want it because the original name was ipmi_thread. But
> it might cause confusion with new_smi->worker. The function gets
> assigned to work->func, see struct kthread_work. Therefore I think that
> _func suffix makes more sense.

My problem with _func is that it's way too generic.  Is this a function
that handled IPMI messages?  Message done handling?  I'm not enamored
with my name, but I want something that gives a better indication of
what the function does.  ipmi_kthread_worker_func() would be fine with me.

Thanks,

-corey

>> Reviewed-by: Corey Minyard <cminyard@mvista.com>
>
> Thanks a lot for review,
> Petr

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web