linux-kernel - Re: [RFC PATCH 11/16] sched/qos: Add rampup multiplier QoS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CANDhNCpbtzP0FXq4tmia45Bkoem3mgMVoAKHCLfvDL-5_T5sLg@mail.gmail.com>
Date: Wed, 27 Nov 2024 16:12:06 -0800
From: John Stultz <jstultz@...gle.com>
To: Qais Yousef <qyousef@...alina.io>
Cc: Ingo Molnar <mingo@...nel.org>, Peter Zijlstra <peterz@...radead.org>, 
	Vincent Guittot <vincent.guittot@...aro.org>, "Rafael J. Wysocki" <rafael@...nel.org>, 
	Viresh Kumar <viresh.kumar@...aro.org>, Juri Lelli <juri.lelli@...hat.com>, 
	Steven Rostedt <rostedt@...dmis.org>, Dietmar Eggemann <dietmar.eggemann@....com>, linux-pm@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH 11/16] sched/qos: Add rampup multiplier QoS

On Tue, Aug 20, 2024 at 9:36 AM Qais Yousef <qyousef@...alina.io> wrote:
>
> Bursty tasks are hard to predict. To use resources efficiently, the
> system would like to be exact as much as possible. But this poses
> a challenge for these bursty tasks that need to get access to more
> resources quickly.
>
> The new SCHED_QOS_RAMPUP_MULTIPLIER allows userspace to do that. As the
> name implies, it only helps them to transition to a higher performance
> state when they get _busier_. That is perfectly periodic tasks by
> definition are not going through a transition and will run at a constant
> performance level. It is the tasks that need to transition from one
> periodic state to another periodic state that is at a higher level that
> this rampup_multiplier will help with. It also slows down the ewma decay
> of util_est which should help those bursty tasks to keep their faster
> rampup.
>
> This should work complimentary with uclamp. uclamp tells the system
> about min and max perf requirements which can be applied immediately.
>
> rampup_multiplier is about reactiveness of the task to change.
> Specifically to a change for a higher performance level. The task might
> necessary need to have a min perf requirements, but it can have sudden
> burst of changes that require higher perf level and it needs the system
> to provide this faster.
>
> TODO: update the sched_qos docs
>
> Signed-off-by: Qais Yousef <qyousef@...alina.io>
> ---
>  include/linux/sched.h      |  7 ++++
>  include/uapi/linux/sched.h |  2 ++
>  kernel/sched/core.c        | 66 ++++++++++++++++++++++++++++++++++++++
>  kernel/sched/fair.c        |  6 ++--
>  kernel/sched/syscalls.c    | 38 ++++++++++++++++++++--
>  5 files changed, 115 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 2e8c5a9ffa76..a30ee43a25fb 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -404,6 +404,11 @@ struct sched_info {
>  #endif /* CONFIG_SCHED_INFO */
>  };
>
> +struct sched_qos {
> +       DECLARE_BITMAP(user_defined, SCHED_QOS_MAX);
> +       unsigned int rampup_multiplier;
> +};
> +
>  /*
>   * Integer metrics need fixed point arithmetic, e.g., sched/fair
>   * has a few: load, load_avg, util_avg, freq, and capacity.
> @@ -882,6 +887,8 @@ struct task_struct {
>
>         struct sched_info               sched_info;
>
> +       struct sched_qos                sched_qos;
> +
>         struct list_head                tasks;
>  #ifdef CONFIG_SMP
>         struct plist_node               pushable_tasks;
> diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h
> index 67ef99f64ddc..0baba91ba5b8 100644
> --- a/include/uapi/linux/sched.h
> +++ b/include/uapi/linux/sched.h
> @@ -104,6 +104,8 @@ struct clone_args {
>  };
>
>  enum sched_qos_type {
> +       SCHED_QOS_RAMPUP_MULTIPLIER,
> +       SCHED_QOS_MAX,
>  };
>  #endif
...
> +static void __setscheduler_sched_qos(struct task_struct *p,
> +                                    const struct sched_attr *attr)
> +{
> +       switch (attr->sched_qos_type) {
> +       case SCHED_QOS_RAMPUP_MULTIPLIER:
> +               set_bit(SCHED_QOS_RAMPUP_MULTIPLIER, p->sched_qos.user_defined);
> +               p->sched_qos.rampup_multiplier = attr->sched_qos_value;
> +       default:
> +               break;
> +       }
> +}
> +
>  /*
>   * Allow unprivileged RT tasks to decrease priority.
>   * Only issue a capable test if needed and only once to avoid an audit
...
> @@ -799,7 +831,9 @@ int __sched_setscheduler(struct task_struct *p,
>                 __setscheduler_params(p, attr);
>                 __setscheduler_prio(p, newprio);
>         }
> +
>         __setscheduler_uclamp(p, attr);
> +       __setscheduler_sched_qos(p, attr);
>

Hey Qais,
  Started tinkering a bit more with this patch series and found
unexpectedly a number of tasks were getting their rampup_multiplier
value set to zero.

It looks like the issue is that the SCHED_QOS_RAMPUP_MULTIPLIER enum
value is 0, so the switch (attr->sched_qos_type) always catches the
uninitialized/unset value during any sched_setscheduler()call, and
further the call to __setscheduler_sched_qos() isn't protected by a
(attr->sched_flags & SCHED_FLAG_QOS) check as is done for
sched_qos_validate() so we always end up falling into it and setting
the rampup_multiplier.

The easiest fix is probably just to have a SCHED_QOS_NONE base value
in the sched_qos_type enum, but we can also add checks on sched_flags
& SCHED_FLAG_QOS. Or do you have another idea?

thanks
-john