[PATCH 0/2] Forced idle time accounting per cpu

[PATCH 0/2] Forced idle time accounting per cpu

[Cruz Zhao]

Josh Don's patch 4feee7d12603 ("sched/core: Forced idle accounting")
provides one means to measure the cost of enabling core scheduling
from the perspective of the task, and this patchset provides another
means to do that from the perspective of the cpu.

Forced idle can be divided into two types, forced idle with cookie'd task
running on it SMT sibling, and forced idle with uncookie'd task running
on it SMT sibling, which should be accounting to measure the cost of
enabling core scheduling too. This patchset accounts both and the sum
of both, which are displayed via /proc/stat.

Cruz Zhao (2):
  sched/core: Cookied forceidle accounting per cpu
  sched/core: Uncookied force idle accounting per cpu

 fs/proc/stat.c              | 26 ++++++++++++++++++++++++++
 include/linux/kernel_stat.h |  4 ++++
 kernel/sched/core.c         |  7 +++----
 kernel/sched/core_sched.c   | 21 +++++++++++++++++++--
 kernel/sched/sched.h        | 10 ++--------
 5 files changed, 54 insertions(+), 14 deletions(-)

base commit: 2850c2311ef4bf30ae8dd8927f0f66b026ff08fb
-- 
1.8.3.1

[PATCH 1/2] sched/core: Cookied forceidle accounting per cpu

[Cruz Zhao]

Adds accounting for "forced idle" time per cpu, which is time where a
cpu is forced to idle by a cookie'd task running on its SMT sibling.

Josh Don's patch 4feee7d12603 ("sched/core: Forced idle accounting")
provides one means to measure the cost of enabling core scheduling
from the perspective of the task, and this patch provides another
means to do that from the perspective of the cpu.

A few details:
 - Cookied forceidle time is displayed via the last column of
   /proc/stat.
 - Cookied forceidle time is ony accounted when this cpu is forced
   idle and a sibling hyperthread is running with a cookie'd task.

Signed-off-by: Cruz Zhao <CruzZhao@linux.alibaba.com>
---
 fs/proc/stat.c              | 15 +++++++++++++++
 include/linux/kernel_stat.h |  3 +++
 kernel/sched/core.c         |  4 ++--
 kernel/sched/core_sched.c   | 20 ++++++++++++++++++--
 kernel/sched/sched.h        | 10 ++--------
 5 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/fs/proc/stat.c b/fs/proc/stat.c
