[PATCH] tg: add cpu's wait_count of a task group

[PATCH] tg: add cpu's wait_count of a task group

[wu860403-Re5JQEeQqe8AvxtiuMwx3w]

From: Liming Wu <wu860403-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Now we can rely on PSI to reflect whether there is contention in
the task group, but it cannot reflect the details of the contention.
Through this metric, we can get details of task group contention
 from the dimension of scheduling.
   delta(wait_sum)/delta(wait_count)

Also unified the cpu.stat output of cgroup v1 and v2

Signed-off-by Liming Wu <19092205-t/fWbKJQg0/QT0dZR+AlfA@public.gmane.org>
---
 kernel/sched/core.c | 33 ++++++++++++++++++++++++++++-----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index e7e453492..e7ff47436 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -8139,19 +8139,29 @@ static int cpu_cfs_stat_show(struct seq_file *sf, void *v)
 {
 	struct task_group *tg = css_tg(seq_css(sf));
 	struct cfs_bandwidth *cfs_b = &tg->cfs_bandwidth;
+	u64 throttled_usec;
 
-	seq_printf(sf, "nr_periods %d\n", cfs_b->nr_periods);
-	seq_printf(sf, "nr_throttled %d\n", cfs_b->nr_throttled);
-	seq_printf(sf, "throttled_time %llu\n", cfs_b->throttled_time);
+	throttled_usec = cfs_b->throttled_time;
+	do_div(throttled_usec, NSEC_PER_USEC);
+
+	seq_printf(sf, "nr_periods %d\n"
+		   "nr_throttled %d\n"
+		   "throttled_usec %llu\n",
+		   cfs_b->nr_periods, cfs_b->nr_throttled,
+		   throttled_usec);
 
 	if (schedstat_enabled() && tg != &root_task_group) {
 		u64 ws = 0;
+		u64 wc = 0;
 		int i;
 
-		for_each_possible_cpu(i)
+		for_each_possible_cpu(i) {
 			ws += schedstat_val(tg->se[i]->statistics.wait_sum);
+			wc += schedstat_val(tg->se[i]->statistics.wait_count);
+		}
 
-		seq_printf(sf, "wait_sum %llu\n", ws);
+		seq_printf(sf, "wait_sum %llu\n"
+			"wait_count %llu\n", ws, wc);
 	}
 
 	return 0;
@@ -8255,6 +8265,19 @@ static int cpu_extra_stat_show(struct seq_file *sf,
 			   "throttled_usec %llu\n",
 			   cfs_b->nr_periods, cfs_b->nr_throttled,
 			   throttled_usec);
+		if (schedstat_enabled() && tg != &root_task_group) {
+			u64 ws = 0;
+			u64 wc = 0;
+			int i;
+
+			for_each_possible_cpu(i) {
+				ws += schedstat_val(tg->se[i]->statistics.wait_sum);
+				wc += schedstat_val(tg->se[i]->statistics.wait_count);
+			}
+
+			seq_printf(sf, "wait_sum %llu\n"
+				"wait_count %llu\n", ws, wc);
+		}
 	}
 #endif
 	return 0;
-- 
2.20.1

Re: [PATCH] tg: add cpu's wait_count of a task group

[Tejun Heo]

Hello,

On Fri, Jan 15, 2021 at 10:30:05PM +0800, wu860403-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> -	seq_printf(sf, "throttled_time %llu\n", cfs_b->throttled_time);
...
> +	seq_printf(sf, "nr_periods %d\n"
> +		   "nr_throttled %d\n"
> +		   "throttled_usec %llu\n",
> +		   cfs_b->nr_periods, cfs_b->nr_throttled,
> +		   throttled_usec);

This is interface breaking change. I don't think we can do this at this
point.

