Re: [PATCH v1 11/14] sched: psi: refactor psi_trigger_create()

[Roman Gushchin <roman.gushchin@linux.dev>]

Suren Baghdasaryan <surenb@google.com> writes:

> On Mon, Aug 18, 2025 at 10:02 AM Roman Gushchin
> <roman.gushchin@linux.dev> wrote:
>>
>> Currently psi_trigger_create() does a lot of things:
>> parses the user text input, allocates and initializes
>> the psi_trigger structure and turns on the trigger.
>> It does it slightly different for two existing types
>> of psi_triggers: system-wide and cgroup-wide.
>>
>> In order to support a new type of psi triggers, which
>> will be owned by a bpf program and won't have a user's
>> text description, let's refactor psi_trigger_create().
>>
>> 1. Introduce psi_trigger_type enum:
>>    currently PSI_SYSTEM and PSI_CGROUP are valid values.
>> 2. Introduce psi_trigger_params structure to avoid passing
>>    a large number of parameters to psi_trigger_create().
>> 3. Move out the user's input parsing into the new
>>    psi_trigger_parse() helper.
>> 4. Move out the capabilities check into the new
>>    psi_file_privileged() helper.
>> 5. Stop relying on t->of for detecting trigger type.
>
> It's worth noting that this is a pure core refactoring without any
> functional change (hopefully :))

Added this to the commit log.

>
>>
>> Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
>> ---
>>  include/linux/psi.h       | 15 +++++--
>>  include/linux/psi_types.h | 33 ++++++++++++++-
>>  kernel/cgroup/cgroup.c    | 14 ++++++-
>>  kernel/sched/psi.c        | 87 +++++++++++++++++++++++++--------------
>>  4 files changed, 112 insertions(+), 37 deletions(-)
>>
>> diff --git a/include/linux/psi.h b/include/linux/psi.h
>> index e0745873e3f2..8178e998d94b 100644
>> --- a/include/linux/psi.h
>> +++ b/include/linux/psi.h
>> @@ -23,14 +23,23 @@ void psi_memstall_enter(unsigned long *flags);
>>  void psi_memstall_leave(unsigned long *flags);
>>
>>  int psi_show(struct seq_file *s, struct psi_group *group, enum psi_res res);
>> -struct psi_trigger *psi_trigger_create(struct psi_group *group, char *buf,
>> -                                      enum psi_res res, struct file *file,
>> -                                      struct kernfs_open_file *of);
>> +int psi_trigger_parse(struct psi_trigger_params *params, const char *buf);
>> +struct psi_trigger *psi_trigger_create(struct psi_group *group,
>> +                               const struct psi_trigger_params *param);
>>  void psi_trigger_destroy(struct psi_trigger *t);
>>
>>  __poll_t psi_trigger_poll(void **trigger_ptr, struct file *file,
>>                         poll_table *wait);
>>
>> +static inline bool psi_file_privileged(struct file *file)
>> +{
>> +       /*
>> +        * Checking the privilege here on file->f_cred implies that a privileged user
>> +        * could open the file and delegate the write to an unprivileged one.
>> +        */
>> +       return cap_raised(file->f_cred->cap_effective, CAP_SYS_RESOURCE);
>> +}
>> +
>>  #ifdef CONFIG_CGROUPS
>>  static inline struct psi_group *cgroup_psi(struct cgroup *cgrp)
>>  {
>> diff --git a/include/linux/psi_types.h b/include/linux/psi_types.h
>> index f1fd3a8044e0..cea54121d9b9 100644
>> --- a/include/linux/psi_types.h
>> +++ b/include/linux/psi_types.h
>> @@ -121,7 +121,38 @@ struct psi_window {
>>         u64 prev_growth;
>>  };
>>
>> +enum psi_trigger_type {
>> +       PSI_SYSTEM,
>> +       PSI_CGROUP,
>> +};
>> +
>> +struct psi_trigger_params {
>> +       /* Trigger type */
>> +       enum psi_trigger_type type;
>> +
>> +       /* Resources that workloads could be stalled on */
>
> I would describe this as "Resource to be monitored"

Fixed.

>
>> +       enum psi_res res;
>> +
>> +       /* True if all threads should be stalled to trigger */
>> +       bool full;
>> +
>> +       /* Threshold in us */
>> +       u32 threshold_us;
>> +
>> +       /* Window in us */
>> +       u32 window_us;
>> +
>> +       /* Privileged triggers are treated differently */
>> +       bool privileged;
>> +
>> +       /* Link to kernfs open file, only for PSI_CGROUP */
>> +       struct kernfs_open_file *of;
...
>>         t->event = 0;
>>         t->last_event_time = 0;
>> -       t->of = of;
>> -       if (!of)
>> +
>> +       switch (params->type) {
>> +       case PSI_SYSTEM:
>>                 init_waitqueue_head(&t->event_wait);
>
> I think t->of will be left uninitialized here. Let's set it to NULL
> please.

Ack.

Thanks!