Re: [PATCH v3] sched/deadline: remove useless param from setup_new_dl_entity

[luca abeni <luca.abeni@unitn.it>]

On Mon, 11 Jul 2016 16:16:20 +0800
Xunlei Pang <xpang@redhat.com> wrote:

> On 2016/07/11 at 16:01, luca abeni wrote:
> > Hello,
> >
> > On Mon, 11 Jul 2016 13:03:56 +0800
> > Xunlei Pang <xpang@redhat.com> wrote:
> >  
> >> On 2016/07/08 at 19:28, Juri Lelli wrote:  
> > [...]  
> >>> @@ -363,6 +364,15 @@ static inline void setup_new_dl_entity(struct
> >>> sched_dl_entity *dl_se, return;
> >>>  
> >>>  	/*
> >>> +	 * Use the scheduling parameters of the top pi-waiter
> >>> task,
> >>> +	 * if we have one from which we can inherit a deadline.
> >>> +	 */
> >>> +	if (dl_se->dl_boosted &&
> >>> +	    (pi_task = rt_mutex_get_top_task(dl_task_of(dl_se)))
> >>> &&
> >>> +	    dl_prio(pi_task->normal_prio))
> >>> +		pi_se = &pi_task->dl;
> >>> +
> >>> +	/*
> >>>  	 * We use the regular wall clock time to set deadlines in
> >>> the
> >>>  	 * future; in fact, we must consider execution overheads
> >>> (time
> >>>  	 * spent on hardirq context, etc.).
> >>> @@ -1721,7 +1731,7 @@ static void switched_from_dl(struct rq *rq,
> >>> struct task_struct *p) static void switched_to_dl(struct rq *rq,
> >>> struct task_struct *p) {
> >>>  	if (dl_time_before(p->dl.deadline, rq_clock(rq)))
> >>> -		setup_new_dl_entity(&p->dl, &p->dl);
> >>> +		setup_new_dl_entity(&p->dl);    
> >> I'm curious why we even call setup_new_dl_entity() for non-queued
> >> cases? It seems more reasonable to do it when it really gets
> >> queued. We can see that enqueue_task_dl()->update_dl_entity() also
> >> has the same update logic as switched_to_dl().  
> > I wondered the same when removing the dl_new field from
> > sched_dl_entity... But then I realised that enqueue_dl_entity() does
> > not always invoke update_dl_entity() or replenish_dl_entity()... For
> > example, when a task switches from SCHED_OTHER (or RT) to -deadline
> > due to sched_setattr() (or similar) these functions are not
> > invoked.  
> 
> Yeah, but for wake-up cases it does, as ENQUEUE_WAKEUP is set.
> What I meant is, can we only update for queued tasks in
> switched_to_dl()?

Uhm... Looking at this code again, I am not sure anymore about it...
(switched_to_dl() is invoked after enqueue enqueue_task_dl()?)

Maybe the best thing to do is to move the
"if (dl_time_before(p->dl.deadline, rq_clock(rq)))"
check in enqueue_dl_entity() (Juri, what do you think)? I'll try this
approach in the next days.



			Thanks,
				Luca