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

Groups > linux.kernel > #1292735 > unrolled thread

Re: [RFCv6 PATCH 03/10] sched: scheduler-driven cpu frequency selection

Started byLeo Yan <leo.yan@linaro.org>
First post2015-12-16 04:50 +0100
Last post2015-12-19 07:00 +0100
Articles 5 — 2 participants

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

  Re: [RFCv6 PATCH 03/10] sched: scheduler-driven cpu frequency  selection Leo Yan <leo.yan@linaro.org> - 2015-12-16 04:50 +0100
    Re: [RFCv6 PATCH 03/10] sched: scheduler-driven cpu frequency  selection Steve Muckle <steve.muckle@linaro.org> - 2015-12-17 02:30 +0100
      Re: [RFCv6 PATCH 03/10] sched: scheduler-driven cpu frequency  selection Leo Yan <leo.yan@linaro.org> - 2015-12-17 08:20 +0100
        Re: [RFCv6 PATCH 03/10] sched: scheduler-driven cpu frequency  selection Steve Muckle <steve.muckle@linaro.org> - 2015-12-18 20:20 +0100
          Re: [RFCv6 PATCH 03/10] sched: scheduler-driven cpu frequency  selection Leo Yan <leo.yan@linaro.org> - 2015-12-19 07:00 +0100

#1292735 — Re: [RFCv6 PATCH 03/10] sched: scheduler-driven cpu frequency selection

FromLeo Yan <leo.yan@linaro.org>
Date2015-12-16 04:50 +0100
SubjectRe: [RFCv6 PATCH 03/10] sched: scheduler-driven cpu frequency selection
Message-ID<qGbgd-4UV-5@gated-at.bofh.it>
Hi Steve,

On Tue, Dec 08, 2015 at 10:19:24PM -0800, Steve Muckle wrote:

[...]

> +static int cpufreq_sched_thread(void *data)
> +{
> +	struct sched_param param;
> +	struct cpufreq_policy *policy;
> +	struct gov_data *gd;
> +	unsigned int new_request = 0;
> +	unsigned int last_request = 0;
> +	int ret;
> +
> +	policy = (struct cpufreq_policy *) data;
> +	gd = policy->governor_data;
> +
> +	param.sched_priority = 50;
> +	ret = sched_setscheduler_nocheck(gd->task, SCHED_FIFO, &param);
> +	if (ret) {
> +		pr_warn("%s: failed to set SCHED_FIFO\n", __func__);
> +		do_exit(-EINVAL);
> +	} else {
> +		pr_debug("%s: kthread (%d) set to SCHED_FIFO\n",
> +				__func__, gd->task->pid);
> +	}
> +
> +	do {
> +		set_current_state(TASK_INTERRUPTIBLE);
> +		new_request = gd->requested_freq;
> +		if (new_request == last_request) {
> +			schedule();
> +		} else {
> +			/*
> +			 * if the frequency thread sleeps while waiting to be
> +			 * unthrottled, start over to check for a newer request
> +			 */
> +			if (finish_last_request(gd))
> +				continue;
> +			last_request = new_request;
> +			cpufreq_sched_try_driver_target(policy, new_request);
> +		}

I also think "set_current_state(TASK_INTERRUPTIBLE)" will introduce
logic error when software flow run into "else" block. The reason is
after you set state with TASK_INTERRUPTIBLE, if there have some
scheduling happen within cpufreq_sched_try_driver_target(), then the
thread will be remove from rq. But generally we suppose the thread
will be on rq and can continue run after next tick.

Juri's suggestion can fix this issue. And we can use atomic_t to
safely accessing gd->requested_freq.

[...]

Thanks,
Leo Yan
--
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]

#1293544

FromSteve Muckle <steve.muckle@linaro.org>
Date2015-12-17 02:30 +0100
Message-ID<qGvyi-13b-11@gated-at.bofh.it>
In reply to#1292735
Hi Leo,

On 12/15/2015 07:48 PM, Leo Yan wrote:
> I also think "set_current_state(TASK_INTERRUPTIBLE)" will introduce
> logic error when software flow run into "else" block. The reason is
> after you set state with TASK_INTERRUPTIBLE, if there have some
> scheduling happen within cpufreq_sched_try_driver_target(), then the
> thread will be remove from rq. But generally we suppose the thread
> will be on rq and can continue run after next tick.
> 
> Juri's suggestion can fix this issue. And we can use atomic_t to
> safely accessing gd->requested_freq.

I agree, it's incorrect. As I replied earlier I believe setting the task
state back to TASK_RUNNING at the top of the else block is the easiest fix.

thanks,
Steve
--
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]

#1293652

FromLeo Yan <leo.yan@linaro.org>
Date2015-12-17 08:20 +0100
Message-ID<qGB10-4zr-11@gated-at.bofh.it>
In reply to#1293544
Hi Steve,

On Wed, Dec 16, 2015 at 05:24:56PM -0800, Steve Muckle wrote:
> Hi Leo,
> 
> On 12/15/2015 07:48 PM, Leo Yan wrote:
> > I also think "set_current_state(TASK_INTERRUPTIBLE)" will introduce
> > logic error when software flow run into "else" block. The reason is
> > after you set state with TASK_INTERRUPTIBLE, if there have some
> > scheduling happen within cpufreq_sched_try_driver_target(), then the
> > thread will be remove from rq. But generally we suppose the thread
> > will be on rq and can continue run after next tick.
> > 
> > Juri's suggestion can fix this issue. And we can use atomic_t to
> > safely accessing gd->requested_freq.
> 
> I agree, it's incorrect. As I replied earlier I believe setting the task
> state back to TASK_RUNNING at the top of the else block is the easiest fix.

