Android is adopting psi to detect and remedy memory pressure that
results in stuttering and decreased responsiveness on mobile devices.
Psi gives us the stall information, but because we're dealing with
latencies in the millisecond range, periodically reading the pressure
files to detect stalls in a timely fashion is not feasible. Psi also
doesn't aggregate its averages at a high-enough frequency right now.
This patch series extends the psi interface such that users can
configure sensitive latency thresholds and use poll() and friends to
be notified when these are breached.
As high-frequency aggregation is costly, it implements an aggregation
method that is optimized for fast, short-interval averaging, and makes
the aggregation frequency adaptive, such that high-frequency updates
only happen while monitored stall events are actively occurring.
With these patches applied, Android can monitor for, and ward off,
mounting memory shortages before they cause problems for the user.
For example, using memory stall monitors in userspace low memory
killer daemon (lmkd) we can detect mounting pressure and kill less
important processes before device becomes visibly sluggish. In our
memory stress testing psi memory monitors produce roughly 10x less
false positives compared to vmpressure signals. Having ability to
specify multiple triggers for the same psi metric allows other parts
of Android framework to monitor memory state of the device and act
accordingly.
The new interface is straight-forward. The user opens one of the
pressure files for writing and writes a trigger description into the
file descriptor that defines the stall state - some or full, and the
maximum stall time over a given window of time. E.g.:
/* Signal when stall time exceeds 100ms of a 1s window */
char trigger[] = "full 100000 1000000"
fd = open("/proc/pressure/memory")
write(fd, trigger, sizeof(trigger))
while (poll() >= 0) {
...
};
close(fd);
When the monitored stall state is entered, psi adapts its aggregation
frequency according to what the configured time window requires in
order to emit event signals in a timely fashion. Once the stalling
subsides, aggregation reverts back to normal.
The trigger is associated with the open file descriptor. To stop
monitoring, the user only needs to close the file descriptor and the
trigger is discarded.
Patches 1-5 prepare the psi code for polling support. Patch 6
implements the adaptive polling logic, the pressure growth detection
optimized for short intervals, and hooks up write() and poll() on the
pressure files.
The patches were developed in collaboration with Johannes Weiner.
The patches are based on 4.20-rc6.
Johannes Weiner (3):
fs: kernfs: add poll file operation
kernel: cgroup: add poll file operation
psi: eliminate lazy clock mode
Suren Baghdasaryan (3):
psi: introduce state_mask to represent stalled psi states
psi: rename psi fields in preparation for psi trigger addition
psi: introduce psi monitor
Documentation/accounting/psi.txt | 105 ++++++
fs/kernfs/file.c | 31 +-
include/linux/cgroup-defs.h | 4 +
include/linux/kernfs.h | 6 +
include/linux/psi.h | 10 +
include/linux/psi_types.h | 90 ++++-
kernel/cgroup/cgroup.c | 119 ++++++-
kernel/sched/psi.c | 586 +++++++++++++++++++++++++++----
8 files changed, 865 insertions(+), 86 deletions(-)
--
2.20.0.405.gbc1bbc6f85-goog
From: Johannes Weiner <[email protected]>
Kernfs has a standardized poll/notification mechanism for waking all
pollers on all fds when a filesystem node changes. To allow polling
for custom events, add a .poll callback that can override the default.
This is in preparation for pollable cgroup pressure files which have
per-fd trigger configurations.
Signed-off-by: Johannes Weiner <[email protected]>
Signed-off-by: Suren Baghdasaryan <[email protected]>
---
fs/kernfs/file.c | 31 ++++++++++++++++++++-----------
include/linux/kernfs.h | 6 ++++++
2 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/fs/kernfs/file.c b/fs/kernfs/file.c
index dbf5bc250bfd..2d8b91f4475d 100644
--- a/fs/kernfs/file.c
+++ b/fs/kernfs/file.c
@@ -832,26 +832,35 @@ void kernfs_drain_open_files(struct kernfs_node *kn)
* to see if it supports poll (Neither 'poll' nor 'select' return
* an appropriate error code). When in doubt, set a suitable timeout value.
*/
+__poll_t kernfs_generic_poll(struct kernfs_open_file *of, poll_table *wait)
+{
+ struct kernfs_node *kn = kernfs_dentry_node(of->file->f_path.dentry);
+ struct kernfs_open_node *on = kn->attr.open;
+
+ poll_wait(of->file, &on->poll, wait);
+
+ if (of->event != atomic_read(&on->event))
+ return DEFAULT_POLLMASK|EPOLLERR|EPOLLPRI;
+
+ return DEFAULT_POLLMASK;
+}
+
static __poll_t kernfs_fop_poll(struct file *filp, poll_table *wait)
{
struct kernfs_open_file *of = kernfs_of(filp);
struct kernfs_node *kn = kernfs_dentry_node(filp->f_path.dentry);
- struct kernfs_open_node *on = kn->attr.open;
+ __poll_t ret;
if (!kernfs_get_active(kn))
- goto trigger;
+ return DEFAULT_POLLMASK|EPOLLERR|EPOLLPRI;
- poll_wait(filp, &on->poll, wait);
+ if (kn->attr.ops->poll)
+ ret = kn->attr.ops->poll(of, wait);
+ else
+ ret = kernfs_generic_poll(of, wait);
kernfs_put_active(kn);
-
- if (of->event != atomic_read(&on->event))
- goto trigger;
-
- return DEFAULT_POLLMASK;
-
- trigger:
- return DEFAULT_POLLMASK|EPOLLERR|EPOLLPRI;
+ return ret;
}
static void kernfs_notify_workfn(struct work_struct *work)
diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
index 5b36b1287a5a..0cac1207bb00 100644
--- a/include/linux/kernfs.h
+++ b/include/linux/kernfs.h
@@ -25,6 +25,7 @@ struct seq_file;
struct vm_area_struct;
struct super_block;
struct file_system_type;
+struct poll_table_struct;
struct kernfs_open_node;
struct kernfs_iattrs;
@@ -261,6 +262,9 @@ struct kernfs_ops {
ssize_t (*write)(struct kernfs_open_file *of, char *buf, size_t bytes,
loff_t off);
+ __poll_t (*poll)(struct kernfs_open_file *of,
+ struct poll_table_struct *pt);
+
int (*mmap)(struct kernfs_open_file *of, struct vm_area_struct *vma);
#ifdef CONFIG_DEBUG_LOCK_ALLOC
@@ -350,6 +354,8 @@ int kernfs_remove_by_name_ns(struct kernfs_node *parent, const char *name,
int kernfs_rename_ns(struct kernfs_node *kn, struct kernfs_node *new_parent,
const char *new_name, const void *new_ns);
int kernfs_setattr(struct kernfs_node *kn, const struct iattr *iattr);
+__poll_t kernfs_generic_poll(struct kernfs_open_file *of,
+ struct poll_table_struct *pt);
void kernfs_notify(struct kernfs_node *kn);
const void *kernfs_super_ns(struct super_block *sb);
--
2.20.0.405.gbc1bbc6f85-goog
From: Johannes Weiner <[email protected]>
Cgroup has a standardized poll/notification mechanism for waking all
pollers on all fds when a filesystem node changes. To allow polling
for custom events, add a .poll callback that can override the default.
This is in preparation for pollable cgroup pressure files which have
per-fd trigger configurations.
Signed-off-by: Johannes Weiner <[email protected]>
Signed-off-by: Suren Baghdasaryan <[email protected]>
---
include/linux/cgroup-defs.h | 4 ++++
kernel/cgroup/cgroup.c | 12 ++++++++++++
2 files changed, 16 insertions(+)
diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
index 5e1694fe035b..6f9ea8601421 100644
--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -32,6 +32,7 @@ struct kernfs_node;
struct kernfs_ops;
struct kernfs_open_file;
struct seq_file;
+struct poll_table_struct;
#define MAX_CGROUP_TYPE_NAMELEN 32
#define MAX_CGROUP_ROOT_NAMELEN 64
@@ -573,6 +574,9 @@ struct cftype {
ssize_t (*write)(struct kernfs_open_file *of,
char *buf, size_t nbytes, loff_t off);
+ __poll_t (*poll)(struct kernfs_open_file *of,
+ struct poll_table_struct *pt);
+
#ifdef CONFIG_DEBUG_LOCK_ALLOC
struct lock_class_key lockdep_key;
#endif
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 6aaf5dd5383b..ffcd7483b8ee 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -3499,6 +3499,16 @@ static ssize_t cgroup_file_write(struct kernfs_open_file *of, char *buf,
return ret ?: nbytes;
}
+static __poll_t cgroup_file_poll(struct kernfs_open_file *of, poll_table *pt)
+{
+ struct cftype *cft = of->kn->priv;
+
+ if (cft->poll)
+ return cft->poll(of, pt);
+
+ return kernfs_generic_poll(of, pt);
+}
+
static void *cgroup_seqfile_start(struct seq_file *seq, loff_t *ppos)
{
return seq_cft(seq)->seq_start(seq, ppos);
@@ -3537,6 +3547,7 @@ static struct kernfs_ops cgroup_kf_single_ops = {
.open = cgroup_file_open,
.release = cgroup_file_release,
.write = cgroup_file_write,
+ .poll = cgroup_file_poll,
.seq_show = cgroup_seqfile_show,
};
@@ -3545,6 +3556,7 @@ static struct kernfs_ops cgroup_kf_ops = {
.open = cgroup_file_open,
.release = cgroup_file_release,
.write = cgroup_file_write,
+ .poll = cgroup_file_poll,
.seq_start = cgroup_seqfile_start,
.seq_next = cgroup_seqfile_next,
.seq_stop = cgroup_seqfile_stop,
--
2.20.0.405.gbc1bbc6f85-goog
The psi monitoring patches will need to determine the same states as
record_times(). To avoid calculating them twice, maintain a state mask
that can be consulted cheaply. Do this in a separate patch to keep the
churn in the main feature patch at a minimum.
Signed-off-by: Suren Baghdasaryan <[email protected]>
---
include/linux/psi_types.h | 3 +++
kernel/sched/psi.c | 29 +++++++++++++++++++----------
2 files changed, 22 insertions(+), 10 deletions(-)
diff --git a/include/linux/psi_types.h b/include/linux/psi_types.h
index 2cf422db5d18..2c6e9b67b7eb 100644
--- a/include/linux/psi_types.h
+++ b/include/linux/psi_types.h
@@ -53,6 +53,9 @@ struct psi_group_cpu {
/* States of the tasks belonging to this group */
unsigned int tasks[NR_PSI_TASK_COUNTS];
+ /* Aggregate pressure state derived from the tasks */
+ u32 state_mask;
+
/* Period time sampling buckets for each state of interest (ns) */
u32 times[NR_PSI_STATES];
diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index d2b9c9a1a62f..153c0624976b 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -212,17 +212,17 @@ static bool test_state(unsigned int *tasks, enum psi_states state)
static void get_recent_times(struct psi_group *group, int cpu, u32 *times)
{
struct psi_group_cpu *groupc = per_cpu_ptr(group->pcpu, cpu);
- unsigned int tasks[NR_PSI_TASK_COUNTS];
u64 now, state_start;
+ enum psi_states s;
unsigned int seq;
- int s;
+ u32 state_mask;
/* Snapshot a coherent view of the CPU state */
do {
seq = read_seqcount_begin(&groupc->seq);
now = cpu_clock(cpu);
memcpy(times, groupc->times, sizeof(groupc->times));
- memcpy(tasks, groupc->tasks, sizeof(groupc->tasks));
+ state_mask = groupc->state_mask;
state_start = groupc->state_start;
} while (read_seqcount_retry(&groupc->seq, seq));
@@ -238,7 +238,7 @@ static void get_recent_times(struct psi_group *group, int cpu, u32 *times)
* (u32) and our reported pressure close to what's
* actually happening.
*/
- if (test_state(tasks, s))
+ if (state_mask & (1 << s))
times[s] += now - state_start;
delta = times[s] - groupc->times_prev[s];
@@ -390,15 +390,15 @@ static void record_times(struct psi_group_cpu *groupc, int cpu,
delta = now - groupc->state_start;
groupc->state_start = now;
- if (test_state(groupc->tasks, PSI_IO_SOME)) {
+ if (groupc->state_mask & (1 << PSI_IO_SOME)) {
groupc->times[PSI_IO_SOME] += delta;
- if (test_state(groupc->tasks, PSI_IO_FULL))
+ if (groupc->state_mask & (1 << PSI_IO_FULL))
groupc->times[PSI_IO_FULL] += delta;
}
- if (test_state(groupc->tasks, PSI_MEM_SOME)) {
+ if (groupc->state_mask & (1 << PSI_MEM_SOME)) {
groupc->times[PSI_MEM_SOME] += delta;
- if (test_state(groupc->tasks, PSI_MEM_FULL))
+ if (groupc->state_mask & (1 << PSI_MEM_FULL))
groupc->times[PSI_MEM_FULL] += delta;
else if (memstall_tick) {
u32 sample;
@@ -419,10 +419,10 @@ static void record_times(struct psi_group_cpu *groupc, int cpu,
}
}
- if (test_state(groupc->tasks, PSI_CPU_SOME))
+ if (groupc->state_mask & (1 << PSI_CPU_SOME))
groupc->times[PSI_CPU_SOME] += delta;
- if (test_state(groupc->tasks, PSI_NONIDLE))
+ if (groupc->state_mask & (1 << PSI_NONIDLE))
groupc->times[PSI_NONIDLE] += delta;
}
@@ -431,6 +431,8 @@ static void psi_group_change(struct psi_group *group, int cpu,
{
struct psi_group_cpu *groupc;
unsigned int t, m;
+ enum psi_states s;
+ u32 state_mask = 0;
groupc = per_cpu_ptr(group->pcpu, cpu);
@@ -463,6 +465,13 @@ static void psi_group_change(struct psi_group *group, int cpu,
if (set & (1 << t))
groupc->tasks[t]++;
+ /* Calculate state mask representing active states */
+ for (s = 0; s < NR_PSI_STATES; s++) {
+ if (test_state(groupc->tasks, s))
+ state_mask |= (1 << s);
+ }
+ groupc->state_mask = state_mask;
+
write_seqcount_end(&groupc->seq);
}
--
2.20.0.405.gbc1bbc6f85-goog
From: Johannes Weiner <[email protected]>
psi currently stops its periodic 2s aggregation runs when there has
not been any task activity, and wakes it back up later from the
scheduler when the system returns from the idle state.
The coordination between the aggregation worker and the scheduler is
minimal: the scheduler has to nudge the worker if it's not running,
and the worker will reschedule itself periodically until it detects no
more activity.
The polling patches will complicate this, because they introduce
another aggregation mode for high-frequency polling that also
eventually times out if the worker sees no more activity of interest.
That means the scheduler portion would have to coordinate three state
transitions - idle to regular, regular to polling, idle to polling -
with the worker's timeouts and self-rescheduling. The additional
overhead from this is undesirable in the scheduler hotpath.
Eliminate the idle mode and keep the worker doing 2s update intervals
at all times. This eliminates worker coordination from the scheduler
completely. The polling patches will then add it back to switch
between regular mode and high-frequency polling mode.
Signed-off-by: Johannes Weiner <[email protected]>
Signed-off-by: Suren Baghdasaryan <[email protected]>
---
kernel/sched/psi.c | 55 +++++++++++++++++++---------------------------
1 file changed, 22 insertions(+), 33 deletions(-)
diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index fe24de3fbc93..d2b9c9a1a62f 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -248,18 +248,10 @@ static void get_recent_times(struct psi_group *group, int cpu, u32 *times)
}
}
-static void calc_avgs(unsigned long avg[3], int missed_periods,
- u64 time, u64 period)
+static void calc_avgs(unsigned long avg[3], u64 time, u64 period)
{
unsigned long pct;
- /* Fill in zeroes for periods of no activity */
- if (missed_periods) {
- avg[0] = calc_load_n(avg[0], EXP_10s, 0, missed_periods);
- avg[1] = calc_load_n(avg[1], EXP_60s, 0, missed_periods);
- avg[2] = calc_load_n(avg[2], EXP_300s, 0, missed_periods);
- }
-
/* Sample the most recent active period */
pct = div_u64(time * 100, period);
pct *= FIXED_1;
@@ -268,10 +260,9 @@ static void calc_avgs(unsigned long avg[3], int missed_periods,
avg[2] = calc_load(avg[2], EXP_300s, pct);
}
-static bool update_stats(struct psi_group *group)
+static void update_stats(struct psi_group *group)
{
u64 deltas[NR_PSI_STATES - 1] = { 0, };
- unsigned long missed_periods = 0;
unsigned long nonidle_total = 0;
u64 now, expires, period;
int cpu;
@@ -321,8 +312,6 @@ static bool update_stats(struct psi_group *group)
expires = group->next_update;
if (now < expires)
goto out;
- if (now - expires > psi_period)
- missed_periods = div_u64(now - expires, psi_period);
/*
* The periodic clock tick can get delayed for various
@@ -331,8 +320,8 @@ static bool update_stats(struct psi_group *group)
* But the deltas we sample out of the per-cpu buckets above
* are based on the actual time elapsing between clock ticks.
*/
- group->next_update = expires + ((1 + missed_periods) * psi_period);
- period = now - (group->last_update + (missed_periods * psi_period));
+ group->next_update = expires + psi_period;
+ period = now - group->last_update;
group->last_update = now;
for (s = 0; s < NR_PSI_STATES - 1; s++) {
@@ -359,18 +348,18 @@ static bool update_stats(struct psi_group *group)
if (sample > period)
sample = period;
group->total_prev[s] += sample;
- calc_avgs(group->avg[s], missed_periods, sample, period);
+ calc_avgs(group->avg[s], sample, period);
}
out:
mutex_unlock(&group->stat_lock);
- return nonidle_total;
}
static void psi_update_work(struct work_struct *work)
{
struct delayed_work *dwork;
struct psi_group *group;
- bool nonidle;
+ unsigned long delay = 0;
+ u64 now;
dwork = to_delayed_work(work);
group = container_of(dwork, struct psi_group, clock_work);
@@ -383,17 +372,12 @@ static void psi_update_work(struct work_struct *work)
* go - see calc_avgs() and missed_periods.
*/
- nonidle = update_stats(group);
-
- if (nonidle) {
- unsigned long delay = 0;
- u64 now;
+ update_stats(group);
- now = sched_clock();
- if (group->next_update > now)
- delay = nsecs_to_jiffies(group->next_update - now) + 1;
- schedule_delayed_work(dwork, delay);
- }
+ now = sched_clock();
+ if (group->next_update > now)
+ delay = nsecs_to_jiffies(group->next_update - now) + 1;
+ schedule_delayed_work(dwork, delay);
}
static void record_times(struct psi_group_cpu *groupc, int cpu,
@@ -480,9 +464,6 @@ static void psi_group_change(struct psi_group *group, int cpu,
groupc->tasks[t]++;
write_seqcount_end(&groupc->seq);
-
- if (!delayed_work_pending(&group->clock_work))
- schedule_delayed_work(&group->clock_work, PSI_FREQ);
}
static struct psi_group *iterate_groups(struct task_struct *task, void **iter)
@@ -619,6 +600,8 @@ int psi_cgroup_alloc(struct cgroup *cgroup)
if (!cgroup->psi.pcpu)
return -ENOMEM;
group_init(&cgroup->psi);
+ schedule_delayed_work(&cgroup->psi.clock_work, PSI_FREQ);
+
return 0;
}
@@ -761,12 +744,18 @@ static const struct file_operations psi_cpu_fops = {
.release = single_release,
};
-static int __init psi_proc_init(void)
+static int __init psi_late_init(void)
{
+ if (static_branch_likely(&psi_disabled))
+ return 0;
+
+ schedule_delayed_work(&psi_system.clock_work, PSI_FREQ);
+
proc_mkdir("pressure", NULL);
proc_create("pressure/io", 0, NULL, &psi_io_fops);
proc_create("pressure/memory", 0, NULL, &psi_memory_fops);
proc_create("pressure/cpu", 0, NULL, &psi_cpu_fops);
+
return 0;
}
-module_init(psi_proc_init);
+module_init(psi_late_init);
--
2.20.0.405.gbc1bbc6f85-goog
Renaming psi_group structure member fields used for calculating psi
totals and averages for clear distinction between them and trigger-related
fields that will be added next.
Signed-off-by: Suren Baghdasaryan <[email protected]>
---
include/linux/psi_types.h | 15 ++++++++-------
kernel/sched/psi.c | 26 ++++++++++++++------------
2 files changed, 22 insertions(+), 19 deletions(-)
diff --git a/include/linux/psi_types.h b/include/linux/psi_types.h
index 2c6e9b67b7eb..11b32b3395a2 100644
--- a/include/linux/psi_types.h
+++ b/include/linux/psi_types.h
@@ -69,20 +69,21 @@ struct psi_group_cpu {
};
struct psi_group {
- /* Protects data updated during an aggregation */
- struct mutex stat_lock;
+ /* Protects data used by the aggregator */
+ struct mutex update_lock;
/* Per-cpu task state & time tracking */
struct psi_group_cpu __percpu *pcpu;
- /* Periodic aggregation state */
- u64 total_prev[NR_PSI_STATES - 1];
- u64 last_update;
- u64 next_update;
struct delayed_work clock_work;
- /* Total stall times and sampled pressure averages */
+ /* Total stall times observed */
u64 total[NR_PSI_STATES - 1];
+
+ /* Running pressure averages */
+ u64 avg_total[NR_PSI_STATES - 1];
+ u64 avg_last_update;
+ u64 avg_next_update;
unsigned long avg[NR_PSI_STATES - 1][3];
};
diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 153c0624976b..694edefdd333 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -172,9 +172,9 @@ static void group_init(struct psi_group *group)
for_each_possible_cpu(cpu)
seqcount_init(&per_cpu_ptr(group->pcpu, cpu)->seq);
- group->next_update = sched_clock() + psi_period;
+ group->avg_next_update = sched_clock() + psi_period;
INIT_DELAYED_WORK(&group->clock_work, psi_update_work);
- mutex_init(&group->stat_lock);
+ mutex_init(&group->update_lock);
}
void __init psi_init(void)
@@ -268,7 +268,7 @@ static void update_stats(struct psi_group *group)
int cpu;
int s;
- mutex_lock(&group->stat_lock);
+ mutex_lock(&group->update_lock);
/*
* Collect the per-cpu time buckets and average them into a
@@ -309,7 +309,7 @@ static void update_stats(struct psi_group *group)
/* avgX= */
now = sched_clock();
- expires = group->next_update;
+ expires = group->avg_next_update;
if (now < expires)
goto out;
@@ -320,14 +320,14 @@ static void update_stats(struct psi_group *group)
* But the deltas we sample out of the per-cpu buckets above
* are based on the actual time elapsing between clock ticks.
*/
- group->next_update = expires + psi_period;
- period = now - group->last_update;
- group->last_update = now;
+ group->avg_next_update = expires + psi_period;
+ period = now - group->avg_last_update;
+ group->avg_last_update = now;
for (s = 0; s < NR_PSI_STATES - 1; s++) {
u32 sample;
- sample = group->total[s] - group->total_prev[s];
+ sample = group->total[s] - group->avg_total[s];
/*
* Due to the lockless sampling of the time buckets,
* recorded time deltas can slip into the next period,
@@ -347,11 +347,11 @@ static void update_stats(struct psi_group *group)
*/
if (sample > period)
sample = period;
- group->total_prev[s] += sample;
+ group->avg_total[s] += sample;
calc_avgs(group->avg[s], sample, period);
}
out:
- mutex_unlock(&group->stat_lock);
+ mutex_unlock(&group->update_lock);
}
static void psi_update_work(struct work_struct *work)
@@ -375,8 +375,10 @@ static void psi_update_work(struct work_struct *work)
update_stats(group);
now = sched_clock();
- if (group->next_update > now)
- delay = nsecs_to_jiffies(group->next_update - now) + 1;
+ if (group->avg_next_update > now) {
+ delay = nsecs_to_jiffies(
+ group->avg_next_update - now) + 1;
+ }
schedule_delayed_work(dwork, delay);
}
--
2.20.0.405.gbc1bbc6f85-goog
Psi monitor aims to provide a low-latency short-term pressure
detection mechanism configurable by users. It allows users to
monitor psi metrics growth and trigger events whenever a metric
raises above user-defined threshold within user-defined time window.
Time window is expressed in usecs and threshold can be expressed in
usecs or percentages of the tracking window. Multiple psi resources
with different thresholds and window sizes can be monitored concurrently.
Psi monitors activate when system enters stall state for the monitored
psi metric and deactivate upon exit from the stall state. While system
is in the stall state psi signal growth is monitored at a rate of 10 times
per tracking window. Min window size is 500ms, therefore the min monitoring
interval is 50ms. Max window size is 10s with monitoring interval of 1s.
When activated psi monitor stays active for at least the duration of one
tracking window to avoid repeated activations/deactivations when psi
signal is bouncing.
Notifications to the users are rate-limited to one per tracking window.
Signed-off-by: Suren Baghdasaryan <[email protected]>
---
Documentation/accounting/psi.txt | 105 +++++++
include/linux/psi.h | 10 +
include/linux/psi_types.h | 72 +++++
kernel/cgroup/cgroup.c | 107 ++++++-
kernel/sched/psi.c | 510 +++++++++++++++++++++++++++++--
5 files changed, 774 insertions(+), 30 deletions(-)
diff --git a/Documentation/accounting/psi.txt b/Documentation/accounting/psi.txt
index b8ca28b60215..b006cc84ad44 100644
--- a/Documentation/accounting/psi.txt
+++ b/Documentation/accounting/psi.txt
@@ -63,6 +63,108 @@ tracked and exported as well, to allow detection of latency spikes
which wouldn't necessarily make a dent in the time averages, or to
average trends over custom time frames.
+Monitoring for pressure thresholds
+==================================
+
+Users can register triggers and use poll() to be woken up when resource
+pressure exceeds certain thresholds.
+
+A trigger describes the maximum cumulative stall time over a specific
+time window, e.g. 100ms of total stall time within any 500ms window to
+generate a wakeup event.
+
+To register a trigger user has to open psi interface file under
+/proc/pressure/ representing the resource to be monitored and write the
+desired threshold and time window. The open file descriptor should be
+used to wait for trigger events using select(), poll() or epoll().
+The following format is used:
+
+<some|full> <stall %|stall amount in us> <time window in us>
+
+For example writing "some 15% 1000000" or "some 150000 1000000" into
+/proc/pressure/memory would add 15% (150ms) threshold for partial memory
+stall measured within 1sec time window. Writing "full 5% 1000000" or
+"full 50000 1000000" into /proc/pressure/io would add 5% (50ms) threshold
+for full io stall measured within 1sec time window.
+
+Triggers can be set on more than one psi metric and more than one trigger
+for the same psi metric can be specified. However for each trigger a separate
+file descriptor is required to be able to poll it separately from others,
+therefore for each trigger a separate open() syscall should be made even
+when opening the same psi interface file.
+
+Monitors activate only when system enters stall state for the monitored
+psi metric and deactivates upon exit from the stall state. While system is
+in the stall state psi signal growth is monitored at a rate of 10 times per
+tracking window.
+
+The kernel accepts window sizes ranging from 500ms to 10s, therefore min
+monitoring update interval is 50ms and max is 1s.
+
+When activated, psi monitor stays active for at least the duration of one
+tracking window to avoid repeated activations/deactivations when system is
+bouncing in and out of the stall state.
+
+Notifications to the userspace are rate-limited to one per tracking window.
+
+The trigger will de-register when the file descriptor used to define the
+trigger is closed.
+
+Userspace monitor usage example
+===============================
+
+#include <errno.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <poll.h>
+#include <string.h>
+#include <unistd.h>
+
+/*
+ * Monitor memory partial stall with 1s tracking window size
+ * and 15% (150ms) threshold.
+ */
+int main() {
+ const char trig[] = "some 15% 1000000";
+ struct pollfd fds;
+ int n;
+
+ fds.fd = open("/proc/pressure/memory", O_RDWR | O_NONBLOCK);
+ if (fds.fd < 0) {
+ printf("/proc/pressure/memory open error: %s\n",
+ strerror(errno));
+ return 1;
+ }
+ fds.events = POLLPRI;
+
+ if (write(fds.fd, trig, strlen(trig) + 1) < 0) {
+ printf("/proc/pressure/memory write error: %s\n",
+ strerror(errno));
+ return 1;
+ }
+
+ printf("waiting for events...\n");
+ while (1) {
+ n = poll(&fds, 1, -1);
+ if (n < 0) {
+ printf("poll error: %s\n", strerror(errno));
+ return 1;
+ }
+ if (fds.revents & POLLERR) {
+ printf("got POLLERR, event source is gone\n");
+ return 0;
+ }
+ if (fds.revents & POLLPRI) {
+ printf("event triggered!\n");
+ } else {
+ printf("unknown event received: 0x%x\n", fds.revents);
+ return 1;
+ }
+ }
+
+ return 0;
+}
+
Cgroup2 interface
=================
@@ -71,3 +173,6 @@ mounted, pressure stall information is also tracked for tasks grouped
into cgroups. Each subdirectory in the cgroupfs mountpoint contains
cpu.pressure, memory.pressure, and io.pressure files; the format is
the same as the /proc/pressure/ files.
+
+Per-cgroup psi monitors can be specified and used the same way as
+system-wide ones.
diff --git a/include/linux/psi.h b/include/linux/psi.h
index 7006008d5b72..7490f8ef83ac 100644
--- a/include/linux/psi.h
+++ b/include/linux/psi.h
@@ -4,6 +4,7 @@
#include <linux/jump_label.h>
#include <linux/psi_types.h>
#include <linux/sched.h>
+#include <linux/poll.h>
struct seq_file;
struct css_set;
@@ -26,6 +27,15 @@ int psi_show(struct seq_file *s, struct psi_group *group, enum psi_res res);
int psi_cgroup_alloc(struct cgroup *cgrp);
void psi_cgroup_free(struct cgroup *cgrp);
void cgroup_move_task(struct task_struct *p, struct css_set *to);
+
+ssize_t psi_trigger_parse(char *buf, size_t nbytes, enum psi_res res,
+ enum psi_states *state, u32 *threshold_us, u32 *win_sz_us);
+struct psi_trigger *psi_trigger_create(struct psi_group *group,
+ enum psi_states state, u32 threshold_us, u32 win_sz_us);
+void psi_trigger_destroy(struct psi_trigger *t);
+
+__poll_t psi_trigger_poll(struct psi_trigger *t, struct file *file,
+ poll_table *wait);
#endif
#else /* CONFIG_PSI */
diff --git a/include/linux/psi_types.h b/include/linux/psi_types.h
index 11b32b3395a2..597c3c07d999 100644
--- a/include/linux/psi_types.h
+++ b/include/linux/psi_types.h
@@ -3,6 +3,7 @@
#include <linux/seqlock.h>
#include <linux/types.h>
+#include <linux/wait.h>
#ifdef CONFIG_PSI
@@ -68,6 +69,63 @@ struct psi_group_cpu {
u32 times_prev[NR_PSI_STATES] ____cacheline_aligned_in_smp;
};
+/*
+ * Aggregation clock mode
+ *
+ * REGULAR: Low-interval updates to flush the per-cpu time buckets
+ * SWITCHING: Transitioning from REGULAR to POLLING mode
+ * POLLING: High-frequency polling based on configured growth triggers
+ */
+enum psi_clock_mode {
+ PSI_CLOCK_REGULAR,
+ PSI_CLOCK_SWITCHING,
+ PSI_CLOCK_POLLING,
+};
+
+/* PSI growth tracking window */
+struct psi_window {
+ /* Window size in ns */
+ u64 size;
+
+ /* Start time of the current window in ns */
+ u64 start_time;
+
+ /* Value at the start of the window */
+ u64 start_value;
+
+ /* Value growth per previous window(s) */
+ u64 per_win_growth;
+};
+
+struct psi_trigger {
+ /* PSI state being monitored by the trigger */
+ enum psi_states state;
+
+ /* User-spacified threshold in ns */
+ u64 threshold;
+
+ /* List node inside triggers list */
+ struct list_head node;
+
+ /* Backpointer needed during trigger destruction */
+ struct psi_group *group;
+
+ /* Wait queue for polling */
+ wait_queue_head_t event_wait;
+
+ /* Pending event flag */
+ int event;
+
+ /* Tracking window */
+ struct psi_window win;
+
+ /*
+ * Time last event was generated. Used for rate-limiting
+ * events to one per window
+ */
+ u64 last_event_time;
+};
+
struct psi_group {
/* Protects data used by the aggregator */
struct mutex update_lock;
@@ -75,6 +133,8 @@ struct psi_group {
/* Per-cpu task state & time tracking */
struct psi_group_cpu __percpu *pcpu;
+ /* Periodic aggregation control */
+ enum psi_clock_mode clock_mode;
struct delayed_work clock_work;
/* Total stall times observed */
@@ -85,6 +145,18 @@ struct psi_group {
u64 avg_last_update;
u64 avg_next_update;
unsigned long avg[NR_PSI_STATES - 1][3];
+
+ /* Configured polling triggers */
+ struct list_head triggers;
+ u32 nr_triggers[NR_PSI_STATES - 1];
+ u32 trigger_mask;
+ u64 trigger_min_period;
+
+ /* Polling state */
+ /* Total stall times at the start of monitor activation */
+ u64 polling_total[NR_PSI_STATES - 1];
+ u64 polling_next_update;
+ u64 polling_until;
};
#else /* CONFIG_PSI */
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index ffcd7483b8ee..c0d7fe0ca02e 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -3430,7 +3430,101 @@ static int cgroup_cpu_pressure_show(struct seq_file *seq, void *v)
{
return psi_show(seq, &seq_css(seq)->cgroup->psi, PSI_CPU);
}
-#endif
+
+static ssize_t cgroup_pressure_write(struct kernfs_open_file *of, char *buf,
+ size_t nbytes, enum psi_res res)
+{
+ enum psi_states state;
+ struct psi_trigger *old;
+ struct psi_trigger *new;
+ struct cgroup *cgrp;
+ u32 threshold_us;
+ u32 win_sz_us;
+ ssize_t ret;
+
+ cgrp = cgroup_kn_lock_live(of->kn, false);
+ if (!cgrp)
+ return -ENODEV;
+
+ cgroup_get(cgrp);
+ cgroup_kn_unlock(of->kn);
+
+ ret = psi_trigger_parse(buf, nbytes, res,
+ &state, &threshold_us, &win_sz_us);
+ if (ret) {
+ cgroup_put(cgrp);
+ return ret;
+ }
+
+ new = psi_trigger_create(&cgrp->psi,
+ state, threshold_us, win_sz_us);
+ if (IS_ERR(new)) {
+ cgroup_put(cgrp);
+ return PTR_ERR(new);
+ }
+
+ old = of->priv;
+ rcu_assign_pointer(of->priv, new);
+ if (old) {
+ synchronize_rcu();
+ psi_trigger_destroy(old);
+ }
+
+ cgroup_put(cgrp);
+
+ return nbytes;
+}
+
+static ssize_t cgroup_io_pressure_write(struct kernfs_open_file *of,
+ char *buf, size_t nbytes,
+ loff_t off)
+{
+ return cgroup_pressure_write(of, buf, nbytes, PSI_IO);
+}
+
+static ssize_t cgroup_memory_pressure_write(struct kernfs_open_file *of,
+ char *buf, size_t nbytes,
+ loff_t off)
+{
+ return cgroup_pressure_write(of, buf, nbytes, PSI_MEM);
+}
+
+static ssize_t cgroup_cpu_pressure_write(struct kernfs_open_file *of,
+ char *buf, size_t nbytes,
+ loff_t off)
+{
+ return cgroup_pressure_write(of, buf, nbytes, PSI_CPU);
+}
+
+static __poll_t cgroup_pressure_poll(struct kernfs_open_file *of,
+ poll_table *pt)
+{
+ struct psi_trigger *t;
+ __poll_t ret;
+
+ rcu_read_lock();
+ t = rcu_dereference(of->priv);
+ if (t)
+ ret = psi_trigger_poll(t, of->file, pt);
+ else
+ ret = DEFAULT_POLLMASK | EPOLLERR | EPOLLPRI;
+ rcu_read_unlock();
+
+ return ret;
+}
+
+static void cgroup_pressure_release(struct kernfs_open_file *of)
+{
+ struct psi_trigger *t = of->priv;
+
+ if (!t)
+ return;
+
+ rcu_assign_pointer(of->priv, NULL);
+ synchronize_rcu();
+ psi_trigger_destroy(t);
+}
+#endif /* CONFIG_PSI */
static int cgroup_file_open(struct kernfs_open_file *of)
{
@@ -4579,18 +4673,27 @@ static struct cftype cgroup_base_files[] = {
.name = "io.pressure",
.flags = CFTYPE_NOT_ON_ROOT,
.seq_show = cgroup_io_pressure_show,
+ .write = cgroup_io_pressure_write,
+ .poll = cgroup_pressure_poll,
+ .release = cgroup_pressure_release,
},
{
.name = "memory.pressure",
.flags = CFTYPE_NOT_ON_ROOT,
.seq_show = cgroup_memory_pressure_show,
+ .write = cgroup_memory_pressure_write,
+ .poll = cgroup_pressure_poll,
+ .release = cgroup_pressure_release,
},
{
.name = "cpu.pressure",
.flags = CFTYPE_NOT_ON_ROOT,
.seq_show = cgroup_cpu_pressure_show,
+ .write = cgroup_cpu_pressure_write,
+ .poll = cgroup_pressure_poll,
+ .release = cgroup_pressure_release,
},
-#endif
+#endif /* CONFIG_PSI */
{ } /* terminate */
};
diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 694edefdd333..4f0e2de5eded 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -4,6 +4,9 @@
* Copyright (c) 2018 Facebook, Inc.
* Author: Johannes Weiner <[email protected]>
*
+ * Polling support by Suren Baghdasaryan <[email protected]>
+ * Copyright (c) 2018 Google, Inc.
+ *
* When CPU, memory and IO are contended, tasks experience delays that
* reduce throughput and introduce latencies into the workload. Memory
* and IO contention, in addition, can cause a full loss of forward
@@ -126,11 +129,16 @@
#include <linux/sched/loadavg.h>
#include <linux/seq_file.h>
+#include <linux/eventfd.h>
#include <linux/proc_fs.h>
#include <linux/seqlock.h>
+#include <linux/uaccess.h>
#include <linux/cgroup.h>
#include <linux/module.h>
#include <linux/sched.h>
+#include <linux/ctype.h>
+#include <linux/file.h>
+#include <linux/poll.h>
#include <linux/psi.h>
#include "sched.h"
@@ -150,11 +158,16 @@ static int __init setup_psi(char *str)
__setup("psi=", setup_psi);
/* Running averages - we need to be higher-res than loadavg */
-#define PSI_FREQ (2*HZ+1) /* 2 sec intervals */
+#define PSI_FREQ (2*HZ+1UL) /* 2 sec intervals */
#define EXP_10s 1677 /* 1/exp(2s/10s) as fixed-point */
#define EXP_60s 1981 /* 1/exp(2s/60s) */
#define EXP_300s 2034 /* 1/exp(2s/300s) */
+/* PSI trigger definitions */
+#define PSI_TRIG_MIN_WIN_US 500000 /* Min window size is 500ms */
+#define PSI_TRIG_MAX_WIN_US 10000000 /* Max window size is 10s */
+#define PSI_TRIG_UPDATES_PER_WIN 10 /* 10 updates per window */
+
/* Sampling frequency in nanoseconds */
static u64 psi_period __read_mostly;
@@ -173,8 +186,17 @@ static void group_init(struct psi_group *group)
for_each_possible_cpu(cpu)
seqcount_init(&per_cpu_ptr(group->pcpu, cpu)->seq);
group->avg_next_update = sched_clock() + psi_period;
+ group->clock_mode = PSI_CLOCK_REGULAR;
INIT_DELAYED_WORK(&group->clock_work, psi_update_work);
mutex_init(&group->update_lock);
+ /* Init trigger-related members */
+ INIT_LIST_HEAD(&group->triggers);
+ memset(group->nr_triggers, 0, sizeof(group->nr_triggers));
+ group->trigger_mask = 0;
+ group->trigger_min_period = U32_MAX;
+ memset(group->polling_total, 0, sizeof(group->polling_total));
+ group->polling_next_update = ULLONG_MAX;
+ group->polling_until = 0;
}
void __init psi_init(void)
@@ -260,16 +282,13 @@ static void calc_avgs(unsigned long avg[3], u64 time, u64 period)
avg[2] = calc_load(avg[2], EXP_300s, pct);
}
-static void update_stats(struct psi_group *group)
+static void collect_percpu_times(struct psi_group *group)
{
u64 deltas[NR_PSI_STATES - 1] = { 0, };
unsigned long nonidle_total = 0;
- u64 now, expires, period;
int cpu;
int s;
- mutex_lock(&group->update_lock);
-
/*
* Collect the per-cpu time buckets and average them into a
* single time sample that is normalized to wallclock time.
@@ -306,12 +325,16 @@ static void update_stats(struct psi_group *group)
/* total= */
for (s = 0; s < NR_PSI_STATES - 1; s++)
group->total[s] += div_u64(deltas[s], max(nonidle_total, 1UL));
+}
+
+static u64 calculate_averages(struct psi_group *group, u64 now)
+{
+ u64 expires, period;
+ u64 avg_next_update;
+ int s;
/* avgX= */
- now = sched_clock();
expires = group->avg_next_update;
- if (now < expires)
- goto out;
/*
* The periodic clock tick can get delayed for various
@@ -320,7 +343,7 @@ static void update_stats(struct psi_group *group)
* But the deltas we sample out of the per-cpu buckets above
* are based on the actual time elapsing between clock ticks.
*/
- group->avg_next_update = expires + psi_period;
+ avg_next_update = expires + psi_period;
period = now - group->avg_last_update;
group->avg_last_update = now;
@@ -350,7 +373,152 @@ static void update_stats(struct psi_group *group)
group->avg_total[s] += sample;
calc_avgs(group->avg[s], sample, period);
}
-out:
+
+ return avg_next_update;
+}
+
+/* Trigger tracking window manupulations */
+static void window_init(struct psi_window *win, u64 now, u64 value)
+{
+ win->start_value = value;
+ win->start_time = now;
+ win->per_win_growth = 0;
+}
+
+/*
+ * PSI growth tracking window update and growth calculation routine.
+ * This approximates a sliding tracking window by interpolating
+ * partially elapsed windows using historical growth data from the
+ * previous intervals. This minimizes memory requirements (by not storing
+ * all the intermediate values in the previous window) and simplifies
+ * the calculations. It works well because PSI signal changes only in
+ * positive direction and over relatively small window sizes the growth
+ * is close to linear.
+ */
+static u64 window_update(struct psi_window *win, u64 now, u64 value)
+{
+ u64 interval;
+ u64 growth;
+
+ interval = now - win->start_time;
+ growth = value - win->start_value;
+ /*
+ * After each tracking window passes win->start_value and
+ * win->start_time get reset and win->per_win_growth stores
+ * the average per-window growth of the previous window.
+ * win->per_win_growth is then used to interpolate additional
+ * growth from the previous window assuming it was linear.
+ */
+ if (interval > win->size) {
+ win->per_win_growth = growth;
+ win->start_value = value;
+ win->start_time = now;
+ } else {
+ u32 unelapsed;
+
+ unelapsed = win->size - interval;
+ growth += div_u64(win->per_win_growth * unelapsed, win->size);
+ }
+
+ return growth;
+}
+
+static u64 poll_triggers(struct psi_group *group, u64 now)
+{
+ struct psi_trigger *t;
+ bool new_stall = false;
+
+ /*
+ * On the first update, initialize the polling state.
+ * Keep the monitor active for at least the duration of the
+ * minimum tracking window. This prevents frequent changes to
+ * clock_mode when system bounces in and out of stall states.
+ */
+ if (cmpxchg(&group->clock_mode, PSI_CLOCK_SWITCHING,
+ PSI_CLOCK_POLLING) == PSI_CLOCK_SWITCHING) {
+ group->polling_until = now + group->trigger_min_period *
+ PSI_TRIG_UPDATES_PER_WIN;
+ list_for_each_entry(t, &group->triggers, node)
+ window_init(&t->win, now, group->total[t->state]);
+ memcpy(group->polling_total, group->total,
+ sizeof(group->polling_total));
+ goto out_next;
+
+ }
+
+ /*
+ * On subsequent updates, calculate growth deltas and let
+ * watchers know when their specified thresholds are exceeded.
+ */
+ list_for_each_entry(t, &group->triggers, node) {
+ u64 growth;
+
+ /* Check for stall activity */
+ if (group->polling_total[t->state] == group->total[t->state])
+ continue;
+
+ /*
+ * Multiple triggers might be looking at the same state,
+ * remember to update group->polling_total[] once we've
+ * been through all of them. Also remember to extend the
+ * polling time if we see new stall activity.
+ */
+ new_stall = true;
+
+ /* Calculate growth since last update */
+ growth = window_update(&t->win, now, group->total[t->state]);
+ if (growth < t->threshold)
+ continue;
+
+ /* Limit event signaling to once per window */
+ if (now < t->last_event_time + t->win.size)
+ continue;
+
+ /* Generate an event */
+ if (cmpxchg(&t->event, 0, 1) == 0)
+ wake_up_interruptible(&t->event_wait);
+ t->last_event_time = now;
+ }
+
+ if (new_stall) {
+ memcpy(group->polling_total, group->total,
+ sizeof(group->polling_total));
+ group->polling_until = now +
+ (group->trigger_min_period * PSI_TRIG_UPDATES_PER_WIN);
+ }
+
+out_next:
+ /* No more new stall in the last window? Disable polling */
+ if (now >= group->polling_until) {
+ WARN_ONCE(group->clock_mode != PSI_CLOCK_POLLING,
+ "psi: invalid clock mode %d\n",
+ group->clock_mode);
+ group->clock_mode = PSI_CLOCK_REGULAR;
+ return ULLONG_MAX;
+ }
+
+ return now + group->trigger_min_period;
+}
+
+/*
+ * Update total stall, update averages if it's time,
+ * check all triggers if in polling state.
+ */
+static void psi_update(struct psi_group *group)
+{
+ u64 now;
+
+ mutex_lock(&group->update_lock);
+
+ collect_percpu_times(group);
+
+ now = sched_clock();
+ if (now >= group->avg_next_update)
+ group->avg_next_update = calculate_averages(group, now);
+
+ if (now >= group->polling_next_update)
+ group->polling_next_update = poll_triggers(group, now);
+
mutex_unlock(&group->update_lock);
}
@@ -358,28 +526,23 @@ static void psi_update_work(struct work_struct *work)
{
struct delayed_work *dwork;
struct psi_group *group;
- unsigned long delay = 0;
- u64 now;
+ u64 next_update;
dwork = to_delayed_work(work);
group = container_of(dwork, struct psi_group, clock_work);
/*
- * If there is task activity, periodically fold the per-cpu
- * times and feed samples into the running averages. If things
- * are idle and there is no data to process, stop the clock.
- * Once restarted, we'll catch up the running averages in one
- * go - see calc_avgs() and missed_periods.
+ * Periodically fold the per-cpu times and feed samples
+ * into the running averages.
*/
- update_stats(group);
+ psi_update(group);
- now = sched_clock();
- if (group->avg_next_update > now) {
- delay = nsecs_to_jiffies(
- group->avg_next_update - now) + 1;
- }
- schedule_delayed_work(dwork, delay);
+ /* Calculate closest update time */
+ next_update = min(group->polling_next_update,
+ group->avg_next_update);
+ schedule_delayed_work(dwork, min(PSI_FREQ,
+ nsecs_to_jiffies(next_update - sched_clock()) + 1));
}
static void record_times(struct psi_group_cpu *groupc, int cpu,
@@ -475,6 +638,24 @@ static void psi_group_change(struct psi_group *group, int cpu,
groupc->state_mask = state_mask;
write_seqcount_end(&groupc->seq);
+
+ /*
+ * If there is a trigger set on this state, make sure the
+ * clock is in polling mode, or switches over right away.
+ *
+ * Monitor state changes into PSI_CLOCK_REGULAR at the max rate
+ * of once per update window (no more than 500ms), therefore
+ * below condition should happen relatively infrequently and
+ * cpu cache invalidation rate should stay low.
+ */
+ if ((state_mask & group->trigger_mask) &&
+ cmpxchg(&group->clock_mode, PSI_CLOCK_REGULAR,
+ PSI_CLOCK_SWITCHING) == PSI_CLOCK_REGULAR) {
+ group->polling_next_update = 0;
+ /* reschedule immediate update */
+ cancel_delayed_work(&group->clock_work);
+ schedule_delayed_work(&group->clock_work, 1);
+ }
}
static struct psi_group *iterate_groups(struct task_struct *task, void **iter)
@@ -623,6 +804,8 @@ void psi_cgroup_free(struct cgroup *cgroup)
cancel_delayed_work_sync(&cgroup->psi.clock_work);
free_percpu(cgroup->psi.pcpu);
+ /* All triggers must be removed by now by psi_trigger_destroy */
+ WARN_ONCE(cgroup->psi.trigger_mask, "psi: trigger leak\n");
}
/**
@@ -682,7 +865,7 @@ int psi_show(struct seq_file *m, struct psi_group *group, enum psi_res res)
if (static_branch_likely(&psi_disabled))
return -EOPNOTSUPP;
- update_stats(group);
+ psi_update(group);
for (full = 0; full < 2 - (res == PSI_CPU); full++) {
unsigned long avg[3];
@@ -734,25 +917,296 @@ static int psi_cpu_open(struct inode *inode, struct file *file)
return single_open(file, psi_cpu_show, NULL);
}
+ssize_t psi_trigger_parse(char *buf, size_t nbytes, enum psi_res res,
+ enum psi_states *state, u32 *threshold_us, u32 *win_sz_us)
+{
+ bool some;
+ bool threshold_pct;
+ u32 threshold;
+ u32 win_sz;
+ char *p;
+
+ p = strsep(&buf, " ");
+ if (p == NULL)
+ return -EINVAL;
+
+ /* parse type */
+ if (!strcmp(p, "some"))
+ some = true;
+ else if (!strcmp(p, "full"))
+ some = false;
+ else
+ return -EINVAL;
+
+ switch (res) {
+ case (PSI_IO):
+ *state = some ? PSI_IO_SOME : PSI_IO_FULL;
+ break;
+ case (PSI_MEM):
+ *state = some ? PSI_MEM_SOME : PSI_MEM_FULL;
+ break;
+ case (PSI_CPU):
+ if (!some)
+ return -EINVAL;
+ *state = PSI_CPU_SOME;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ while (isspace(*buf))
+ buf++;
+
+ p = strsep(&buf, "%");
+ if (p == NULL)
+ return -EINVAL;
+
+ if (buf == NULL) {
+ /* % sign was not found, threshold is specified in us */
+ buf = p;
+ p = strsep(&buf, " ");
+ if (p == NULL)
+ return -EINVAL;
+
+ threshold_pct = false;
+ } else
+ threshold_pct = true;
+
+ /* parse threshold */
+ if (kstrtouint(p, 0, &threshold))
+ return -EINVAL;
+
+ while (isspace(*buf))
+ buf++;
+
+ p = strsep(&buf, " ");
+ if (p == NULL)
+ return -EINVAL;
+
+ /* Parse window size */
+ if (kstrtouint(p, 0, &win_sz))
+ return -EINVAL;
+
+ /* Check window size */
+ if (win_sz < PSI_TRIG_MIN_WIN_US || win_sz > PSI_TRIG_MAX_WIN_US)
+ return -EINVAL;
+
+ if (threshold_pct)
+ threshold = (threshold * win_sz) / 100;
+
+ /* Check threshold */
+ if (threshold == 0 || threshold > win_sz)
+ return -EINVAL;
+
+ *threshold_us = threshold;
+ *win_sz_us = win_sz;
+
+ return 0;
+}
+
+struct psi_trigger *psi_trigger_create(struct psi_group *group,
+ enum psi_states state, u32 threshold_us, u32 win_sz_us)
+{
+ struct psi_trigger *t;
+
+ if (static_branch_likely(&psi_disabled))
+ return ERR_PTR(-EOPNOTSUPP);
+
+ t = kzalloc(sizeof(*t), GFP_KERNEL);
+ if (!t)
+ return ERR_PTR(-ENOMEM);
+
+ t->group = group;
+ t->state = state;
+ t->threshold = threshold_us * NSEC_PER_USEC;
+ t->win.size = win_sz_us * NSEC_PER_USEC;
+ t->event = 0;
+ init_waitqueue_head(&t->event_wait);
+
+ mutex_lock(&group->update_lock);
+
+ list_add(&t->node, &group->triggers);
+ group->trigger_min_period = min(group->trigger_min_period,
+ t->win.size / PSI_TRIG_UPDATES_PER_WIN);
+ group->nr_triggers[t->state]++;
+ group->trigger_mask |= (1 << t->state);
+
+ mutex_unlock(&group->update_lock);
+
+ return t;
+}
+
+void psi_trigger_destroy(struct psi_trigger *t)
+{
+ struct psi_group *group = t->group;
+
+ if (static_branch_likely(&psi_disabled))
+ return;
+
+ mutex_lock(&group->update_lock);
+ if (!list_empty(&t->node)) {
+ struct psi_trigger *tmp;
+ u64 period = ULLONG_MAX;
+
+ list_del_init(&t->node);
+ group->nr_triggers[t->state]--;
+ if (!group->nr_triggers[t->state])
+ group->trigger_mask &= ~(1 << t->state);
+ /* reset min update period for the remaining triggers */
+ list_for_each_entry(tmp, &group->triggers, node) {
+ period = min(period,
+ tmp->win.size / PSI_TRIG_UPDATES_PER_WIN);
+ }
+ group->trigger_min_period = period;
+ /*
+ * Wakeup waiters to stop polling.
+ * Can happen if cgroup is deleted from under
+ * a polling process.
+ */
+ wake_up_interruptible(&t->event_wait);
+ kfree(t);
+ }
+ mutex_unlock(&group->update_lock);
+}
+
+__poll_t psi_trigger_poll(struct psi_trigger *t,
+ struct file *file, poll_table *wait)
+{
+ if (static_branch_likely(&psi_disabled))
+ return DEFAULT_POLLMASK | EPOLLERR | EPOLLPRI;
+
+ poll_wait(file, &t->event_wait, wait);
+
+ if (cmpxchg(&t->event, 1, 0) == 1)
+ return DEFAULT_POLLMASK | EPOLLPRI;
+
+ /* Wait */
+ return DEFAULT_POLLMASK;
+}
+
+static ssize_t psi_write(struct file *file, const char __user *user_buf,
+ size_t nbytes, enum psi_res res)
+{
+ char buf[32];
+ size_t buf_size;
+ struct seq_file *seq;
+ struct psi_trigger *old;
+ struct psi_trigger *new;
+ enum psi_states state;
+ u32 threshold_us;
+ u32 win_sz_us;
+ ssize_t ret;
+
+ if (static_branch_likely(&psi_disabled))
+ return -EOPNOTSUPP;
+
+ buf_size = min(nbytes, (sizeof(buf) - 1));
+ if (copy_from_user(buf, user_buf, buf_size))
+ return -EFAULT;
+
+ buf[buf_size - 1] = '\0';
+
+ ret = psi_trigger_parse(buf, nbytes, res,
+ &state, &threshold_us, &win_sz_us);
+ if (ret < 0)
+ return ret;
+
+ new = psi_trigger_create(&psi_system,
+ state, threshold_us, win_sz_us);
+ if (IS_ERR(new))
+ return PTR_ERR(new);
+
+ seq = file->private_data;
+ /* Take seq->lock to protect seq->private from concurrent writes */
+ mutex_lock(&seq->lock);
+ old = seq->private;
+ rcu_assign_pointer(seq->private, new);
+ mutex_unlock(&seq->lock);
+
+ if (old) {
+ synchronize_rcu();
+ psi_trigger_destroy(old);
+ }
+
+ return nbytes;
+}
+
+static ssize_t psi_io_write(struct file *file,
+ const char __user *user_buf, size_t nbytes, loff_t *ppos)
+{
+ return psi_write(file, user_buf, nbytes, PSI_IO);
+}
+
+static ssize_t psi_memory_write(struct file *file,
+ const char __user *user_buf, size_t nbytes, loff_t *ppos)
+{
+ return psi_write(file, user_buf, nbytes, PSI_MEM);
+}
+
+static ssize_t psi_cpu_write(struct file *file,
+ const char __user *user_buf, size_t nbytes, loff_t *ppos)
+{
+ return psi_write(file, user_buf, nbytes, PSI_CPU);
+}
+
+static __poll_t psi_fop_poll(struct file *file, poll_table *wait)
+{
+ struct seq_file *seq = file->private_data;
+ struct psi_trigger *t;
+ __poll_t ret;
+
+ rcu_read_lock();
+ t = rcu_dereference(seq->private);
+ if (t)
+ ret = psi_trigger_poll(t, file, wait);
+ else
+ ret = DEFAULT_POLLMASK | EPOLLERR | EPOLLPRI;
+ rcu_read_unlock();
+
+ return ret;
+
+}
+
+static int psi_fop_release(struct inode *inode, struct file *file)
+{
+ struct seq_file *seq = file->private_data;
+ struct psi_trigger *t = seq->private;
+
+ if (static_branch_likely(&psi_disabled) || !t)
+ goto out;
+
+ rcu_assign_pointer(seq->private, NULL);
+ synchronize_rcu();
+ psi_trigger_destroy(t);
+out:
+ return single_release(inode, file);
+}
+
static const struct file_operations psi_io_fops = {
.open = psi_io_open,
.read = seq_read,
.llseek = seq_lseek,
- .release = single_release,
+ .write = psi_io_write,
+ .poll = psi_fop_poll,
+ .release = psi_fop_release,
};
static const struct file_operations psi_memory_fops = {
.open = psi_memory_open,
.read = seq_read,
.llseek = seq_lseek,
- .release = single_release,
+ .write = psi_memory_write,
+ .poll = psi_fop_poll,
+ .release = psi_fop_release,
};
static const struct file_operations psi_cpu_fops = {
.open = psi_cpu_open,
.read = seq_read,
.llseek = seq_lseek,
- .release = single_release,
+ .write = psi_cpu_write,
+ .poll = psi_fop_poll,
+ .release = psi_fop_release,
};
static int __init psi_late_init(void)
--
2.20.0.405.gbc1bbc6f85-goog
On Fri, Dec 14, 2018 at 09:15:08AM -0800, Suren Baghdasaryan wrote:
> @@ -358,28 +526,23 @@ static void psi_update_work(struct work_struct *work)
> {
> struct delayed_work *dwork;
> struct psi_group *group;
> + u64 next_update;
>
> dwork = to_delayed_work(work);
> group = container_of(dwork, struct psi_group, clock_work);
>
> /*
> + * Periodically fold the per-cpu times and feed samples
> + * into the running averages.
> */
>
> + psi_update(group);
>
> + /* Calculate closest update time */
> + next_update = min(group->polling_next_update,
> + group->avg_next_update);
> + schedule_delayed_work(dwork, min(PSI_FREQ,
> + nsecs_to_jiffies(next_update - sched_clock()) + 1));
See, so I don't at _all_ like how there is no idle option..
> }
On Fri, Dec 14, 2018 at 09:15:05AM -0800, Suren Baghdasaryan wrote:
> Eliminate the idle mode and keep the worker doing 2s update intervals
> at all times.
That sounds like a bad deal.. esp. so for battery powered devices like
say Andoird.
In general the push has been to always idle everything, see NOHZ and
NOHZ_FULL and all the work that's being put into getting rid of any and
all period work.
On Fri, Dec 14, 2018 at 09:15:06AM -0800, Suren Baghdasaryan wrote:
> The psi monitoring patches will need to determine the same states as
> record_times(). To avoid calculating them twice, maintain a state mask
> that can be consulted cheaply. Do this in a separate patch to keep the
> churn in the main feature patch at a minimum.
>
> Signed-off-by: Suren Baghdasaryan <[email protected]>
> ---
> include/linux/psi_types.h | 3 +++
> kernel/sched/psi.c | 29 +++++++++++++++++++----------
> 2 files changed, 22 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/psi_types.h b/include/linux/psi_types.h
> index 2cf422db5d18..2c6e9b67b7eb 100644
> --- a/include/linux/psi_types.h
> +++ b/include/linux/psi_types.h
> @@ -53,6 +53,9 @@ struct psi_group_cpu {
> /* States of the tasks belonging to this group */
> unsigned int tasks[NR_PSI_TASK_COUNTS];
>
> + /* Aggregate pressure state derived from the tasks */
> + u32 state_mask;
> +
> /* Period time sampling buckets for each state of interest (ns) */
> u32 times[NR_PSI_STATES];
>
Since we spend so much time counting space in that line, maybe add a
note to the Changlog about how this fits.
Also, since I just had to re-count, you might want to add explicit
numbers to the psi_res and psi_states enums.
> + if (state_mask & (1 << s))
We have the BIT() macro, but I'm honestly not sure that will improve
things.
On Fri, Dec 14, 2018 at 09:15:08AM -0800, Suren Baghdasaryan wrote:
> +ssize_t psi_trigger_parse(char *buf, size_t nbytes, enum psi_res res,
> + enum psi_states *state, u32 *threshold_us, u32 *win_sz_us)
> +{
> + bool some;
> + bool threshold_pct;
> + u32 threshold;
> + u32 win_sz;
> + char *p;
> +
> + p = strsep(&buf, " ");
> + if (p == NULL)
> + return -EINVAL;
> +
> + /* parse type */
> + if (!strcmp(p, "some"))
> + some = true;
> + else if (!strcmp(p, "full"))
> + some = false;
> + else
> + return -EINVAL;
> +
> + switch (res) {
> + case (PSI_IO):
> + *state = some ? PSI_IO_SOME : PSI_IO_FULL;
> + break;
> + case (PSI_MEM):
> + *state = some ? PSI_MEM_SOME : PSI_MEM_FULL;
> + break;
> + case (PSI_CPU):
> + if (!some)
> + return -EINVAL;
> + *state = PSI_CPU_SOME;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + while (isspace(*buf))
> + buf++;
> +
> + p = strsep(&buf, "%");
> + if (p == NULL)
> + return -EINVAL;
> +
> + if (buf == NULL) {
> + /* % sign was not found, threshold is specified in us */
> + buf = p;
> + p = strsep(&buf, " ");
> + if (p == NULL)
> + return -EINVAL;
> +
> + threshold_pct = false;
> + } else
> + threshold_pct = true;
> +
> + /* parse threshold */
> + if (kstrtouint(p, 0, &threshold))
> + return -EINVAL;
> +
> + while (isspace(*buf))
> + buf++;
> +
> + p = strsep(&buf, " ");
> + if (p == NULL)
> + return -EINVAL;
> +
> + /* Parse window size */
> + if (kstrtouint(p, 0, &win_sz))
> + return -EINVAL;
> +
> + /* Check window size */
> + if (win_sz < PSI_TRIG_MIN_WIN_US || win_sz > PSI_TRIG_MAX_WIN_US)
> + return -EINVAL;
> +
> + if (threshold_pct)
> + threshold = (threshold * win_sz) / 100;
> +
> + /* Check threshold */
> + if (threshold == 0 || threshold > win_sz)
> + return -EINVAL;
> +
> + *threshold_us = threshold;
> + *win_sz_us = win_sz;
> +
> + return 0;
> +}
How well has this thing been fuzzed? Custom string parser, yay!
On Mon, Dec 17, 2018 at 6:58 AM Peter Zijlstra <[email protected]> wrote:
>
> On Fri, Dec 14, 2018 at 09:15:05AM -0800, Suren Baghdasaryan wrote:
> > Eliminate the idle mode and keep the worker doing 2s update intervals
> > at all times.
>
> That sounds like a bad deal.. esp. so for battery powered devices like
> say Andoird.
>
> In general the push has been to always idle everything, see NOHZ and
> NOHZ_FULL and all the work that's being put into getting rid of any and
> all period work.
Thanks for the feedback Peter! The removal of idle mode is unfortunate
but so far we could not find an elegant solution to handle 3 states
(IDLE / REGULAR / POLLING) without additional synchronization inside
the hotpath. The issue, as I remember it, was that while scheduling a
regular update inside psi_group_change() (IDLE to REGULAR transition)
we might override an earlier update being scheduled inside
psi_update_work(). I think we can solve that by using
mod_delayed_work_on() inside psi_update_work() but I might be missing
some other race. I'll discuss this again with Johannes and see if we
can synchronize all states using only atomic operations on clock_mode.
> --
> You received this message because you are subscribed to the Google Groups "kernel-team" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
>
On Mon, Dec 17, 2018 at 7:55 AM Peter Zijlstra <[email protected]> wrote:
>
> On Fri, Dec 14, 2018 at 09:15:06AM -0800, Suren Baghdasaryan wrote:
> > The psi monitoring patches will need to determine the same states as
> > record_times(). To avoid calculating them twice, maintain a state mask
> > that can be consulted cheaply. Do this in a separate patch to keep the
> > churn in the main feature patch at a minimum.
> >
> > Signed-off-by: Suren Baghdasaryan <[email protected]>
> > ---
> > include/linux/psi_types.h | 3 +++
> > kernel/sched/psi.c | 29 +++++++++++++++++++----------
> > 2 files changed, 22 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/linux/psi_types.h b/include/linux/psi_types.h
> > index 2cf422db5d18..2c6e9b67b7eb 100644
> > --- a/include/linux/psi_types.h
> > +++ b/include/linux/psi_types.h
> > @@ -53,6 +53,9 @@ struct psi_group_cpu {
> > /* States of the tasks belonging to this group */
> > unsigned int tasks[NR_PSI_TASK_COUNTS];
> >
> > + /* Aggregate pressure state derived from the tasks */
> > + u32 state_mask;
> > +
> > /* Period time sampling buckets for each state of interest (ns) */
> > u32 times[NR_PSI_STATES];
> >
>
> Since we spend so much time counting space in that line, maybe add a
> note to the Changlog about how this fits.
Will do.
> Also, since I just had to re-count, you might want to add explicit
> numbers to the psi_res and psi_states enums.
Sounds reasonable.
> > + if (state_mask & (1 << s))
>
> We have the BIT() macro, but I'm honestly not sure that will improve
> things.
I was mimicking the rest of the code in psi.c that uses this kind of
bit masking. Can change if you think that would be better.
On Mon, Dec 17, 2018 at 8:22 AM Peter Zijlstra <[email protected]> wrote:
>
> On Fri, Dec 14, 2018 at 09:15:08AM -0800, Suren Baghdasaryan wrote:
> > +ssize_t psi_trigger_parse(char *buf, size_t nbytes, enum psi_res res,
> > + enum psi_states *state, u32 *threshold_us, u32 *win_sz_us)
> > +{
> > + bool some;
> > + bool threshold_pct;
> > + u32 threshold;
> > + u32 win_sz;
> > + char *p;
> > +
> > + p = strsep(&buf, " ");
> > + if (p == NULL)
> > + return -EINVAL;
> > +
> > + /* parse type */
> > + if (!strcmp(p, "some"))
> > + some = true;
> > + else if (!strcmp(p, "full"))
> > + some = false;
> > + else
> > + return -EINVAL;
> > +
> > + switch (res) {
> > + case (PSI_IO):
> > + *state = some ? PSI_IO_SOME : PSI_IO_FULL;
> > + break;
> > + case (PSI_MEM):
> > + *state = some ? PSI_MEM_SOME : PSI_MEM_FULL;
> > + break;
> > + case (PSI_CPU):
> > + if (!some)
> > + return -EINVAL;
> > + *state = PSI_CPU_SOME;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + while (isspace(*buf))
> > + buf++;
> > +
> > + p = strsep(&buf, "%");
> > + if (p == NULL)
> > + return -EINVAL;
> > +
> > + if (buf == NULL) {
> > + /* % sign was not found, threshold is specified in us */
> > + buf = p;
> > + p = strsep(&buf, " ");
> > + if (p == NULL)
> > + return -EINVAL;
> > +
> > + threshold_pct = false;
> > + } else
> > + threshold_pct = true;
> > +
> > + /* parse threshold */
> > + if (kstrtouint(p, 0, &threshold))
> > + return -EINVAL;
> > +
> > + while (isspace(*buf))
> > + buf++;
> > +
> > + p = strsep(&buf, " ");
> > + if (p == NULL)
> > + return -EINVAL;
> > +
> > + /* Parse window size */
> > + if (kstrtouint(p, 0, &win_sz))
> > + return -EINVAL;
> > +
> > + /* Check window size */
> > + if (win_sz < PSI_TRIG_MIN_WIN_US || win_sz > PSI_TRIG_MAX_WIN_US)
> > + return -EINVAL;
> > +
> > + if (threshold_pct)
> > + threshold = (threshold * win_sz) / 100;
> > +
> > + /* Check threshold */
> > + if (threshold == 0 || threshold > win_sz)
> > + return -EINVAL;
> > +
> > + *threshold_us = threshold;
> > + *win_sz_us = win_sz;
> > +
> > + return 0;
> > +}
>
> How well has this thing been fuzzed? Custom string parser, yay!
Honestly, not much. Normal cases and some obvious corner cases. Will
check if I can use some fuzzer to get more coverage or will write a
script.
I'm not thrilled about writing a custom parser, so if there is a
better way to handle this please advise.
> --
> You received this message because you are subscribed to the Google Groups "kernel-team" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
>
On Mon, Dec 17, 2018 at 8:37 AM Peter Zijlstra <[email protected]> wrote:
>
> On Fri, Dec 14, 2018 at 09:15:08AM -0800, Suren Baghdasaryan wrote:
> > @@ -358,28 +526,23 @@ static void psi_update_work(struct work_struct *work)
> > {
> > struct delayed_work *dwork;
> > struct psi_group *group;
> > + u64 next_update;
> >
> > dwork = to_delayed_work(work);
> > group = container_of(dwork, struct psi_group, clock_work);
> >
> > /*
> > + * Periodically fold the per-cpu times and feed samples
> > + * into the running averages.
> > */
> >
> > + psi_update(group);
> >
> > + /* Calculate closest update time */
> > + next_update = min(group->polling_next_update,
> > + group->avg_next_update);
> > + schedule_delayed_work(dwork, min(PSI_FREQ,
> > + nsecs_to_jiffies(next_update - sched_clock()) + 1));
>
> See, so I don't at _all_ like how there is no idle option..
Copy that. Will see what we can do to bring it back.
Thanks!
> > }
>
>
> --
> You received this message because you are subscribed to the Google Groups "kernel-team" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
>
On Mon, Dec 17, 2018 at 05:14:53PM -0800, Suren Baghdasaryan wrote:
> On Mon, Dec 17, 2018 at 7:55 AM Peter Zijlstra <[email protected]> wrote:
> > > + if (state_mask & (1 << s))
> >
> > We have the BIT() macro, but I'm honestly not sure that will improve
> > things.
>
> I was mimicking the rest of the code in psi.c that uses this kind of
> bit masking. Can change if you think that would be better.
Yeah, I really don't know.. keep it as is I suppose.
On Mon, Dec 17, 2018 at 05:21:05PM -0800, Suren Baghdasaryan wrote:
> On Mon, Dec 17, 2018 at 8:22 AM Peter Zijlstra <[email protected]> wrote:
> > How well has this thing been fuzzed? Custom string parser, yay!
>
> Honestly, not much. Normal cases and some obvious corner cases. Will
> check if I can use some fuzzer to get more coverage or will write a
> script.
> I'm not thrilled about writing a custom parser, so if there is a
> better way to handle this please advise.
The grammar seems fairly simple, something like:
some-full = "some" | "full" ;
threshold-abs = integer ;
threshold-pct = integer, { "%" } ;
threshold = threshold-abs | threshold-pct ;
window = integer ;
trigger = some-full, space, threshold, space, window ;
And that could even be expressed as two scanf formats:
"%4s %u%% %u" , "%4s %u %u"
which then gets your something like:
char type[5];
if (sscanf(input, "%4s %u%% %u", &type, &pct, &window) == 3) {
// do pct thing
} else if (sscanf(intput, "%4s %u %u", &type, &thres, &window) == 3) {
// do abs thing
} else return -EFAIL;
if (!strcmp(type, "some")) {
// some
} else if (!strcmp(type, "full")) {
// full
} else return -EFAIL;
// do more
which seems like a lot less error prone. Alternatively you can use 4
formats:
"some %u%% %u" "some %u %u"
"full %u%% %u" "full %u %u"
and avoid the whole 'type' thing.
On Tue, Dec 18, 2018 at 11:46:22AM +0100, Peter Zijlstra wrote:
> On Mon, Dec 17, 2018 at 05:21:05PM -0800, Suren Baghdasaryan wrote:
> > On Mon, Dec 17, 2018 at 8:22 AM Peter Zijlstra <[email protected]> wrote:
>
> > > How well has this thing been fuzzed? Custom string parser, yay!
> >
> > Honestly, not much. Normal cases and some obvious corner cases. Will
> > check if I can use some fuzzer to get more coverage or will write a
> > script.
> > I'm not thrilled about writing a custom parser, so if there is a
> > better way to handle this please advise.
>
> The grammar seems fairly simple, something like:
>
> some-full = "some" | "full" ;
> threshold-abs = integer ;
> threshold-pct = integer, { "%" } ;
Sorry, no {} there obviously. That '%' isn't optional.
> threshold = threshold-abs | threshold-pct ;
> window = integer ;
> trigger = some-full, space, threshold, space, window ;
Clearly it's been a fair while since I wrote BNF like stuff ;-)
Hi Suren,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.20-rc7]
[cannot apply to next-20181218]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Suren-Baghdasaryan/psi-pressure-stall-monitors/20181215-181714
config: i386-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
kernel/sched/psi.o: In function `psi_trigger_create':
>> psi.c:(.text+0x1709): undefined reference to `__udivdi3'
kernel/sched/psi.o: In function `psi_trigger_destroy':
psi.c:(.text+0x1833): undefined reference to `__udivdi3'
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Tue, Dec 18, 2018 at 11:46:22AM +0100, Peter Zijlstra wrote:
> On Mon, Dec 17, 2018 at 05:21:05PM -0800, Suren Baghdasaryan wrote:
> > On Mon, Dec 17, 2018 at 8:22 AM Peter Zijlstra <[email protected]> wrote:
>
> > > How well has this thing been fuzzed? Custom string parser, yay!
> >
> > Honestly, not much. Normal cases and some obvious corner cases. Will
> > check if I can use some fuzzer to get more coverage or will write a
> > script.
> > I'm not thrilled about writing a custom parser, so if there is a
> > better way to handle this please advise.
>
> The grammar seems fairly simple, something like:
>
> some-full = "some" | "full" ;
> threshold-abs = integer ;
> threshold-pct = integer, { "%" } ;
> threshold = threshold-abs | threshold-pct ;
> window = integer ;
> trigger = some-full, space, threshold, space, window ;
>
> And that could even be expressed as two scanf formats:
>
> "%4s %u%% %u" , "%4s %u %u"
>
> which then gets your something like:
>
> char type[5];
>
> if (sscanf(input, "%4s %u%% %u", &type, &pct, &window) == 3) {
> // do pct thing
> } else if (sscanf(intput, "%4s %u %u", &type, &thres, &window) == 3) {
> // do abs thing
> } else return -EFAIL;
>
> if (!strcmp(type, "some")) {
> // some
> } else if (!strcmp(type, "full")) {
> // full
> } else return -EFAIL;
>
> // do more
We might want to drop the percentage notation.
While it's somewhat convenient, it's also not unreasonable to ask
userspace to do a simple "threshold * win / 100" themselves, and it
would simplify the interface spec and the parser.
Sure, psi outputs percentages, but only for fixed window sizes, so
that actually saves us something, whereas this parser here needs to
take a fractional anyway. The output is also in decimal notation,
which is necessary for granularity. And I really don't think we want
to add float parsing on top of this interface spec.
So neither the convenience nor the symmetry argument are very
compelling IMO. It might be better to just not go there.
Current design supports only whole percentages and if userspace needs
more granularity then it has to use usecs.
I agree that usecs cover % usecase and "threshold * win / 100" is
simple enough for userspace to calculate. I'm fine with changing to
usecs only.
On Tue, Dec 18, 2018 at 9:30 AM Johannes Weiner <[email protected]> wrote:
>
> On Tue, Dec 18, 2018 at 11:46:22AM +0100, Peter Zijlstra wrote:
> > On Mon, Dec 17, 2018 at 05:21:05PM -0800, Suren Baghdasaryan wrote:
> > > On Mon, Dec 17, 2018 at 8:22 AM Peter Zijlstra <[email protected]> wrote:
> >
> > > > How well has this thing been fuzzed? Custom string parser, yay!
> > >
> > > Honestly, not much. Normal cases and some obvious corner cases. Will
> > > check if I can use some fuzzer to get more coverage or will write a
> > > script.
> > > I'm not thrilled about writing a custom parser, so if there is a
> > > better way to handle this please advise.
> >
> > The grammar seems fairly simple, something like:
> >
> > some-full = "some" | "full" ;
> > threshold-abs = integer ;
> > threshold-pct = integer, { "%" } ;
> > threshold = threshold-abs | threshold-pct ;
> > window = integer ;
> > trigger = some-full, space, threshold, space, window ;
> >
> > And that could even be expressed as two scanf formats:
> >
> > "%4s %u%% %u" , "%4s %u %u"
> >
> > which then gets your something like:
> >
> > char type[5];
> >
> > if (sscanf(input, "%4s %u%% %u", &type, &pct, &window) == 3) {
> > // do pct thing
> > } else if (sscanf(intput, "%4s %u %u", &type, &thres, &window) == 3) {
> > // do abs thing
> > } else return -EFAIL;
> >
> > if (!strcmp(type, "some")) {
> > // some
> > } else if (!strcmp(type, "full")) {
> > // full
> > } else return -EFAIL;
> >
> > // do more
>
> We might want to drop the percentage notation.
>
> While it's somewhat convenient, it's also not unreasonable to ask
> userspace to do a simple "threshold * win / 100" themselves, and it
> would simplify the interface spec and the parser.
>
> Sure, psi outputs percentages, but only for fixed window sizes, so
> that actually saves us something, whereas this parser here needs to
> take a fractional anyway. The output is also in decimal notation,
> which is necessary for granularity. And I really don't think we want
> to add float parsing on top of this interface spec.
>
> So neither the convenience nor the symmetry argument are very
> compelling IMO. It might be better to just not go there.
On Tue, Dec 18, 2018 at 9:58 AM 'Suren Baghdasaryan' via kernel-team
<[email protected]> wrote:
>
> Current design supports only whole percentages and if userspace needs
> more granularity then it has to use usecs.
> I agree that usecs cover % usecase and "threshold * win / 100" is
> simple enough for userspace to calculate. I'm fine with changing to
> usecs only.
Suren, please avoid top-posting to LKML.
Also I was going to say the same thing, just usecs only is better.
thanks,
- Joel
> On Tue, Dec 18, 2018 at 9:30 AM Johannes Weiner <[email protected]> wrote:
> >
> > On Tue, Dec 18, 2018 at 11:46:22AM +0100, Peter Zijlstra wrote:
> > > On Mon, Dec 17, 2018 at 05:21:05PM -0800, Suren Baghdasaryan wrote:
> > > > On Mon, Dec 17, 2018 at 8:22 AM Peter Zijlstra <[email protected]> wrote:
> > >
> > > > > How well has this thing been fuzzed? Custom string parser, yay!
> > > >
> > > > Honestly, not much. Normal cases and some obvious corner cases. Will
> > > > check if I can use some fuzzer to get more coverage or will write a
> > > > script.
> > > > I'm not thrilled about writing a custom parser, so if there is a
> > > > better way to handle this please advise.
> > >
> > > The grammar seems fairly simple, something like:
> > >
> > > some-full = "some" | "full" ;
> > > threshold-abs = integer ;
> > > threshold-pct = integer, { "%" } ;
> > > threshold = threshold-abs | threshold-pct ;
> > > window = integer ;
> > > trigger = some-full, space, threshold, space, window ;
> > >
> > > And that could even be expressed as two scanf formats:
> > >
> > > "%4s %u%% %u" , "%4s %u %u"
> > >
> > > which then gets your something like:
> > >
> > > char type[5];
> > >
> > > if (sscanf(input, "%4s %u%% %u", &type, &pct, &window) == 3) {
> > > // do pct thing
> > > } else if (sscanf(intput, "%4s %u %u", &type, &thres, &window) == 3) {
> > > // do abs thing
> > > } else return -EFAIL;
> > >
> > > if (!strcmp(type, "some")) {
> > > // some
> > > } else if (!strcmp(type, "full")) {
> > > // full
> > > } else return -EFAIL;
> > >
> > > // do more
> >
> > We might want to drop the percentage notation.
> >
> > While it's somewhat convenient, it's also not unreasonable to ask
> > userspace to do a simple "threshold * win / 100" themselves, and it
> > would simplify the interface spec and the parser.
> >
> > Sure, psi outputs percentages, but only for fixed window sizes, so
> > that actually saves us something, whereas this parser here needs to
> > take a fractional anyway. The output is also in decimal notation,
> > which is necessary for granularity. And I really don't think we want
> > to add float parsing on top of this interface spec.
> >
> > So neither the convenience nor the symmetry argument are very
> > compelling IMO. It might be better to just not go there.
>
> --
> You received this message because you are subscribed to the Google Groups "kernel-team" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
>
On Tue, Dec 18, 2018 at 11:18 AM Joel Fernandes <[email protected]> wrote:
>
> On Tue, Dec 18, 2018 at 9:58 AM 'Suren Baghdasaryan' via kernel-team
> <[email protected]> wrote:
> >
> > Current design supports only whole percentages and if userspace needs
> > more granularity then it has to use usecs.
> > I agree that usecs cover % usecase and "threshold * win / 100" is
> > simple enough for userspace to calculate. I'm fine with changing to
> > usecs only.
>
> Suren, please avoid top-posting to LKML.
Sorry, did that by accident.
> Also I was going to say the same thing, just usecs only is better.
Thanks for the input.
> thanks,
>
> - Joel
> > On Tue, Dec 18, 2018 at 9:30 AM Johannes Weiner <[email protected]> wrote:
> > >
> > > On Tue, Dec 18, 2018 at 11:46:22AM +0100, Peter Zijlstra wrote:
> > > > On Mon, Dec 17, 2018 at 05:21:05PM -0800, Suren Baghdasaryan wrote:
> > > > > On Mon, Dec 17, 2018 at 8:22 AM Peter Zijlstra <[email protected]> wrote:
> > > >
> > > > > > How well has this thing been fuzzed? Custom string parser, yay!
> > > > >
> > > > > Honestly, not much. Normal cases and some obvious corner cases. Will
> > > > > check if I can use some fuzzer to get more coverage or will write a
> > > > > script.
> > > > > I'm not thrilled about writing a custom parser, so if there is a
> > > > > better way to handle this please advise.
> > > >
> > > > The grammar seems fairly simple, something like:
> > > >
> > > > some-full = "some" | "full" ;
> > > > threshold-abs = integer ;
> > > > threshold-pct = integer, { "%" } ;
> > > > threshold = threshold-abs | threshold-pct ;
> > > > window = integer ;
> > > > trigger = some-full, space, threshold, space, window ;
> > > >
> > > > And that could even be expressed as two scanf formats:
> > > >
> > > > "%4s %u%% %u" , "%4s %u %u"
> > > >
> > > > which then gets your something like:
> > > >
> > > > char type[5];
> > > >
> > > > if (sscanf(input, "%4s %u%% %u", &type, &pct, &window) == 3) {
> > > > // do pct thing
> > > > } else if (sscanf(intput, "%4s %u %u", &type, &thres, &window) == 3) {
> > > > // do abs thing
> > > > } else return -EFAIL;
> > > >
> > > > if (!strcmp(type, "some")) {
> > > > // some
> > > > } else if (!strcmp(type, "full")) {
> > > > // full
> > > > } else return -EFAIL;
> > > >
> > > > // do more
> > >
> > > We might want to drop the percentage notation.
> > >
> > > While it's somewhat convenient, it's also not unreasonable to ask
> > > userspace to do a simple "threshold * win / 100" themselves, and it
> > > would simplify the interface spec and the parser.
> > >
> > > Sure, psi outputs percentages, but only for fixed window sizes, so
> > > that actually saves us something, whereas this parser here needs to
> > > take a fractional anyway. The output is also in decimal notation,
> > > which is necessary for granularity. And I really don't think we want
> > > to add float parsing on top of this interface spec.
> > >
> > > So neither the convenience nor the symmetry argument are very
> > > compelling IMO. It might be better to just not go there.
> >
> > --
> > You received this message because you are subscribed to the Google Groups "kernel-team" group.
> > To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
> >
Hi Suren,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v4.20-rc7]
[cannot apply to next-20181221]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Suren-Baghdasaryan/psi-pressure-stall-monitors/20181215-181714
config: i386-randconfig-i2-12162314 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
kernel/sched/psi.o: In function `psi_trigger_create':
>> kernel/sched/psi.c:1029: undefined reference to `__udivdi3'
kernel/sched/psi.o: In function `psi_trigger_destroy':
kernel/sched/psi.c:1057: undefined reference to `__udivdi3'
vim +1029 kernel/sched/psi.c
1006
1007 struct psi_trigger *psi_trigger_create(struct psi_group *group,
1008 enum psi_states state, u32 threshold_us, u32 win_sz_us)
1009 {
1010 struct psi_trigger *t;
1011
1012 if (static_branch_likely(&psi_disabled))
1013 return ERR_PTR(-EOPNOTSUPP);
1014
1015 t = kzalloc(sizeof(*t), GFP_KERNEL);
1016 if (!t)
1017 return ERR_PTR(-ENOMEM);
1018
1019 t->group = group;
1020 t->state = state;
1021 t->threshold = threshold_us * NSEC_PER_USEC;
1022 t->win.size = win_sz_us * NSEC_PER_USEC;
1023 t->event = 0;
1024 init_waitqueue_head(&t->event_wait);
1025
1026 mutex_lock(&group->update_lock);
1027
1028 list_add(&t->node, &group->triggers);
> 1029 group->trigger_min_period = min(group->trigger_min_period,
1030 t->win.size / PSI_TRIG_UPDATES_PER_WIN);
1031 group->nr_triggers[t->state]++;
1032 group->trigger_mask |= (1 << t->state);
1033
1034 mutex_unlock(&group->update_lock);
1035
1036 return t;
1037 }
1038
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation