Messages in this thread | | | Date | Tue, 15 May 2012 12:10:16 +0200 | From | Juri Lelli <> | Subject | Re: [PATCH 05/16] sched: SCHED_DEADLINE policy implementation. |
| |
On 04/23/2012 05:43 PM, Peter Zijlstra wrote: > On Mon, 2012-04-23 at 17:39 +0200, Juri Lelli wrote: >> On 04/23/2012 04:35 PM, Peter Zijlstra wrote: >>> On Fri, 2012-04-06 at 09:14 +0200, Juri Lelli wrote: >>>> +static void init_dl_task_timer(struct sched_dl_entity *dl_se) >>>> +{ >>>> + struct hrtimer *timer =&dl_se->dl_timer; >>>> + >>>> + if (hrtimer_active(timer)) { >>>> + hrtimer_try_to_cancel(timer); >>>> + return; >>>> + } >>> >>> Same question I guess, how can it be active here? Also, just letting it >>> run doesn't seem like the best way out.. >>> >> >> Probably s/hrtimer_try_to_cancel/hrtimer_cancel is better. > > Yeah, not sure you can do hrtimer_cancel() there though, you're holding > ->pi_lock and rq->lock and have IRQs disabled. That sounds like asking > for trouble. > > Anyway, if it can't happen, we don't have to fix it.. so lets answer > that first ;-)
Even if I dropped the bits for allowing !root users, this critical point still remains. What if I leave this like it is and instead I do the following?
@@ -488,9 +488,10 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer) /* * We need to take care of a possible races here. In fact, the * task might have changed its scheduling policy to something - * different from SCHED_DEADLINE (through sched_setscheduler()). + * different from SCHED_DEADLINE or changed its reservation + * parameters (through sched_{setscheduler(),setscheduler2()}). */ - if (!dl_task(p)) + if (!dl_task(p) || dl_se->dl_new) goto unlock; dl_se->dl_throttled = 0;
The idea is that hrtimer_try_to_cancel should fail only if the callback routine is running. If, meanwhile, I set up new parameters, I can try to recognize this situation through dl_new (set to 1 during __setparam_dl).
BTW, I'd have a new version ready (also rebased on the current tip/master). It address all the comments excluding your gcc work-around, math128 and nr_cpus_allowed shift (patches are ready but those changes not yet mainline, right?). Anyway, do you think would be fine to post it?
Thanks and Regards,
- Juri
| |