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

[Alessio Balsini]

Right!

Verified that sysctl_sched_dl_period_max and sysctl_sched_dl_period_min values
are now always consistent.

I spent some time in trying to figure out if not having any mutex in
__checkparam_dl() is safe. There can surely happen that "max < min", e.g.:

          |              |               periods
User1     | User2        | checkparam_dl()  | sysctl_sched_dl_*
----------|--------------|------------------|-------------------
          |              |                  | [x, x]
p_min = 5 |              |                  |
          |              |                  | [5, x]
p_max = 5 |              |                  |
          |              |                  | [5, 5]
          | setattr(p=8) |                  |
          |              | p = 8            |
          |              | [x, 5]           |
p_max = 9 |              |                  |
          |              |                  | [5, 9]
p_min = 6 |              |                  |
          |              |                  | [6, 9]
          |              | [6, 5]           |
----------|--------------|------------------|-------------------

Sharing my thoughts, a "BUG_ON(max < min)" in __checkparam_dl() is then a
guaranteed source of explosions, but the good news is that "if (period < min ||
period > max" in __checkparam_dl() surely fails if "max < min".  Also the fact
that, when we are writing the new sysctl_sched_dl_* values, only one is
actually changed at a time, that surely helps to preserve the consistency.

But is that enough?

Well, I couldn't find any counterexample to make __checkparam_dl() pass with
wrong parameters. And the tests I made are happy.

On Thu, Aug 22, 2019 at 06:51:25PM +0200, Peter Zijlstra wrote:
>  include/linux/sched/sysctl.h |    7 +++++
>  kernel/sched/deadline.c      |   58 +++++++++++++++++++++++++++++++++++++++++--
>  kernel/sysctl.c              |   14 ++++++++++
>  3 files changed, 77 insertions(+), 2 deletions(-)
> 
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> +int sched_dl_period_handler(struct ctl_table *table, int write,
> +			    void __user *buffer, size_t *lenp,
> +			    loff_t *ppos)
> +{
> +		if (min > 1ULL << DL_SCALE && max > min) {

s/>/>=/

> +			WRITE_ONCE(sysctl_sched_dl_period_max, new_max);
> +			WRITE_ONCE(sysctl_sched_dl_period_min, new_min);

Besides the inline comment above, this is my ack to your patch.

Otherwise, here follows a slightly more convoluted version of your patch with a
couple of changes to sched_dl_period_handler(), summarized as:
 - handle new_table only if writing;
 - directly compare the us min and max (saves one multiplication);
 - atomic-writes only the sysctl_sched_dl_period_XXX which changed.

M2c.

Thanks!
-Alessio

---