LinuxLists.cc - [PATCH] sched/core: add forced idle accounting for cgroups

2022-05-13 17:57:33

Subject: [PATCH] sched/core: add forced idle accounting for cgroups

4feee7d1260 previously added per-task forced idle accounting. This patch
extends this to also include cgroups.

rstat is used for cgroup accounting, except for the root, which uses
kcpustat in order to bypass the need for doing an rstat flush when
reading root stats.

Only cgroup v2 is supported. Similar to the task accounting, the cgroup
accounting requires that schedstats is enabled.

Signed-off-by: Josh Don <[email protected]>
---
include/linux/kernel_stat.h | 1 +
kernel/sched/core.c | 15 ++++++++-
kernel/sched/core_sched.c | 62 +++++++++++++++++++++++++++++++++++++
kernel/sched/sched.h | 18 +++++++++++
4 files changed, 95 insertions(+), 1 deletion(-)

diff --git a/include/linux/kernel_stat.h b/include/linux/kernel_stat.h
index 69ae6b278464..2e9b3c7d2f18 100644
--- a/include/linux/kernel_stat.h
+++ b/include/linux/kernel_stat.h
@@ -28,6 +28,7 @@ enum cpu_usage_stat {
CPUTIME_STEAL,
CPUTIME_GUEST,
CPUTIME_GUEST_NICE,
+ CPUTIME_FORCEIDLE,
NR_STATS,
};

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 48cfad152b86..a29cb4029818 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -10828,12 +10828,18 @@ static struct cftype cpu_legacy_files[] = {
{ } /* Terminate */
};

+static void cpu_cgroup_css_rstat_flush(struct cgroup_subsys_state *css, int cpu)
+{
+ sched_core_rstat_flush(css_tg(css), cpu);
+}
+
static int cpu_extra_stat_show(struct seq_file *sf,
struct cgroup_subsys_state *css)
{
+ struct task_group __maybe_unused *tg = css_tg(css);
+
#ifdef CONFIG_CFS_BANDWIDTH
{
- struct task_group *tg = css_tg(css);
struct cfs_bandwidth *cfs_b = &tg->cfs_bandwidth;
u64 throttled_usec, burst_usec;

@@ -10851,6 +10857,12 @@ static int cpu_extra_stat_show(struct seq_file *sf,
throttled_usec, cfs_b->nr_burst, burst_usec);
}
#endif
+
+#ifdef CONFIG_SCHED_CORE
+ /* already updated stats via rstat flush */
+ seq_printf(sf, "forceidle_usec %llu\n",
+ sched_core_forceidle_sum(tg) / NSEC_PER_USEC);
+#endif
return 0;
}

@@ -11031,6 +11043,7 @@ struct cgroup_subsys cpu_cgrp_subsys = {
.css_online = cpu_cgroup_css_online,
.css_released = cpu_cgroup_css_released,
.css_free = cpu_cgroup_css_free,
+ .css_rstat_flush = cpu_cgroup_css_rstat_flush,
.css_extra_stat_show = cpu_extra_stat_show,
.fork = cpu_cgroup_fork,
.can_attach = cpu_cgroup_can_attach,
diff --git a/kernel/sched/core_sched.c b/kernel/sched/core_sched.c
index 38a2cec21014..ccfeef6542dc 100644
--- a/kernel/sched/core_sched.c
+++ b/kernel/sched/core_sched.c
@@ -277,7 +277,16 @@ void __sched_core_account_forceidle(struct rq *rq)
if (p == rq_i->idle)
continue;

+ /* thread accounting */
__schedstat_add(p->stats.core_forceidle_sum, delta);
+
+ /* root accounting */
+ kcpustat_cpu(i).cpustat[CPUTIME_FORCEIDLE] += delta;
+
+ /* cgroup accounting */
+#ifdef CONFIG_FAIR_GROUP_SCHED
+ task_group(p)->cfs_rq[i]->forceidle_sum += delta;
+#endif
}
}

@@ -292,4 +301,57 @@ void __sched_core_tick(struct rq *rq)
__sched_core_account_forceidle(rq);
}

