Re: [RFC PATCH] sched/deadline: sched_getattr() returns absolute dl-task information

[Patrick Bellasi <patrick.bellasi@arm.com>]

On 23-Jul 16:13, Peter Zijlstra wrote:
> On Mon, Jul 23, 2018 at 01:49:46PM +0100, Patrick Bellasi wrote:
> > On 23-Jul 11:49, Peter Zijlstra wrote:
> > 
> > [...]
> > 
> > > > -void __getparam_dl(struct task_struct *p, struct sched_attr *attr)
> > > > +void __getparam_dl(struct task_struct *p, struct sched_attr *attr,
> > > > +		   unsigned int flags)
> > > >  {
> > > >  	struct sched_dl_entity *dl_se = &p->dl;
> > > >  
> > > >  	attr->sched_priority = p->rt_priority;
> > > > -	attr->sched_runtime = dl_se->dl_runtime;
> > > > -	attr->sched_deadline = dl_se->dl_deadline;
> > > > +
> > > > +	if (flags & SCHED_GETATTR_FLAGS_DL_ABSOLUTE) {
> > > > +		/*
> > > > +		 * If the task is not running, its runtime is already
> > > > +		 * properly accounted. Otherwise, update clocks and the
> > > > +		 * statistics for the task.
> > > > +		 */
> > > > +		if (task_running(task_rq(p), p)) {
> > > > +			struct rq_flags rf;
> > > > +			struct rq *rq;
> > > > +
> > > > +			rq = task_rq_lock(p, &rf);
> > > > +			sched_clock_tick();
> > > > +			update_rq_clock(rq);
> > > > +			task_tick_dl(rq, p, 0);
> > > 
> > > Do we really want task_tick_dl() here, or update_curr_dl()?
> > 
> > I think this was to cover the case of a syscall being called while the
> > task is running and we are midway between two ticks...
> 
> Sure, I know what it's there for, just saying that update_curr_dl()
> would've updated the accounting as well. Calling tick stuff from !tick
> context is a wee bit dodgy.

Right, I think it depends on how much we want to be "precise" in closing
a control loop with user-space.

On Android we have ticks every 3-4ms, I'm wondering if this maximum
"latency" on measuring the remaining run-time can introduce a too big
error for certain applications...

Alessio: you have an interesting low-latency audio use-case on hand,
do you think we can tolerate a 4ms error in remaining run-time
readings?

[...]

> > Which means we should use something like:
> > 
> >    if (flags & SCHED_GETATTR_FLAGS_DL_ABSOLUTE) {
> >         /* Lock the task and the RQ before any other check and upate */
> >         rq = task_rq_lock(p, &rf);
> > 
> >         /* Check the task is still DL ?*/
> > 
> >         /* Update task stats */
> > 
> >         task_rq_unlock(rq, p, &rf);
> >    }
> > 
> > right?
> 
> Yeah, something along those lines.
> 
> > If that's better, then we should probably even better move the
> > task_rq_lock at the beginning of SYSCALL_DEFINE4(sched_getattr()) ?
> 
> Hurm.. yes, we should probably have the has_dl_policy test under the
> lock too. Which is really annoying, because this basically turns a
> lockless syscall into locked one.

Indeed...

> Another method would be to have __getparam_dl() 'fail' and retry if it
> finds !has_dl_policy() once we have the lock. That would retain the
> lockless nature for all current use-cases and only incur the locking
> overhead for this new case.

... right, this is actually the best solution to have a bit more
guarantees for the new DL control scenarios without affecting existing
ones!

-- 
#include <best/regards.h>

Patrick Bellasi