Could you check if below corner case will introduce logic error?
The task still will be removed from rq if timer tick is triggered
between two time's set_current_state().

set_current_state(TASK_INTERRUPTIBLE);
           `-------> timer_tick and
                     schedule();
do_something...
set_current_state(TASK_RUNNING);

It will be safe for combination for set_current_state()/schedule()
with waken_up_process():

Thread_A:                                       Thread_B:

set_current_state(TASK_INTERRUPTIBLE);
             `-------> timer_tick and
                       schedule();
....
                                                wake_up_process(Thread_A);
                           <---------------------/
schedule();

The first time's schedule() will remove task from rq which is caused
by timer tick and call schedule(), and the second time schdule() will
be equal yeild().

Thanks,
Leo Yan
--
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]

#1295103

FromSteve Muckle <steve.muckle@linaro.org>
Date2015-12-18 20:20 +0100
Message-ID<qH8Jk-1fV-9@gated-at.bofh.it>
In reply to#1293652
Hi Leo,

On 12/16/2015 11:17 PM, Leo Yan wrote:
> Could you check if below corner case will introduce logic error?
> The task still will be removed from rq if timer tick is triggered
> between two time's set_current_state().
> 
> set_current_state(TASK_INTERRUPTIBLE);
>            `-------> timer_tick and
>                      schedule();
> do_something...
> set_current_state(TASK_RUNNING);
> 
> It will be safe for combination for set_current_state()/schedule()
> with waken_up_process():
> 
> Thread_A:                                       Thread_B:
> 
> set_current_state(TASK_INTERRUPTIBLE);
>              `-------> timer_tick and
>                        schedule();
> ....
>                                                 wake_up_process(Thread_A);
>                            <---------------------/
> schedule();
> 
> The first time's schedule() will remove task from rq which is caused
> by timer tick and call schedule(), and the second time schdule() will
> be equal yeild().

I was initially concerned about preemption while task state =
TASK_INTERRUPTIBLE as well, but a task with state TASK_INTERRUPTIBLE is
not dequeued if it is preempted. See core.c:__schedule():

        if (!preempt && prev->state) {
                if (unlikely(signal_pending_state(prev->state, prev))) {
                        prev->state = TASK_RUNNING;
                } else {
                        deactivate_task(rq, prev, DEQUEUE_SLEEP);
                        prev->on_rq = 0;

I knew this had to be the case, because this design pattern is used in
many other places in the kernel, so many things would be very broken if
this were a problem.

thanks,
Steve

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

#1295312

FromLeo Yan <leo.yan@linaro.org>
Date2015-12-19 07:00 +0100
Message-ID<qHiIF-7zM-1@gated-at.bofh.it>
In reply to#1295103
Hi Steve,

On Fri, Dec 18, 2015 at 11:15:01AM -0800, Steve Muckle wrote:
> On 12/16/2015 11:17 PM, Leo Yan wrote:
> > Could you check if below corner case will introduce logic error?
> > The task still will be removed from rq if timer tick is triggered
> > between two time's set_current_state().
> > 
> > set_current_state(TASK_INTERRUPTIBLE);
> >            `-------> timer_tick and
> >                      schedule();
> > do_something...
> > set_current_state(TASK_RUNNING);
> > 
> > It will be safe for combination for set_current_state()/schedule()
> > with waken_up_process():
> > 
> > Thread_A:                                       Thread_B:
> > 
> > set_current_state(TASK_INTERRUPTIBLE);
> >              `-------> timer_tick and
> >                        schedule();
> > ....
> >                                                 wake_up_process(Thread_A);
> >                            <---------------------/
> > schedule();
> > 
> > The first time's schedule() will remove task from rq which is caused
> > by timer tick and call schedule(), and the second time schdule() will
> > be equal yeild().
> 
> I was initially concerned about preemption while task state =
> TASK_INTERRUPTIBLE as well, but a task with state TASK_INTERRUPTIBLE is
> not dequeued if it is preempted. See core.c:__schedule():
> 
>         if (!preempt && prev->state) {
>                 if (unlikely(signal_pending_state(prev->state, prev))) {
>                         prev->state = TASK_RUNNING;
>                 } else {
>                         deactivate_task(rq, prev, DEQUEUE_SLEEP);
>                         prev->on_rq = 0;
> 
> I knew this had to be the case, because this design pattern is used in
> many other places in the kernel, so many things would be very broken if
> this were a problem.

You are right, I went through the code again and sched tick irq will
call preempt_schedule_irq() and __schedule(true); so finally set the
parameter "preempt" = true. Sorry for noise :p

---8<---

arch/arm64/kernel/entry.S:

#ifdef CONFIG_PREEMPT
el1_preempt:
        mov     x24, lr
1:      bl      preempt_schedule_irq            // irq en/disable is done inside
        ldr     x0, [tsk, #TI_FLAGS]            // get new tasks TI_FLAGS
        tbnz    x0, #TIF_NEED_RESCHED, 1b       // needs rescheduling?
        ret     x24
#endif

Thanks,
Leo Yan
--
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