+void sched_core_rstat_flush(struct task_group *tg, int cpu)
+{
+ struct cfs_rq *cfs_rq = tg->cfs_rq[cpu];
+ struct task_group *parent = tg->parent;
+ u64 delta, curr_sum;
+
+ /* root uses cpustat */
+ if (!parent)
+ return;
+
+ /*
+ * Note: cgroup_rstat_lock protects cfs_rq->forceidle_sum_prev and
+ * tg->{forceidle_sum, forceidle_sum_pending}.
+ */
+
+ delta = tg->forceidle_sum_pending;
+ if (delta)
+ tg->forceidle_sum_pending = 0;
+
+ /* rq lock not held; value may change concurrently */
+ curr_sum = READ_ONCE(cfs_rq->forceidle_sum);
+ if (curr_sum != cfs_rq->forceidle_sum_prev) {
+ delta += curr_sum - cfs_rq->forceidle_sum_prev;
+ cfs_rq->forceidle_sum_prev = curr_sum;
+ }
+
+ if (!delta)
+ return;
+
+ tg->forceidle_sum += delta;
+ parent->forceidle_sum_pending += delta;
+}
+
+/* REQUIRES: If tg is not root, an rstat flush was recently done. */
+u64 sched_core_forceidle_sum(struct task_group *tg)
+{
+ if (!tg->parent) {
+ u64 sum = 0;
+ int i;
+
+ for_each_possible_cpu(i) {
+ struct kernel_cpustat kcpustat;
+
+ kcpustat_cpu_fetch(&kcpustat, i);
+ sum += kcpustat.cpustat[CPUTIME_FORCEIDLE];
+ }
+
+ return sum;
+ } else {
+ return tg->forceidle_sum;
+ }
+}
+
#endif /* CONFIG_SCHEDSTATS */
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index 7f338c53ce42..36bef97b9e2f 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -425,6 +425,12 @@ struct task_group {
struct uclamp_se uclamp[UCLAMP_CNT];
#endif

+#ifdef CONFIG_SCHED_CORE
+ /* used with rstat */
+ u64 forceidle_sum;
+ u64 forceidle_sum_pending;
+#endif
+
};

#ifdef CONFIG_FAIR_GROUP_SCHED
@@ -526,6 +532,10 @@ struct cfs_rq {
#ifdef CONFIG_SCHED_CORE
unsigned int forceidle_seq;
u64 min_vruntime_fi;
+
+ /* for accounting with rstat */
+ u64 forceidle_sum;
+ u64 forceidle_sum_prev;
#endif

#ifndef CONFIG_64BIT
@@ -1849,12 +1859,20 @@ static inline void sched_core_tick(struct rq *rq)
__sched_core_tick(rq);
}

+extern void sched_core_rstat_flush(struct task_group *tg, int cpu);
+
+extern u64 sched_core_forceidle_sum(struct task_group *tg);
+
#else

static inline void sched_core_account_forceidle(struct rq *rq) {}

static inline void sched_core_tick(struct rq *rq) {}

+static inline void sched_core_rstat_flush(struct task_group *tg, int cpu) {}
+
+static inline u64 sched_core_forceidle_sum(struct task_group *tg) { return 0; }
+
#endif /* CONFIG_SCHED_CORE && CONFIG_SCHEDSTATS */

#ifdef CONFIG_CGROUP_SCHED
--
2.36.0.512.ge40c2bad7a-goog

2022-05-14 01:53:13

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH] sched/core: add forced idle accounting for cgroups

2022-05-14 01:56:42

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH] sched/core: add forced idle accounting for cgroups

