Re: [RFC][PATCH 01/13] sched/deadline: Impose global limits on sched_attr::sched_period

From: Juri Lelli
Date: Wed May 20 2020 - 14:38:44 EST


Hi Peter,

On 26/07/19 16:54, Peter Zijlstra wrote:
>
> Cc: Daniel Bristot de Oliveira <bristot@xxxxxxxxxx>
> Cc: Luca Abeni <luca.abeni@xxxxxxxxxxxxxxx>
> Cc: Juri Lelli <juri.lelli@xxxxxxxxxx>
> Cc: Dmitry Vyukov <dvyukov@xxxxxxxxxx>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> ---
> include/linux/sched/sysctl.h | 3 +++
> kernel/sched/deadline.c | 23 +++++++++++++++++++++--
> kernel/sysctl.c | 14 ++++++++++++++
> 3 files changed, 38 insertions(+), 2 deletions(-)
>
> --- a/include/linux/sched/sysctl.h
> +++ b/include/linux/sched/sysctl.h
> @@ -56,6 +56,9 @@ int sched_proc_update_handler(struct ctl
> extern unsigned int sysctl_sched_rt_period;
> extern int sysctl_sched_rt_runtime;
>
> +extern unsigned int sysctl_sched_dl_period_max;
> +extern unsigned int sysctl_sched_dl_period_min;
> +
> #ifdef CONFIG_UCLAMP_TASK
> extern unsigned int sysctl_sched_uclamp_util_min;
> extern unsigned int sysctl_sched_uclamp_util_max;
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -2597,6 +2597,14 @@ void __getparam_dl(struct task_struct *p
> }
>
> /*
> + * Default limits for DL period; on the top end we guard against small util
> + * tasks still getting rediculous long effective runtimes, on the bottom end we
> + * guard against timer DoS.
> + */
> +unsigned int sysctl_sched_dl_period_max = 1 << 22; /* ~4 seconds */
> +unsigned int sysctl_sched_dl_period_min = 100; /* 100 us */
> +
> +/*
> * This function validates the new parameters of a -deadline task.
> * We ask for the deadline not being zero, and greater or equal
> * than the runtime, as well as the period of being zero or
> @@ -2608,6 +2616,8 @@ void __getparam_dl(struct task_struct *p
> */
> bool __checkparam_dl(const struct sched_attr *attr)
> {
> + u64 period, max, min;
> +
> /* special dl tasks don't actually use any parameter */
> if (attr->sched_flags & SCHED_FLAG_SUGOV)
> return true;
> @@ -2631,12 +2641,21 @@ bool __checkparam_dl(const struct sched_
> attr->sched_period & (1ULL << 63))
> return false;
>
> + period = attr->sched_period;
> + if (!period)
> + period = attr->sched_deadline;
> +
> /* runtime <= deadline <= period (if period != 0) */
> - if ((attr->sched_period != 0 &&
> - attr->sched_period < attr->sched_deadline) ||
> + if (period < attr->sched_deadline ||
> attr->sched_deadline < attr->sched_runtime)
> return false;
>
> + max = (u64)READ_ONCE(sysctl_sched_dl_period_max) * NSEC_PER_USEC;
> + min = (u64)READ_ONCE(sysctl_sched_dl_period_min) * NSEC_PER_USEC;
> +
> + if (period < min || period > max)
> + return false;
> +
> return true;
> }
>
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -443,6 +443,20 @@ static struct ctl_table kern_table[] = {
> .proc_handler = sched_rt_handler,
> },
> {
> + .procname = "sched_deadline_period_max_us",
> + .data = &sysctl_sched_dl_period_max,
> + .maxlen = sizeof(unsigned int),
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
> + },
> + {
> + .procname = "sched_deadline_period_min_us",
> + .data = &sysctl_sched_dl_period_min,
> + .maxlen = sizeof(unsigned int),
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
> + },
> + {
> .procname = "sched_rr_timeslice_ms",
> .data = &sysctl_sched_rr_timeslice,
> .maxlen = sizeof(int),

I think this never made it upstream. And I believe both me and Daniel
were OK with it. Do you recall if any additional change was needed?

Thanks,

Juri