Messages in this thread | | | Date | Mon, 23 Jul 2018 11:49:04 +0200 | From | Peter Zijlstra <> | Subject | Re: [RFC PATCH] sched/deadline: sched_getattr() returns absolute dl-task information |
| |
On Fri, Jun 29, 2018 at 02:09:47PM +0200, Alessio Balsini wrote:
Joel nailed it wrt the Changelog, that needs improvement.
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c > index fbfc3f1d368a..f75a4169cd47 100644 > --- a/kernel/sched/deadline.c > +++ b/kernel/sched/deadline.c > @@ -2568,13 +2568,41 @@ void __setparam_dl(struct task_struct *p, const struct sched_attr *attr) > dl_se->dl_density = to_ratio(dl_se->dl_deadline, dl_se->dl_runtime); > } > > -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();
This isn't required here. The reason it is used elsewhere is because those are interrupts, but this is a system call, the clock state should be good.
> + update_rq_clock(rq); > + task_tick_dl(rq, p, 0);
Do we really want task_tick_dl() here, or update_curr_dl()? Also, who says the task still is dl ? :-)
> + task_rq_unlock(rq, p, &rf); > + } > + > + /* > + * If the task is throttled, this value could be negative, > + * but sched_runtime is unsigned. > + */ > + attr->sched_runtime = dl_se->runtime <= 0 ? 0 : dl_se->runtime; > + attr->sched_deadline = dl_se->deadline;
This is all very racy..
Even if the task wasn't running when you did the task_running() test, it could be running now. And if it was running, it might not be running anymore by the time you've acquired the rq->lock.
On 32bit reading these numbers without locks is broken to boot. And even on 64bit, I suppose you can a consistent snapshot of runtime and deadline together, which isn't possible without the locks.
And of course, by the time we get back to userspace, the returned values will be out-of-date anyway. But that isn't to be helped I suppose.
> + } else { > + attr->sched_runtime = dl_se->dl_runtime; > + attr->sched_deadline = dl_se->dl_deadline; > + } > + > attr->sched_period = dl_se->dl_period; > attr->sched_flags = dl_se->flags; > }
| |