Hi Josh,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on tip/sched/core]
[also build test WARNING on tip/master linux/master linus/master v5.18-rc6 next-20220512]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/intel-lab-lkp/linux/commits/Josh-Don/sched-core-add-forced-idle-accounting-for-cgroups/20220513-085621
base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 734387ec2f9d77b00276042b1fa7c95f48ee879d
config: i386-randconfig-m021 (https://download.01.org/0day-ci/archive/20220513/[email protected]/config)
compiler: gcc-11 (Debian 11.2.0-20) 11.2.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/21dca7763777c1985d6f5ac289e6e0c20d429d05
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Josh-Don/sched-core-add-forced-idle-accounting-for-cgroups/20220513-085621
git checkout 21dca7763777c1985d6f5ac289e6e0c20d429d05
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash kernel/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

In file included from kernel/sched/build_utility.c:93:
kernel/sched/core_sched.c: In function 'sched_core_rstat_flush':
kernel/sched/core_sched.c:306:35: error: invalid use of undefined type 'struct task_group'
306 | struct cfs_rq *cfs_rq = tg->cfs_rq[cpu];
| ^~
kernel/sched/core_sched.c:307:39: error: invalid use of undefined type 'struct task_group'
307 | struct task_group *parent = tg->parent;
| ^~
kernel/sched/core_sched.c:319:19: error: invalid use of undefined type 'struct task_group'
319 | delta = tg->forceidle_sum_pending;
| ^~
kernel/sched/core_sched.c:321:19: error: invalid use of undefined type 'struct task_group'
321 | tg->forceidle_sum_pending = 0;
| ^~
kernel/sched/core_sched.c:333:11: error: invalid use of undefined type 'struct task_group'
333 | tg->forceidle_sum += delta;
| ^~
kernel/sched/core_sched.c:334:15: error: invalid use of undefined type 'struct task_group'
334 | parent->forceidle_sum_pending += delta;
| ^~
>> kernel/sched/core_sched.c:304:56: warning: parameter 'cpu' set but not used [-Wunused-but-set-parameter]
304 | void sched_core_rstat_flush(struct task_group *tg, int cpu)
| ~~~~^~~
kernel/sched/core_sched.c: In function 'sched_core_forceidle_sum':
kernel/sched/core_sched.c:340:16: error: invalid use of undefined type 'struct task_group'
340 | if (!tg->parent) {
| ^~
kernel/sched/core_sched.c:353:26: error: invalid use of undefined type 'struct task_group'
353 | return tg->forceidle_sum;
| ^~
kernel/sched/core_sched.c:355:1: error: control reaches end of non-void function [-Werror=return-type]
355 | }
| ^
cc1: some warnings being treated as errors

vim +/cpu +304 kernel/sched/core_sched.c

303
> 304 void sched_core_rstat_flush(struct task_group *tg, int cpu)
305 {
306 struct cfs_rq *cfs_rq = tg->cfs_rq[cpu];
307 struct task_group *parent = tg->parent;
308 u64 delta, curr_sum;
309
310 /* root uses cpustat */
311 if (!parent)
312 return;
313
314 /*
315 * Note: cgroup_rstat_lock protects cfs_rq->forceidle_sum_prev and
316 * tg->{forceidle_sum, forceidle_sum_pending}.
317 */
318
319 delta = tg->forceidle_sum_pending;
320 if (delta)
> 321 tg->forceidle_sum_pending = 0;
322
323 /* rq lock not held; value may change concurrently */
324 curr_sum = READ_ONCE(cfs_rq->forceidle_sum);
325 if (curr_sum != cfs_rq->forceidle_sum_prev) {
326 delta += curr_sum - cfs_rq->forceidle_sum_prev;
327 cfs_rq->forceidle_sum_prev = curr_sum;
328 }
329
330 if (!delta)
331 return;
332
333 tg->forceidle_sum += delta;
334 parent->forceidle_sum_pending += delta;
335 }
336

--
0-DAY CI Kernel Test Service
https://01.org/lkp

2022-05-14 02:00:52

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH] sched/core: add forced idle accounting for cgroups

