sched,psi: fix the 'int' underflow for psi
Subsystems (2)
PATCH 1/1 sched,psi: fix the 'int' underflow for psi
psi_group_cpu->tasks, represented by the unsigned int, stores the number of tasks that could be stalled on a psi resource(io/mem/cpu). Decrementing these counters at zero leads to wrapping which further leads to the psi_group_cpu->state_mask is being set with the respective pressure state. This could result into the unnecessary time sampling for the pressure state thus cause the spurious psi events. This can further lead to wrong actions being taken at the user land based on these psi events.
Though psi_bug is set under these conditions but that just for debug purpose. Fix it by decrementing the ->tasks count only when it is non-zero.
--- kernel/sched/psi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
kernel/sched/psi.c +2 -1
@@ -718,7 +718,8 @@ static void psi_group_change(struct psi_group *group, int cpu, groupc->tasks[3], clear, set); psi_bug =1 ; } - groupc->tasks[t]--; +if (groupc->tasks[t]) + groupc->tasks[t]--; }for (t =0 ; set; set &= ~(1 << t), t++)
Signature · 2 lines
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation
Johannes Weiner
maintainerQuoted 11 lines · Charan Teja Reddy
psi_group_cpu->tasks, represented by the unsigned int, stores the number of tasks that could be stalled on a psi resource(io/mem/cpu). Decrementing these counters at zero leads to wrapping which further leads to thepsi_group_cpu->state_maskis being set with the respective pressure state. This could result into the unnecessary time sampling for the pressure state thus cause the spurious psi events. This can further lead to wrong actions being taken at the user land based on these psi events.
Thoughpsi_bugis set under these conditions but that just for debug purpose. Fix it by decrementing the ->tasks count only when it is non-zero.
Makes sense, it's more graceful in the event of a bug.
But what motivates this change? Is it something you hit recently with an upstream kernel and we should investigate?
Quoted 10 lines
Signed-off-by Charan Teja Reddy <[email protected]>--- kernel/sched/psi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)kernel/sched/psi.c +0
-0
kernel/sched/psi.c static void psi_group_change(struct psi_group *group, int cpu, · line 722
Quoted 6 lines
kernel/sched/psi.c +2
-1@@ -718,7 +718,8 @@ static void psi_group_change(struct psi_group *group, int cpu, groupc->tasks[3], clear, set); psi_bug =1 ; } - groupc->tasks[t]--; +if (groupc->tasks[t]) + groupc->tasks[t]--;
There is already a branch on the tasks to signal the bug. How about:
if (groupc->tasksk[t]) {
groupc->tasks[t]--;
} else if (!psi_bug) {
printk_deferred(...
psi_bug = 1;
}Thanks Johannes!!
Quoted 4 lines · Johannes Weiner
Makes sense, it's more graceful in the event of a bug.
But what motivates this change? Is it something you hit recently with an upstream kernel and we should investigate?
We specifically didn't hit the issue around this change. Identified this while doing code walk through.
Quoted 9 lines
There is already a branch on the tasks to signal the bug. How about:
if (groupc->tasksk[t]) { groupc->tasks[t]--; } else if (!psi_bug) { printk_deferred(... psi_bug = 1; }
Looks cleaner. Will raise V2 with this.
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project