Hi,
On 05/12/17 11:55, Patrick Bellasi wrote:
> Hi Juri,
>
> On 04-Dec 11:23, Juri Lelli wrote:
> [...]
>
> > diff --git a/kernel/sched/cpufreq_schedutil.c
> > b/kernel/sched/cpufreq_schedutil.c
> > index de1ad1fffbdc..c22457868ee6 100644
> > --- a/kernel/sched/cpufreq_schedutil.c
> > +++ b/kernel/sched/cpufreq_schedutil.c
> > @@ -475,7 +475,20 @@ static void sugov_policy_free(struct sugov_policy
> > *sg_policy)
> > static int sugov_kthread_create(struct sugov_policy *sg_policy)
> > {
> > struct task_struct *thread;
> > - struct sched_param param = { .sched_priority = MAX_USER_RT_PRIO / 2 };
> > + struct sched_attr attr = {
> > + .size = sizeof(struct sched_attr),
> > + .sched_policy = SCHED_DEADLINE,
> > + .sched_flags = SCHED_FLAG_SUGOV,
> > + .sched_nice = 0,
> > + .sched_priority = 0,
> > + /*
> > + * Fake (unused) bandwidth; workaround to "fix"
> > + * priority inheritance.
> > + */
> > + .sched_runtime = 1000000,
> > + .sched_deadline = 10000000,
> > + .sched_period = 10000000,
>
> Why not assigning a minimal (but yet CBS accounted) bandwidth to
> this DL task?
>
> I understand that it should be a minimal task which bandwidth
> requirement is likely into the "noise".
> Is there any other more specific reason?
>
At least two, IMHO.
1. Throttling: assigning any sort of bandwidth is difficult (every
platform is different), and if that is too small the task responsible
for changing frequency might be throttled and delayed; if too big you
are wasting resources.
2. Affinity: some platform affine these kthreads to related_cpus; and it
is something you might want to do to save power anyway. Problem with DL
is that (at least currently) you are not free to change a task's
affinity mask without creating an exclusive cpuset.
[...]
> > +static inline
> > +void add_rq_bw(struct sched_dl_entity *dl_se, struct dl_rq *dl_rq)
> > +{
> > + if (!(dl_se->flags & SCHED_FLAG_SUGOV))
> > + __add_rq_bw(dl_se->dl_bw, dl_rq);
>
> What about using for all these wrappers the same utility function you
> already use in this source file? I.e.
>
> if (unlikely(dl_entity_is_special(dl_se)))
> return;
> __add_rq_bw(dl_se->dl_bw, dl_rq);
Should work. I'll try to do the change.
[...]
> > @@ -2436,6 +2472,9 @@ int sched_dl_overflow(struct task_struct *p, int
> > policy,
> > u64 new_bw = dl_policy(policy) ? to_ratio(period, runtime) : 0;
> > int cpus, err = -1;
> >
> > + if (attr->sched_flags & SCHED_FLAG_SUGOV)
> > + return 0;
> > +
>
> Same note on using:
>
> if (unlikely(dl_entity_is_special(dl_se)))
>
> here and in the next chunk too.
OK.
>
> > /* !deadline task may carry old deadline bandwidth */
> > if (new_bw == p->dl.dl_bw && task_has_dl_policy(p))
> > return 0;
> > @@ -2522,6 +2561,10 @@ void __getparam_dl(struct task_struct *p, struct
> > sched_attr *attr)
> > */
> > bool __checkparam_dl(const struct sched_attr *attr)
> > {
> > + /* special dl tasks don't actually use any parameter */
> > + if (attr->sched_flags & SCHED_FLAG_SUGOV)
> > + return true;
> > +
> > /* deadline != 0 */
> > if (attr->sched_deadline == 0)
> > return false;
> > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > index a1730e39cbc6..280b421a82e8 100644
> > --- a/kernel/sched/sched.h
> > +++ b/kernel/sched/sched.h
> > @@ -156,13 +156,33 @@ static inline int task_has_dl_policy(struct
> > task_struct *p)
> > return dl_policy(p->policy);
> > }
> >
> > +/*
> > + * !! For sched_setattr_nocheck() (kernel) only !!
> > + *
> > + * This is actually gross. :(
> > + *
> > + * It is used to make schedutil kworker(s) higher priority than
> > SCHED_DEADLINE
> > + * tasks, but still be able to sleep. We need this on platforms that cannot
> > + * atomically change clock frequency. Remove once fast switching will be
> > + * available on such platforms.
> > + *
> > + * SUGOV stands for SchedUtil GOVernor.
> > + */
> > +#define SCHED_FLAG_SUGOV 0x10000000
> > +
> > +static inline int dl_entity_is_special(struct sched_dl_entity *dl_se)
>
> This should better return a bool...
>
>
> > +{
>
> ... and maybe it can optimize some builds via constants propagations to add:
>
> #ifdef CONFIG_CPU_FREQ_GOV_SCHEDUTIL
> > + return dl_se->flags & SCHED_FLAG_SUGOV;
> #else
> return false;
> #endif
Sure.
>
> > +}
> > +
> > /*
> > * Tells if entity @a should preempt entity @b.
> > */
> > static inline bool
> > dl_entity_preempt(struct sched_dl_entity *a, struct sched_dl_entity *b)
> > {
> > - return dl_time_before(a->deadline, b->deadline);
> > + return dl_entity_is_special(a) ||
> > + dl_time_before(a->deadline, b->deadline);
>
> Given that being special is less likely, perhaps better to have:
>
> return dl_time_before(a->deadline, b->deadline) ||
> dl_entity_is_special(a);
OK.
Thanks for the review!
Best,
- Juri
