Suren Baghdasaryan <surenb@xxxxxxxxxx> writes:
> On Mon, Aug 18, 2025 at 10:02 AM Roman Gushchin
> <roman.gushchin@xxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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!