index 4fb8729..3a2fbc9 100644
--- a/fs/proc/stat.c
+++ b/fs/proc/stat.c
@@ -109,6 +109,9 @@ static int show_stat(struct seq_file *p, void *v)
 {
 	int i, j;
 	u64 user, nice, system, idle, iowait, irq, softirq, steal;
+#ifdef CONFIG_SCHED_CORE
+	u64 cookied_forceidle = 0;
+#endif
 	u64 guest, guest_nice;
 	u64 sum = 0;
 	u64 sum_softirq = 0;
@@ -140,6 +143,9 @@ static int show_stat(struct seq_file *p, void *v)
 		guest_nice	+= cpustat[CPUTIME_GUEST_NICE];
 		sum		+= kstat_cpu_irqs_sum(i);
 		sum		+= arch_irq_stat_cpu(i);
+#ifdef CONFIG_SCHED_CORE
+		cookied_forceidle	+= cpustat[CPUTIME_COOKIED_FORCEIDLE];
+#endif
 
 		for (j = 0; j < NR_SOFTIRQS; j++) {
 			unsigned int softirq_stat = kstat_softirqs_cpu(j, i);
@@ -160,6 +166,9 @@ static int show_stat(struct seq_file *p, void *v)
 	seq_put_decimal_ull(p, " ", nsec_to_clock_t(steal));
 	seq_put_decimal_ull(p, " ", nsec_to_clock_t(guest));
 	seq_put_decimal_ull(p, " ", nsec_to_clock_t(guest_nice));
+#ifdef CONFIG_SCHED_CORE
+	seq_put_decimal_ull(p, " ", nsec_to_clock_t(cookied_forceidle));
+#endif
 	seq_putc(p, '\n');
 
 	for_each_online_cpu(i) {
@@ -179,6 +188,9 @@ static int show_stat(struct seq_file *p, void *v)
 		steal		= cpustat[CPUTIME_STEAL];
 		guest		= cpustat[CPUTIME_GUEST];
 		guest_nice	= cpustat[CPUTIME_GUEST_NICE];
+#ifdef CONFIG_SCHED_CORE
+		cookied_forceidle	= cpustat[CPUTIME_COOKIED_FORCEIDLE];
+#endif
 		seq_printf(p, "cpu%d", i);
 		seq_put_decimal_ull(p, " ", nsec_to_clock_t(user));
 		seq_put_decimal_ull(p, " ", nsec_to_clock_t(nice));
@@ -190,6 +202,9 @@ static int show_stat(struct seq_file *p, void *v)
 		seq_put_decimal_ull(p, " ", nsec_to_clock_t(steal));
 		seq_put_decimal_ull(p, " ", nsec_to_clock_t(guest));
 		seq_put_decimal_ull(p, " ", nsec_to_clock_t(guest_nice));
+#ifdef CONFIG_SCHED_CORE
+		seq_put_decimal_ull(p, " ", nsec_to_clock_t(cookied_forceidle));
+#endif
 		seq_putc(p, '\n');
 	}
 	seq_put_decimal_ull(p, "intr ", (unsigned long long)sum);
diff --git a/include/linux/kernel_stat.h b/include/linux/kernel_stat.h
index 69ae6b2..a21b065 100644
--- a/include/linux/kernel_stat.h
+++ b/include/linux/kernel_stat.h
@@ -28,6 +28,9 @@ enum cpu_usage_stat {
 	CPUTIME_STEAL,
 	CPUTIME_GUEST,
 	CPUTIME_GUEST_NICE,
+#ifdef CONFIG_SCHED_CORE
+	CPUTIME_COOKIED_FORCEIDLE,
+#endif
 	NR_STATS,
 };
 
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 956d699..f4f4b24 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5723,7 +5723,6 @@ static inline struct task_struct *pick_task(struct rq *rq)
 	need_sync = !!rq->core->core_cookie;
 
 	/* reset state */
-	rq->core->core_cookie = 0UL;
 	if (rq->core->core_forceidle_count) {
 		if (!core_clock_updated) {
 			update_rq_clock(rq->core);
@@ -5737,6 +5736,7 @@ static inline struct task_struct *pick_task(struct rq *rq)
 		need_sync = true;
 		fi_before = true;
 	}
+	rq->core->core_cookie = 0UL;
 
 	/*
 	 * core->core_task_seq, core->core_pick_seq, rq->core_sched_seq
@@ -5821,7 +5821,7 @@ static inline struct task_struct *pick_task(struct rq *rq)
 		}
 	}
 
-	if (schedstat_enabled() && rq->core->core_forceidle_count) {
+	if (rq->core->core_forceidle_count) {
 		if (cookie)
 			rq->core->core_forceidle_start = rq_clock(rq->core);
 		rq->core->core_forceidle_occupation = occ;
diff --git a/kernel/sched/core_sched.c b/kernel/sched/core_sched.c
index 1fb4567..bc5f45f 100644
--- a/kernel/sched/core_sched.c
+++ b/kernel/sched/core_sched.c
@@ -239,13 +239,14 @@ int sched_core_share_pid(unsigned int cmd, pid_t pid, enum pid_type type,
 #ifdef CONFIG_SCHEDSTATS
 
 /* REQUIRES: rq->core's clock recently updated. */
-void __sched_core_account_forceidle(struct rq *rq)
+void sched_core_account_forceidle(struct rq *rq)
 {
 	const struct cpumask *smt_mask = cpu_smt_mask(cpu_of(rq));
 	u64 delta, now = rq_clock(rq->core);
 	struct rq *rq_i;
 	struct task_struct *p;
 	int i;
+	u64 *cpustat;
 
 	lockdep_assert_rq_held(rq);
 
@@ -260,6 +261,21 @@ void __sched_core_account_forceidle(struct rq *rq)
 
 	rq->core->core_forceidle_start = now;
 
+	for_each_cpu(i, smt_mask) {
+		rq_i = cpu_rq(i);
+		p = rq_i->core_pick ?: rq_i->curr;
+
+		if (!rq->core->core_cookie)
+			continue;
+		if (p == rq_i->idle && rq_i->nr_running) {
+			cpustat = kcpustat_cpu(i).cpustat;
+			cpustat[CPUTIME_COOKIED_FORCEIDLE] += delta;
+		}
+	}
+
+	if (!schedstat_enabled())
+		return;
+
 	if (WARN_ON_ONCE(!rq->core->core_forceidle_occupation)) {
 		/* can't be forced idle without a running task */
 	} else if (rq->core->core_forceidle_count > 1 ||
@@ -292,7 +308,7 @@ void __sched_core_tick(struct rq *rq)
 	if (rq != rq->core)
 		update_rq_clock(rq->core);
 
-	__sched_core_account_forceidle(rq);
+	sched_core_account_forceidle(rq);
 }
 
 #endif /* CONFIG_SCHEDSTATS */
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index de53be9..09cb1f2 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -1858,19 +1858,13 @@ static inline void flush_smp_call_function_from_idle(void) { }
 
 #if defined(CONFIG_SCHED_CORE) && defined(CONFIG_SCHEDSTATS)
 
-extern void __sched_core_account_forceidle(struct rq *rq);
-
-static inline void sched_core_account_forceidle(struct rq *rq)
-{
-	if (schedstat_enabled())
-		__sched_core_account_forceidle(rq);
-}
+extern void sched_core_account_forceidle(struct rq *rq);
 
 extern void __sched_core_tick(struct rq *rq);
 
 static inline void sched_core_tick(struct rq *rq)
 {
-	if (sched_core_enabled(rq) && schedstat_enabled())
+	if (sched_core_enabled(rq))
 		__sched_core_tick(rq);
 }
 
-- 
1.8.3.1

[PATCH 2/2] sched/core: Uncookied force idle accounting per cpu

[Cruz Zhao]

Forced idle can be divided into two types, forced idle with cookie'd task
running on it SMT sibling, and forced idle with uncookie'd task running
on it SMT sibling, which should be accounting to measure the cost of
enabling core scheduling too.

This patch accounts the forced idle time with uncookie'd task, and the
sum of both.

A few details:
 - Uncookied forceidle time and total forceidle time is displayed via
   the last two columns of /proc/stat.
 - Uncookied forceidle time is ony accounted when this cpu is forced
   idle and a sibling hyperthread is running with an uncookie'd task.

Signed-off-by: Cruz Zhao <CruzZhao@linux.alibaba.com>
---
 fs/proc/stat.c              | 13 ++++++++++++-
 include/linux/kernel_stat.h |  1 +
 kernel/sched/core.c         |  3 +--
 kernel/sched/core_sched.c   |  7 ++++---
 4 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/fs/proc/stat.c b/fs/proc/stat.c
index 3a2fbc9..21607cf 100644
--- a/fs/proc/stat.c
+++ b/fs/proc/stat.c
@@ -110,7 +110,7 @@ static int show_stat(struct seq_file *p, void *v)
 	int i, j;
 	u64 user, nice, system, idle, iowait, irq, softirq, steal;
 #ifdef CONFIG_SCHED_CORE
-	u64 cookied_forceidle = 0;
+	u64 cookied_forceidle, uncookied_forceidle, forceidle;
 #endif
 	u64 guest, guest_nice;
 	u64 sum = 0;
@@ -121,6 +121,9 @@ static int show_stat(struct seq_file *p, void *v)
 	user = nice = system = idle = iowait =
 		irq = softirq = steal = 0;
 	guest = guest_nice = 0;
+#ifdef CONFIG_SCHED_CORE
+	cookied_forceidle = uncookied_forceidle = forceidle = 0;
+#endif
 	getboottime64(&boottime);
 	/* shift boot timestamp according to the timens offset */
 	timens_sub_boottime(&boottime);
@@ -145,6 +148,8 @@ static int show_stat(struct seq_file *p, void *v)
 		sum		+= arch_irq_stat_cpu(i);
 #ifdef CONFIG_SCHED_CORE
 		cookied_forceidle	+= cpustat[CPUTIME_COOKIED_FORCEIDLE];
+		uncookied_forceidle	+= cpustat[CPUTIME_UNCOOKIED_FORCEIDLE];
+		forceidle		= cookied_forceidle + uncookied_forceidle;
 #endif
 
 		for (j = 0; j < NR_SOFTIRQS; j++) {
@@ -168,6 +173,8 @@ static int show_stat(struct seq_file *p, void *v)
 	seq_put_decimal_ull(p, " ", nsec_to_clock_t(guest_nice));
 #ifdef CONFIG_SCHED_CORE
 	seq_put_decimal_ull(p, " ", nsec_to_clock_t(cookied_forceidle));
+	seq_put_decimal_ull(p, " ", nsec_to_clock_t(uncookied_forceidle));
+	seq_put_decimal_ull(p, " ", nsec_to_clock_t(forceidle));
 #endif
 	seq_putc(p, '\n');
 
@@ -190,6 +197,8 @@ static int show_stat(struct seq_file *p, void *v)
 		guest_nice	= cpustat[CPUTIME_GUEST_NICE];
 #ifdef CONFIG_SCHED_CORE
 		cookied_forceidle	= cpustat[CPUTIME_COOKIED_FORCEIDLE];
+		uncookied_forceidle	= cpustat[CPUTIME_UNCOOKIED_FORCEIDLE];
+		forceidle		= cookied_forceidle + uncookied_forceidle;
 #endif
 		seq_printf(p, "cpu%d", i);
 		seq_put_decimal_ull(p, " ", nsec_to_clock_t(user));
@@ -204,6 +213,8 @@ static int show_stat(struct seq_file *p, void *v)
 		seq_put_decimal_ull(p, " ", nsec_to_clock_t(guest_nice));
 #ifdef CONFIG_SCHED_CORE
 		seq_put_decimal_ull(p, " ", nsec_to_clock_t(cookied_forceidle));
+		seq_put_decimal_ull(p, " ", nsec_to_clock_t(uncookied_forceidle));
+		seq_put_decimal_ull(p, " ", nsec_to_clock_t(forceidle));
 #endif
 		seq_putc(p, '\n');
 	}
diff --git a/include/linux/kernel_stat.h b/include/linux/kernel_stat.h
index a21b065..23945c1 100644
--- a/include/linux/kernel_stat.h
+++ b/include/linux/kernel_stat.h
@@ -30,6 +30,7 @@ enum cpu_usage_stat {
 	CPUTIME_GUEST_NICE,
 #ifdef CONFIG_SCHED_CORE
 	CPUTIME_COOKIED_FORCEIDLE,
+	CPUTIME_UNCOOKIED_FORCEIDLE,
 #endif
 	NR_STATS,
 };
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index f4f4b24..16d937e4 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5822,8 +5822,7 @@ static inline struct task_struct *pick_task(struct rq *rq)
 	}
 
 	if (rq->core->core_forceidle_count) {
-		if (cookie)
-			rq->core->core_forceidle_start = rq_clock(rq->core);
+		rq->core->core_forceidle_start = rq_clock(rq->core);
 		rq->core->core_forceidle_occupation = occ;
 	}
 
diff --git a/kernel/sched/core_sched.c b/kernel/sched/core_sched.c
index bc5f45f..89bd49d 100644
--- a/kernel/sched/core_sched.c
+++ b/kernel/sched/core_sched.c
@@ -265,11 +265,12 @@ void sched_core_account_forceidle(struct rq *rq)
 		rq_i = cpu_rq(i);
 		p = rq_i->core_pick ?: rq_i->curr;
 
-		if (!rq->core->core_cookie)
-			continue;
 		if (p == rq_i->idle && rq_i->nr_running) {
 			cpustat = kcpustat_cpu(i).cpustat;
-			cpustat[CPUTIME_COOKIED_FORCEIDLE] += delta;
+			if (rq->core->core_cookie)
+				cpustat[CPUTIME_COOKIED_FORCEIDLE] += delta;
+			else
+				cpustat[CPUTIME_UNCOOKIED_FORCEIDLE] += delta;
 		}
 	}
 
-- 
1.8.3.1

Re: [PATCH 0/2] Forced idle time accounting per cpu

[cruzzhao]

Here's also a problem confused me: how to account the uncookie'd forced
idle from the perspective of task. A feasible method is to divide the
uncookie'd forced idle time by the number of tasks in the core_tree, and
add the result to these tasks, but it will cost a lot on traversing the
core_tree.

在 2021/12/23 下午8:30, Cruz Zhao 写道:
> Josh Don's patch 4feee7d12603 ("sched/core: Forced idle accounting")
> provides one means to measure the cost of enabling core scheduling
> from the perspective of the task, and this patchset provides another
> means to do that from the perspective of the cpu.
> 
> Forced idle can be divided into two types, forced idle with cookie'd task
> running on it SMT sibling, and forced idle with uncookie'd task running
> on it SMT sibling, which should be accounting to measure the cost of
> enabling core scheduling too. This patchset accounts both and the sum
> of both, which are displayed via /proc/stat.
> 
> Cruz Zhao (2):
>   sched/core: Cookied forceidle accounting per cpu
>   sched/core: Uncookied force idle accounting per cpu
> 
>  fs/proc/stat.c              | 26 ++++++++++++++++++++++++++
>  include/linux/kernel_stat.h |  4 ++++
>  kernel/sched/core.c         |  7 +++----
>  kernel/sched/core_sched.c   | 21 +++++++++++++++++++--
>  kernel/sched/sched.h        | 10 ++--------
>  5 files changed, 54 insertions(+), 14 deletions(-)
> 
> base commit: 2850c2311ef4bf30ae8dd8927f0f66b026ff08fb
> 

Re: [PATCH 0/2] Forced idle time accounting per cpu

[cruzzhao]

Ping.
Accounting forced idle time for per cpu can help us measure the cost of
enabling core scheduling from a global perspective. Mind having a look
at it?
在 2021/12/23 下午8:30, Cruz Zhao 写道:
> Josh Don's patch 4feee7d12603 ("sched/core: Forced idle accounting")
> provides one means to measure the cost of enabling core scheduling
> from the perspective of the task, and this patchset provides another
> means to do that from the perspective of the cpu.
> 
> Forced idle can be divided into two types, forced idle with cookie'd task
> running on it SMT sibling, and forced idle with uncookie'd task running
> on it SMT sibling, which should be accounting to measure the cost of
> enabling core scheduling too. This patchset accounts both and the sum
> of both, which are displayed via /proc/stat.
> 
> Cruz Zhao (2):
>   sched/core: Cookied forceidle accounting per cpu
>   sched/core: Uncookied force idle accounting per cpu
> 
>  fs/proc/stat.c              | 26 ++++++++++++++++++++++++++
>  include/linux/kernel_stat.h |  4 ++++
>  kernel/sched/core.c         |  7 +++----
>  kernel/sched/core_sched.c   | 21 +++++++++++++++++++--
>  kernel/sched/sched.h        | 10 ++--------
>  5 files changed, 54 insertions(+), 14 deletions(-)
> 
> base commit: 2850c2311ef4bf30ae8dd8927f0f66b026ff08fb

Re: [PATCH 0/2] Forced idle time accounting per cpu

[Josh Don]

On Mon, Jan 3, 2022 at 11:15 PM cruzzhao <cruzzhao@linux.alibaba.com> wrote:
>
> Ping.
> Accounting forced idle time for per cpu can help us measure the cost of
> enabling core scheduling from a global perspective. Mind having a look
> at it?

Sorry, just got back from vacation. I'll have a look at the patches
later today, but the idea sounds reasonable.

Best,
Josh

Re: [PATCH 1/2] sched/core: Cookied forceidle accounting per cpu

[Josh Don]

Hi Cruz,

Could you add a bit more background to help me understand what case
this patch solves? Is your issue that you want to be able to
attribute forced idle time to the specific cpus it happens on, or do
you simply want a more convenient way of summing forced idle without
iterating your cookie'd tasks and summing the schedstat manually?

> @@ -190,6 +202,9 @@ static int show_stat(struct seq_file *p, void *v)
>                 seq_put_decimal_ull(p, " ", nsec_to_clock_t(steal));
>                 seq_put_decimal_ull(p, " ", nsec_to_clock_t(guest));
>                 seq_put_decimal_ull(p, " ", nsec_to_clock_t(guest_nice));
> +#ifdef CONFIG_SCHED_CORE
> +               seq_put_decimal_ull(p, " ", nsec_to_clock_t(cookied_forceidle));
> +#endif

IMO it would be better to always print this stat, otherwise it sets a
weird precedent for new stats added in the future (much more difficult
for userspace to reason about which column corresponds with which
field, since it would depend on kernel config).

Also, did you intend to put this in /proc/stat instead of
/proc/schedstat (the latter of which would be more attractive to
prevent calculation of these stats unless schestat was enabled)?

> diff --git a/kernel/sched/core_sched.c b/kernel/sched/core_sched.c
> @@ -260,6 +261,21 @@ void __sched_core_account_forceidle(struct rq *rq)
>
>         rq->core->core_forceidle_start = now;
>
> +       for_each_cpu(i, smt_mask) {
> +               rq_i = cpu_rq(i);
> +               p = rq_i->core_pick ?: rq_i->curr;
> +
> +               if (!rq->core->core_cookie)
> +                       continue;

I see this is temporary given your other patch, but just a note that
if your other patch is dropped, this check can be pulled outside the
loop.

> +               if (p == rq_i->idle && rq_i->nr_running) {
> +                       cpustat = kcpustat_cpu(i).cpustat;
> +                       cpustat[CPUTIME_COOKIED_FORCEIDLE] += delta;
> +               }
> +       }

I don't think this is right. If a cpu was simply idle while some other
SMT sibling on its core was forced idle, and then a task happens to
wake on the idle cpu, that cpu will now be charged the full delta here
as forced idle (when actually it was never forced idle, we just
haven't been through pick_next_task yet). One workaround would be to
add a boolean to struct rq to cache whether the rq was in forced idle
state.

Best,
Josh

Re: [PATCH 2/2] sched/core: Uncookied force idle accounting per cpu

[Josh Don]

Hi Cruz,

On Thu, Dec 23, 2021 at 4:30 AM Cruz Zhao <CruzZhao@linux.alibaba.com> wrote:
>
> Forced idle can be divided into two types, forced idle with cookie'd task
> running on it SMT sibling, and forced idle with uncookie'd task running
> on it SMT sibling, which should be accounting to measure the cost of
> enabling core scheduling too.
>
> This patch accounts the forced idle time with uncookie'd task, and the
> sum of both.
>
> A few details:
>  - Uncookied forceidle time and total forceidle time is displayed via
>    the last two columns of /proc/stat.
>  - Uncookied forceidle time is ony accounted when this cpu is forced
>    idle and a sibling hyperthread is running with an uncookie'd task.

What is the purpose/use-case to account the forced idle from
uncookie'd tasks? The forced-idle from cookie'd tasks represents
capacity loss due to adding in some cookie'd tasks. If forced idle is
high, that can be rectified by making some changes to the cookie'd
tasks (ie. their affinity, cpu budget, etc.).

Re: [PATCH 1/2] sched/core: Cookied forceidle accounting per cpu

[cruzzhao]

在 2022/1/5 上午9:48, Josh Don 写道:
> Hi Cruz,
> 
Firstly, attributing forced idle time to the specific cpus it happens on
can help us measure the effect of steal_cookie_task(). We found out that
steal_cookie_task() conflicts with load balance sometimes, for example,
a cookie'd task is stolen by steal_cookie_task(), but it'll be migrated
to another core by load balance soon. Secondly, a more convenient way of
summing forced idle instead of iterating cookie'd task is indeed what we
need. In the multi-rent scenario, it'll be complex to maintain the list
of cookie'd task and it'll cost a lot to iterate it.

> Could you add a bit more background to help me understand what case
> this patch solves? Is your issue that you want to be able to
> attribute forced idle time to the specific cpus it happens on, or do
> you simply want a more convenient way of summing forced idle without
> iterating your cookie'd tasks and summing the schedstat manually?
> 
>> @@ -190,6 +202,9 @@ static int show_stat(struct seq_file *p, void *v)
>>                 seq_put_decimal_ull(p, " ", nsec_to_clock_t(steal));
>>                 seq_put_decimal_ull(p, " ", nsec_to_clock_t(guest));
>>                 seq_put_decimal_ull(p, " ", nsec_to_clock_t(guest_nice));
>> +#ifdef CONFIG_SCHED_CORE
>> +               seq_put_decimal_ull(p, " ", nsec_to_clock_t(cookied_forceidle));
>> +#endif
> 
I'll put this in /proc/schedstat and fix the problem that accounting
simply idle as force idle in the next version.

Many thanks for suggestions.

Best,
Cruz Zhao
> IMO it would be better to always print this stat, otherwise it sets a
> weird precedent for new stats added in the future (much more difficult
> for userspace to reason about which column corresponds with which
> field, since it would depend on kernel config).
> 
> Also, did you intend to put this in /proc/stat instead of
> /proc/schedstat (the latter of which would be more attractive to
> prevent calculation of these stats unless schestat was enabled)?
> 
>> diff --git a/kernel/sched/core_sched.c b/kernel/sched/core_sched.c
>> @@ -260,6 +261,21 @@ void __sched_core_account_forceidle(struct rq *rq)
>>
>>         rq->core->core_forceidle_start = now;
>>
>> +       for_each_cpu(i, smt_mask) {
>> +               rq_i = cpu_rq(i);
>> +               p = rq_i->core_pick ?: rq_i->curr;
>> +
>> +               if (!rq->core->core_cookie)
>> +                       continue;
> 
> I see this is temporary given your other patch, but just a note that
> if your other patch is dropped, this check can be pulled outside the
> loop.
> 
>> +               if (p == rq_i->idle && rq_i->nr_running) {
>> +                       cpustat = kcpustat_cpu(i).cpustat;
>> +                       cpustat[CPUTIME_COOKIED_FORCEIDLE] += delta;
>> +               }
>> +       }
> 
> I don't think this is right. If a cpu was simply idle while some other
> SMT sibling on its core was forced idle, and then a task happens to
> wake on the idle cpu, that cpu will now be charged the full delta here
> as forced idle (when actually it was never forced idle, we just
> haven't been through pick_next_task yet). One workaround would be to
> add a boolean to struct rq to cache whether the rq was in forced idle
> state.
> 
> Best,
> Josh

Re: [PATCH 2/2] sched/core: Uncookied force idle accounting per cpu

[cruzzhao]

在 2022/1/5 上午9:56, Josh Don 写道:
> Hi Cruz,
> 
> On Thu, Dec 23, 2021 at 4:30 AM Cruz Zhao <CruzZhao@linux.alibaba.com> wrote:
>>
>> Forced idle can be divided into two types, forced idle with cookie'd task
>> running on it SMT sibling, and forced idle with uncookie'd task running
>> on it SMT sibling, which should be accounting to measure the cost of
>> enabling core scheduling too.
>>
>> This patch accounts the forced idle time with uncookie'd task, and the
>> sum of both.
>>
>> A few details:
>>  - Uncookied forceidle time and total forceidle time is displayed via
>>    the last two columns of /proc/stat.
>>  - Uncookied forceidle time is ony accounted when this cpu is forced
>>    idle and a sibling hyperthread is running with an uncookie'd task.
> 
When we care about capacity loss, we care about all but not some of it.
The forced idle time from uncookie'd task is actually caused by the
cookie'd task in runqueue indirectly, and it's more accurate to measure
the capacity loss with the sum of cookie'd forced idle time and
uncookie'd forced idle time, as far as I'm concerned.

Assuming cpu x and cpu y are a pair of smt siblings, consider the
following scenarios:
1. There's a cookie'd task A running on cpu x, and there're 4 uncookie'd
tasks B~E running on cpu y. For cpu x, there will be 80% forced idle
time(from uncookie'd task); for cpu y, there will be 20% forced idle
time(from cookie'd task).
2. There's a uncookie'd task A running on cpu x, and there're 4 cookie'd
tasks B~E running on cpu y. For cpu x, there will be 80% forced idle
time(from cookie'd task); for cpu y, there will be 20% forced idle
time(from uncookie'd task).
The scenario1 can recurrent by stress-ng(scenario2 can recurrent similary):
(cookie'd)taskset -c x stress-ng -c 1 -l 100
(uncookie'd)taskset -c y stress-ng -c 4 -l 100

In the above two scenarios, the capacity loss is 1 cpu, but in
scenario1, the cookie'd forced idle time tells us 20%cpu loss, in
scenario2, the cookie'd forced idle time tells us 80% forced idle time,
which are not accurate. It'll be more accurate with the sum of cookie'd
forced idle time and uncookie'd forced idle time.

Best,
Cruz Zhao

> What is the purpose/use-case to account the forced idle from
> uncookie'd tasks? The forced-idle from cookie'd tasks represents
> capacity loss due to adding in some cookie'd tasks. If forced idle is
> high, that can be rectified by making some changes to the cookie'd
> tasks (ie. their affinity, cpu budget, etc.).

Re: [PATCH 1/2] sched/core: Cookied forceidle accounting per cpu

[Josh Don]

On Wed, Jan 5, 2022 at 3:33 AM cruzzhao <cruzzhao@linux.alibaba.com> wrote:
>
> Firstly, attributing forced idle time to the specific cpus it happens on
> can help us measure the effect of steal_cookie_task(). We found out that
> steal_cookie_task() conflicts with load balance sometimes, for example,
> a cookie'd task is stolen by steal_cookie_task(), but it'll be migrated
> to another core by load balance soon.

I don't see how this is very helpful for steal_cookie_task(), since it
isn't a targeted metric for that specific case. If you were interested
in that specifically, I'd think you'd want to look at more direct
metrics, such as task migration counts, or adding some
accounting/histogram for the time between steal and load balance away.

> Secondly, a more convenient way of
> summing forced idle instead of iterating cookie'd task is indeed what we
> need. In the multi-rent scenario, it'll be complex to maintain the list
> of cookie'd task and it'll cost a lot to iterate it.

That motivation makes more sense to me. Have you considered
accumulating this at the cgroup level (ie. attributing it as another
type of usage)?

Re: [PATCH 2/2] sched/core: Uncookied force idle accounting per cpu

[Josh Don]

On Wed, Jan 5, 2022 at 3:33 AM cruzzhao <cruzzhao@linux.alibaba.com> wrote:
>
> When we care about capacity loss, we care about all but not some of it.
> The forced idle time from uncookie'd task is actually caused by the
> cookie'd task in runqueue indirectly, and it's more accurate to measure
> the capacity loss with the sum of cookie'd forced idle time and
> uncookie'd forced idle time, as far as I'm concerned.
>
> Assuming cpu x and cpu y are a pair of smt siblings, consider the
> following scenarios:
> 1. There's a cookie'd task A running on cpu x, and there're 4 uncookie'd
> tasks B~E running on cpu y. For cpu x, there will be 80% forced idle
> time(from uncookie'd task); for cpu y, there will be 20% forced idle
> time(from cookie'd task).
> 2. There's a uncookie'd task A running on cpu x, and there're 4 cookie'd
> tasks B~E running on cpu y. For cpu x, there will be 80% forced idle
> time(from cookie'd task); for cpu y, there will be 20% forced idle
> time(from uncookie'd task).
> The scenario1 can recurrent by stress-ng(scenario2 can recurrent similary):
> (cookie'd)taskset -c x stress-ng -c 1 -l 100
> (uncookie'd)taskset -c y stress-ng -c 4 -l 100
>
> In the above two scenarios, the capacity loss is 1 cpu, but in
> scenario1, the cookie'd forced idle time tells us 20%cpu loss, in
> scenario2, the cookie'd forced idle time tells us 80% forced idle time,
> which are not accurate. It'll be more accurate with the sum of cookie'd
> forced idle time and uncookie'd forced idle time.

Why do you need this separated out into two fields then? Could we just
combine the uncookie'd and cookie'd forced idle into a single sum?

IMO it is fine to account the forced idle from uncookie'd tasks, but
we should then also change the task accounting to do the same, for
consistency.

Re: [PATCH 2/2] sched/core: Uncookied force idle accounting per cpu

[cruzzhao]

在 2022/1/6 上午4:59, Josh Don 写道:

It's a good idea to combine them into a single sum. I separated them in
order to be consistent with the task accounting and for easy to understand.
As for change the task accounting, I've tried but I haven't found a
proper method to do so. I've considered the following methods:
1. Account the uncookie'd force idle time to the uncookie'd task, but
it'll be hard to trace the uncookie'd task.
2. Account the uncookie'd force idle time to the cookie'd task in the
core_tree of the core, but it will cost a lot on traversing the core_tree.

Many thanks for suggestions.
Best,
Cruz Zhao

> Why do you need this separated out into two fields then? Could we just
> combine the uncookie'd and cookie'd forced idle into a single sum?
> 
> IMO it is fine to account the forced idle from uncookie'd tasks, but
> we should then also change the task accounting to do the same, for
> consistency.

Re: [PATCH 1/2] sched/core: Cookied forceidle accounting per cpu

[cruzzhao]

在 2022/1/6 上午4:47, Josh Don 写道:
> On Wed, Jan 5, 2022 at 3:33 AM cruzzhao <cruzzhao@linux.alibaba.com> wrote:

> 
> I don't see how this is very helpful for steal_cookie_task(), since it
> isn't a targeted metric for that specific case. If you were interested
> in that specifically, I'd think you'd want to look at more direct
> metrics, such as task migration counts, or adding some
> accounting/histogram for the time between steal and load balance away.
> 

I've already read the patch "sched: CGroup tagging interface for core
scheduling", but it hasn't been merged into linux-next. IMO it's better
to do this at the cgroup level after the cgroup tagging interface is
introduced.

Best,
Cruz Zhao

> 
> That motivation makes more sense to me. Have you considered
> accumulating this at the cgroup level (ie. attributing it as another
> type of usage)?

Re: [PATCH 1/2] sched/core: Cookied forceidle accounting per cpu

[Josh Don]

On Thu, Jan 6, 2022 at 4:10 AM cruzzhao <cruzzhao@linux.alibaba.com> wrote:
>
> >
> > That motivation makes more sense to me. Have you considered
> > accumulating this at the cgroup level (ie. attributing it as another
> > type of usage)?
>
> I've already read the patch "sched: CGroup tagging interface for core
> scheduling", but it hasn't been merged into linux-next. IMO it's better
> to do this at the cgroup level after the cgroup tagging interface is
> introduced.
>
> Best,
> Cruz Zhao

There are no plans to introduce cgroup-level tagging for core sched.
But the accounting is a separate issue. Similar to how tasks account
usage both to themselves and to their cgroup hierarchy, we could
account forced idle in a similar way, and add another field to
cpu_extra_stat_show. That still gives you the total system forced idle
time by looking at the root cgroup, and allows you to slice the
accounting by different job groups. It also makes the accounting a
single value per cgroup rather than a per-cpu value (I still don't see
the value of attributing to specific cpus, as I described in my prior
reply).

Re: [PATCH 2/2] sched/core: Uncookied force idle accounting per cpu

[Josh Don]

On Thu, Jan 6, 2022 at 4:05 AM cruzzhao <cruzzhao@linux.alibaba.com> wrote:
>
>
> 在 2022/1/6 上午4:59, Josh Don 写道:
>
> It's a good idea to combine them into a single sum. I separated them in
> order to be consistent with the task accounting and for easy to understand.
> As for change the task accounting, I've tried but I haven't found a
> proper method to do so. I've considered the following methods:
> 1. Account the uncookie'd force idle time to the uncookie'd task, but
> it'll be hard to trace the uncookie'd task.

Not sure what you mean there, I think you just need to add

--- a/kernel/sched/core_sched.c
+++ b/kernel/sched/core_sched.c
@@ -294,7 +294,7 @@ void sched_core_account_forceidle(struct rq *rq)
                rq_i = cpu_rq(i);
                p = rq_i->core_pick ?: rq_i->curr;

-               if (!p->core_cookie)
+               if (p == rq_i->idle)
                        continue;

                __schedstat_add(p->stats.core_forceidle_sum, delta)

> 2. Account the uncookie'd force idle time to the cookie'd task in the
> core_tree of the core, but it will cost a lot on traversing the core_tree.
>
> Many thanks for suggestions.
> Best,
> Cruz Zhao
>
> > Why do you need this separated out into two fields then? Could we just
> > combine the uncookie'd and cookie'd forced idle into a single sum?
> >
> > IMO it is fine to account the forced idle from uncookie'd tasks, but
> > we should then also change the task accounting to do the same, for
> > consistency.