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 <cminy...@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 <pmla...@suse.com>
> CC: Corey Minyard <miny...@acm.org>
> CC: openipmi-develo...@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-api" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
