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

[Leo Yan]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/