Hi, Juri, 13.01.2015, 11:10, "Juri Lelli" <juri.le...@arm.com>: > Hi all, > > really sorry for the huge delay in replying to this! :( > > On 07/01/2015 12:29, Kirill Tkhai wrote: >> On Ср, 2015-01-07 at 08:01 +0100, Luca Abeni wrote: >>> Hi Kirill, >>> >>> On Tue, 06 Jan 2015 02:07:21 +0300 >>> Kirill Tkhai <tk...@yandex.ru> wrote: >>>> On Пн, 2015-01-05 at 16:21 +0100, Luca Abeni wrote: >>> [...] >>>>> For reference, I attach the patch I am using locally (based on what >>>>> I suggested in my previous mail) and seems to work fine here. >>>>> >>>>> Based on your comments, I suspect my patch can be further >>>>> simplified by moving the call to init_dl_task_timer() in >>>>> __sched_fork(). >>>> It seems this way has problems. The first one is that task may become >>>> throttled again, and we will start dl_timer again. >>> Well, in my understanding if I change the parameters of a >>> SCHED_DEADLINE task when it is throttled, it stays throttled... So, the >>> task might not become throttled again before the dl timer fires. >>> So, I hoped this problem does not exist. But I might be wrong. >> You keep zeroing of dl_se->dl_throttled, and further enqueue_task() >> places it on the dl_rq. So, further update_curr_dl() may make it throttled >> again, and it will try to start dl_timer (which is already set). >>>> The second is that >>>> it's better to minimize number of combination of situations we have. >>>> Let's keep only one combination: timer is set <-> task is throttled. >>> Yes, this was my goal too... So, if I change the parameters of a task >>> when it is throttled, I leave dl_throttled set to 1 and I leave the >>> timer active. >> As I see, >> >> dl_se->dl_throttled = 0; >> >> is still in __setparam_dl() after your patch, so you do not leave >> it set to 1. >>> [...] >>>>>> @@ -3250,16 +3251,19 @@ static void >>>>>> __setparam_dl(struct task_struct *p, const struct sched_attr >>>>>> *attr) { >>>>>> struct sched_dl_entity *dl_se = &p->dl; >>>>>> + struct hrtimer *timer = &dl_se->dl_timer; >>>>>> + >>>>>> + if (!hrtimer_active(timer) || >>>>>> hrtimer_try_to_cancel(timer) != -1) { >>>>> Just for the sake of curiosity, why trying to cancel the timer >>>>> ("|| hrtimer_try_to_cancel(timer)") here? If it is active, cannot >>>>> we leave it active (without touching dl_throttled, dl_new and >>>>> dl_yielded)? >>>>> >>>>> I mean: if I try to change the parameters of a task when it is >>>>> throttled, I'd like it to stay throttled until the end of the >>>>> reservation period... Or am I missing something? >>>> I think that when people change task's parameters, they want the >>>> kernel reacts on this immediately. For example, you want to kill >>>> throttled deadline task. You change parameters, but nothing happens. >>>> I think all developers had this use case when they were debugging >>>> deadline class. >>> I see... Different people have different requirements :) >>> My goal was to do something like adaptive scheduling (or scheduling >>> tasks with mode changes), so I did not want that changing the >>> scheduling parameters of a task affected the scheduling of the other >>> tasks... But if a task exits the throttled state when I change its >>> parameters, it might consume much more than the reserved CPU time. >>> Also, I suspect this kind of approach can be exploited by malicious >>> users: if I create a task with runtime 30ms and period 100ms, and I >>> change its scheduling parameters (to runtime=29ms and back) frequently >>> enough, I can consume much more than 30% of the CPU time... > > Well, I'm inclined to agree to Luca's viewpoint. We should not change > parameters of a throttled task or we may affect other tasks.
Could you explain your viewpoint more? How does this affects other tasks? As I understand, in __setparam_dl() we are sure that there is enough dl_bw. In __sched_setscheduler() we call it after dl_overflow() check. >>> Anyway, I am fine with every patch that fixes the bug :) >> Deadline class requires root privileges. So, I do not see a problem >> here. Please, see __sched_setscheduler(). >> >> If in the future we allow non-privileged users to increase deadline, >> we will reflect that in __setparam_dl() too. > > I'd say it is better to implement the right behavior even for root, so > that we will find it right when we'll grant access to non root users > too. Also, even if root can do everything, we always try not to break > guarantees that come with admission control (root or non root that is). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/