> @@ -8255,6 +8265,19 @@ static int cpu_extra_stat_show(struct seq_file *sf,
>  			   "throttled_usec %llu\n",
>  			   cfs_b->nr_periods, cfs_b->nr_throttled,
>  			   throttled_usec);
> +		if (schedstat_enabled() && tg != &root_task_group) {
> +			u64 ws = 0;
> +			u64 wc = 0;
> +			int i;
> +
> +			for_each_possible_cpu(i) {
> +				ws += schedstat_val(tg->se[i]->statistics.wait_sum);
> +				wc += schedstat_val(tg->se[i]->statistics.wait_count);
> +			}
> +
> +			seq_printf(sf, "wait_sum %llu\n"
> +				"wait_count %llu\n", ws, wc);
> +		}

What does sum/count tell?

Thanks.

-- 
tejun

Re: [PATCH] tg: add cpu's wait_count of a task group

[liming wu]

Hello tejun:
On Sat, Jan 16, 2021 at 4:20 AM Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
> Hello,
>
> On Fri, Jan 15, 2021 at 10:30:05PM +0800, wu860403-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org wrote:
> > -     seq_printf(sf, "throttled_time %llu\n", cfs_b->throttled_time);
> ...
> > +     seq_printf(sf, "nr_periods %d\n"
> > +                "nr_throttled %d\n"
> > +                "throttled_usec %llu\n",
> > +                cfs_b->nr_periods, cfs_b->nr_throttled,
> > +                throttled_usec);
>
> This is interface breaking change. I don't think we can do this at this
> point.
Thanks for your reply, agree with it.
> > @@ -8255,6 +8265,19 @@ static int cpu_extra_stat_show(struct seq_file *sf,
> >                          "throttled_usec %llu\n",
> >                          cfs_b->nr_periods, cfs_b->nr_throttled,
> >                          throttled_usec);
> > +             if (schedstat_enabled() && tg != &root_task_group) {
> > +                     u64 ws = 0;
> > +                     u64 wc = 0;
> > +                     int i;
> > +
> > +                     for_each_possible_cpu(i) {
> > +                             ws += schedstat_val(tg->se[i]->statistics.wait_sum);
> > +                             wc += schedstat_val(tg->se[i]->statistics.wait_count);
> > +                     }
> > +
> > +                     seq_printf(sf, "wait_sum %llu\n"
> > +                             "wait_count %llu\n", ws, wc);
> > +             }
>
> What does sum/count tell?
It can tell the task group average latency of every context switch
wait_sum is equivalent to sched_info->run_delay (the second parameter
of /proc/$pid/schedstat)
wait_count is equivalent to sched_info->pcount(the third parameter of
/proc/$pid/schedstat)

Thanks

Liming

Re: [PATCH] tg: add cpu's wait_count of a task group

[Tejun Heo]

Hello,

On Mon, Jan 18, 2021 at 11:07:47AM +0800, liming wu wrote:
> > > +             if (schedstat_enabled() && tg != &root_task_group) {
> > > +                     u64 ws = 0;
> > > +                     u64 wc = 0;
> > > +                     int i;
> > > +
> > > +                     for_each_possible_cpu(i) {
> > > +                             ws += schedstat_val(tg->se[i]->statistics.wait_sum);
> > > +                             wc += schedstat_val(tg->se[i]->statistics.wait_count);
> > > +                     }
> > > +
> > > +                     seq_printf(sf, "wait_sum %llu\n"
> > > +                             "wait_count %llu\n", ws, wc);
> > > +             }
> >
> > What does sum/count tell?
> It can tell the task group average latency of every context switch
> wait_sum is equivalent to sched_info->run_delay (the second parameter
> of /proc/$pid/schedstat)
> wait_count is equivalent to sched_info->pcount(the third parameter of
> /proc/$pid/schedstat)

Sounds good to me but can you please:

* Rename wait_sum to wait_usec and make sure the duration is in usecs.

* Rename wait_count to nr_waits.

* This should go through the scheduler tree. When you post the updated
  version, please cc the scheduler folks from MAINAINERS.

Thanks.

-- 
tejun