Hi Josh,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tip/sched/core]
[also build test ERROR on tip/master linux/master linus/master v5.18-rc6 next-20220512]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url: https://github.com/intel-lab-lkp/linux/commits/Josh-Don/sched-core-add-forced-idle-accounting-for-cgroups/20220513-085621
base: https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 734387ec2f9d77b00276042b1fa7c95f48ee879d
config: s390-randconfig-r003-20220512 (https://download.01.org/0day-ci/archive/20220513/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 38189438b69ca27b4c6ce707c52dbd217583d046)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install s390 cross compiling tool for clang build
# apt-get install binutils-s390x-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/21dca7763777c1985d6f5ac289e6e0c20d429d05
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Josh-Don/sched-core-add-forced-idle-accounting-for-cgroups/20220513-085621
git checkout 21dca7763777c1985d6f5ac289e6e0c20d429d05
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All errors (new ones prefixed by >>):

In file included from kernel/sched/build_utility.c:15:
In file included from include/linux/sched/isolation.h:6:
In file included from include/linux/tick.h:8:
In file included from include/linux/clockchips.h:14:
In file included from include/linux/clocksource.h:22:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
#define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
^
In file included from kernel/sched/build_utility.c:15:
In file included from include/linux/sched/isolation.h:6:
In file included from include/linux/tick.h:8:
In file included from include/linux/clockchips.h:14:
In file included from include/linux/clocksource.h:22:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
#define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
^
In file included from kernel/sched/build_utility.c:15:
In file included from include/linux/sched/isolation.h:6:
In file included from include/linux/tick.h:8:
In file included from include/linux/clockchips.h:14:
In file included from include/linux/clocksource.h:22:
In file included from arch/s390/include/asm/io.h:75:
include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
readsl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesb(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesw(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
writesl(PCI_IOBASE + addr, buffer, count);
~~~~~~~~~~ ^
In file included from kernel/sched/build_utility.c:93:
>> kernel/sched/core_sched.c:306:28: error: incomplete definition of type 'struct task_group'
struct cfs_rq *cfs_rq = tg->cfs_rq[cpu];
~~^
include/linux/sched.h:69:8: note: forward declaration of 'struct task_group'
struct task_group;
^
In file included from kernel/sched/build_utility.c:93:
kernel/sched/core_sched.c:307:32: error: incomplete definition of type 'struct task_group'
struct task_group *parent = tg->parent;
~~^
include/linux/sched.h:69:8: note: forward declaration of 'struct task_group'
struct task_group;
^
In file included from kernel/sched/build_utility.c:93:
kernel/sched/core_sched.c:319:12: error: incomplete definition of type 'struct task_group'
delta = tg->forceidle_sum_pending;
~~^
include/linux/sched.h:69:8: note: forward declaration of 'struct task_group'
struct task_group;
^
In file included from kernel/sched/build_utility.c:93:
kernel/sched/core_sched.c:321:5: error: incomplete definition of type 'struct task_group'
tg->forceidle_sum_pending = 0;
~~^
include/linux/sched.h:69:8: note: forward declaration of 'struct task_group'
struct task_group;
^
In file included from kernel/sched/build_utility.c:93:
kernel/sched/core_sched.c:333:4: error: incomplete definition of type 'struct task_group'
tg->forceidle_sum += delta;
~~^
include/linux/sched.h:69:8: note: forward declaration of 'struct task_group'
struct task_group;
^
In file included from kernel/sched/build_utility.c:93:
kernel/sched/core_sched.c:334:8: error: incomplete definition of type 'struct task_group'
parent->forceidle_sum_pending += delta;
~~~~~~^
include/linux/sched.h:69:8: note: forward declaration of 'struct task_group'
struct task_group;
^
In file included from kernel/sched/build_utility.c:93:
kernel/sched/core_sched.c:340:9: error: incomplete definition of type 'struct task_group'
if (!tg->parent) {
~~^
include/linux/sched.h:69:8: note: forward declaration of 'struct task_group'
struct task_group;
^
In file included from kernel/sched/build_utility.c:93:
kernel/sched/core_sched.c:353:12: error: incomplete definition of type 'struct task_group'
return tg->forceidle_sum;
~~^
include/linux/sched.h:69:8: note: forward declaration of 'struct task_group'
struct task_group;
^
12 warnings and 8 errors generated.

vim +306 kernel/sched/core_sched.c

303
304 void sched_core_rstat_flush(struct task_group *tg, int cpu)
305 {
> 306 struct cfs_rq *cfs_rq = tg->cfs_rq[cpu];
307 struct task_group *parent = tg->parent;
308 u64 delta, curr_sum;
309
310 /* root uses cpustat */
311 if (!parent)
312 return;
313
314 /*
315 * Note: cgroup_rstat_lock protects cfs_rq->forceidle_sum_prev and
316 * tg->{forceidle_sum, forceidle_sum_pending}.
317 */
318
319 delta = tg->forceidle_sum_pending;
320 if (delta)
321 tg->forceidle_sum_pending = 0;
322
323 /* rq lock not held; value may change concurrently */
324 curr_sum = READ_ONCE(cfs_rq->forceidle_sum);
325 if (curr_sum != cfs_rq->forceidle_sum_prev) {
326 delta += curr_sum - cfs_rq->forceidle_sum_prev;
327 cfs_rq->forceidle_sum_prev = curr_sum;
328 }
329
330 if (!delta)
331 return;
332
333 tg->forceidle_sum += delta;
334 parent->forceidle_sum_pending += delta;
335 }
336

--
0-DAY CI Kernel Test Service
https://01.org/lkp

2022-05-21 10:38:08

by Josh Don

[permalink] [raw]

Subject: Re: [PATCH] sched/core: add forced idle accounting for cgroups

On Fri, May 20, 2022 at 11:43 AM Tejun Heo <[email protected]> wrote:
>
> Hello,
>
> Sorry about late reply and thanks for the ping. I missed this one.
>
> On Fri, May 13, 2022 at 12:23:16PM -0700, Josh Don wrote:
> > Yea, that's right, this doesn't require the cpu controller to be
> > enabled. Are you suggesting to add a new field to cgroup_base_stat?
>
> Yes, that's what I meant. I think it'd fit there better.

Sounds good, I'll send a second version of the patch with that change.

> > One other weird artifact of collecting forceidle time is that a cpu
> > may account it on behalf of its hyperthread sibling. Currently, the
> > core rstat code always accounts to the current cpu's percpu rstat
> > field. I can add an accounting function to support writes to a
> > different cpu's field, in order to make sure that the per-cpu totals
> > are correct (the forceidle accounting code holds rq->__lock, which
> > protects all HT siblings of a core). percpu totals aren't currently
> > exported in cgroup v2, but this is useful information that we'll
> > consume, so it would be nice to keep it accurate.
>
> Sure, as long as it doesn't incur overhead when not used.

The extra complexity actually doesn't seem to be required. Per-core
totals will be accurate, which is the important part.

Thanks,
Josh