UPSTREAM: psi: fix aggregation idle shut-off

psi has provisions to shut off the periodic aggregation worker when
there is a period of no task activity - and thus no data that needs
aggregating.  However, while developing psi monitoring, Suren noticed
that the aggregation clock currently won't stay shut off for good.

DebugGing this revealed a flaw in the idle design: an aggregation run
will see no task activity and decide to go to sleep; shortly thereafter,
the kworker thread that executed the aggregation will go idle and cause
a scheduling change, during which the psi callback will kick the
!pending worker again.  This will ping-pong forever, and is equivalent
to having no shut-off logic at all (but with more code!)

Fix this by exempting aggregation workers from psi's clock waking logic
when the state change is them going to sleep.  To do this, tag workers
with the last work function they executed, and if in psi we see a worker
going to sleep after aggregating psi data, we will not reschedule the
aggregation work item.

What if the worker is also executing other items before or after?

Any psi state times that were incurred by work items preceding the
aggregation work will have been collected from the per-cpu buckets
during the aggregation itself.  If there are work items following the
aggregation work, the worker's last_func tag will be overwritten and the
aggregator will be kept alive to process this genuine new activity.

If the aggregation work is the last thing the worker does, and we decide
to go idle, the brief period of non-idle time incurred between the
aggregation run and the kworker's dequeue will be stranded in the
per-cpu buckets until the clock is woken by later activity.  But that
should not be a problem.  The buckets can hold 4s worth of time, and
future activity will wake the clock with a 2s delay, giving us 2s worth
of data we can leave behind when disabling aggregation.  If it takes a
worker more than two seconds to go idle after it finishes its last work
item, we likely have bigger problems in the system, and won't notice one
sample that was averaged with a bogus per-CPU weight.

Link: http://lkml.kernel.org/r/20190116193501.1910-1-hannes@cmpxchg.org
Fixes: eb414681d5a0 ("psi: pressure stall information for CPU, memory, and IO")
Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Reported-by: Suren Baghdasaryan <surenb@google.com>
Acked-by: Tejun Heo <tj@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Lai Jiangshan <jiangshanlai@gmail.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
(cherry picked from commit 1b69ac6b40ebd85eed73e4dbccde2a36961ab990)

Bug: 127712811
Test: lmkd in PSI mode
Change-Id: I2877fec3d381b1006b8bd1261895fdfd68bd21db
Signed-off-by: Suren Baghdasaryan <surenb@google.com>

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index fe24de3fbc93805f0c1e913a85657a15d141ad2f..c3484785b17958df85b11ca2823a1f4a4d41f59b 100644 (file)
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -124,6 +124,7 @@
  * sampling of the aggregate task states would be.
  */
 
+#include "../workqueue_internal.h"
 #include <linux/sched/loadavg.h>
 #include <linux/seq_file.h>
 #include <linux/proc_fs.h>
@@ -480,9 +481,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)
@@ -513,6 +511,7 @@ void psi_task_change(struct task_struct *task, int clear, int set)
 {
        int cpu = task_cpu(task);
        struct psi_group *group;
+       bool wake_clock = true;
        void *iter = NULL;
 
        if (!task->pid)
@@ -530,8 +529,22 @@ void psi_task_change(struct task_struct *task, int clear, int set)
        task->psi_flags &= ~clear;
        task->psi_flags |= set;
 
-       while ((group = iterate_groups(task, &iter)))
+       /*
+        * Periodic aggregation shuts off if there is a period of no
+        * task changes, so we wake it back up if necessary. However,
+        * don't do this if the task change is the aggregation worker
+        * itself going to sleep, or we'll ping-pong forever.
+        */
+       if (unlikely((clear & TSK_RUNNING) &&
+                    (task->flags & PF_WQ_WORKER) &&
+                    wq_worker_last_func(task) == psi_update_work))
+               wake_clock = false;
+
+       while ((group = iterate_groups(task, &iter))) {
                psi_group_change(group, cpu, clear, set);
+               if (wake_clock && !delayed_work_pending(&group->clock_work))
+                       schedule_delayed_work(&group->clock_work, PSI_FREQ);
+       }
 }
 
 void psi_memstall_tick(struct task_struct *task, int cpu)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 8906016a392bd9d364550d648eb3798e1df3b0ba..62ac1e5e4bb76ec0034f01eb0935d8472d77a52f 100644 (file)
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -912,6 +912,26 @@ struct task_struct *wq_worker_sleeping(struct task_struct *task)
        return to_wakeup ? to_wakeup->task : NULL;
 }
 
+/**
+ * wq_worker_last_func - retrieve worker's last work function
+ *
+ * Determine the last function a worker executed. This is called from
+ * the scheduler to get a worker's last known identity.
+ *
+ * CONTEXT:
+ * spin_lock_irq(rq->lock)
+ *
+ * Return:
+ * The last work function %current executed as a worker, NULL if it
+ * hasn't executed any work yet.
+ */
+work_func_t wq_worker_last_func(struct task_struct *task)
+{
+       struct worker *worker = kthread_data(task);
+
+       return worker->last_func;
+}
+
 /**
  * worker_set_flags - set worker flags and adjust nr_running accordingly
  * @worker: self
@@ -2148,6 +2168,9 @@ __acquires(&pool->lock)
        if (unlikely(cpu_intensive))
                worker_clr_flags(worker, WORKER_CPU_INTENSIVE);
 
+       /* tag the worker for identification in schedule() */
+       worker->last_func = worker->current_func;
+
        /* we're done with it, release */
        hash_del(&worker->hentry);
        worker->current_work = NULL;

diff --git a/kernel/workqueue_internal.h b/kernel/workqueue_internal.h
index d390d1be37489f37bd8ac42826a12782a4ca99dc..f8f5fd8df88fa92493eefd444c1eb48ebb8529ee 100644 (file)
--- a/kernel/workqueue_internal.h
+++ b/kernel/workqueue_internal.h
@@ -54,6 +54,9 @@ struct worker {
 
        /* used only by rescuers to point to the target workqueue */
        struct workqueue_struct *rescue_wq;     /* I: the workqueue to rescue */
+
+       /* used by the scheduler to determine a worker's last known identity */
+       work_func_t             last_func;
 };
 
 /**
@@ -68,9 +71,10 @@ static inline struct worker *current_wq_worker(void)
 
 /*
  * Scheduler hooks for concurrency managed workqueue.  Only to be used from
- * sched/core.c and workqueue.c.
+ * sched/ and workqueue.c.
  */
 void wq_worker_waking_up(struct task_struct *task, int cpu);
 struct task_struct *wq_worker_sleeping(struct task_struct *task);
+work_func_t wq_worker_last_func(struct task_struct *task);
 
 #endif /* _KERNEL_WORKQUEUE_INTERNAL_H */