Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 2 Aug 2019 18:21:04 +0100
From:   Alessio Balsini <balsini@android.com>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     mingo@kernel.org, juri.lelli@redhat.com,
        linux-kernel@vger.kernel.org, dietmar.eggemann@arm.com,
        luca.abeni@santannapisa.it, bristot@redhat.com, dvyukov@google.com,
        tglx@linutronix.de, vpillai@digitalocean.com, rostedt@goodmis.org
Subject: Re: [RFC][PATCH 01/13] sched/deadline: Impose global limits on
 sched_attr::sched_period
Message-ID: <20190802172104.GA134279@google.com>
References: <20190726145409.947503076@infradead.org>
 <20190726161357.397880775@infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190726161357.397880775@infradead.org>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Peter,

This is indeed an important missing piece.

I think it would be worth having some simple checks, e.g.:
- period_min <= period_max;
- period_min >= (1ULL << DL_SCALE).

To be even more picky, I'm wondering if it would be a good practice to
fail the write operation if there are already dl-tasks in the system
that violate the new constraint.  I'm afraid there is no cheap way of
achieving such check, so, I think we can skip this last tricky thing for
now.

Thanks,
Alessio

On Fri, Jul 26, 2019 at 04:54:10PM +0200, Peter Zijlstra wrote:
> 
> Cc: Daniel Bristot de Oliveira <bristot@redhat.com>
> Cc: Luca Abeni <luca.abeni@santannapisa.it>
> Cc: Juri Lelli <juri.lelli@redhat.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  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),
> 
>