LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Yafang Shao <laoar.shao@gmail.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>, Mel Gorman <mgorman@suse.de>,
	Juri Lelli <juri.lelli@redhat.com>,
	Vincent Guittot <vincent.guittot@linaro.org>,
	Dietmar Eggemann <dietmar.eggemann@arm.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Benjamin Segall <bsegall@google.com>,
	Daniel Bristot de Oliveira <bristot@redhat.com>,
	Alison Chaiken <achaiken@aurora.tech>,
	kbuild test robot <lkp@intel.com>,
	LKML <linux-kernel@vger.kernel.org>,
	linux-rt-users <linux-rt-users@vger.kernel.org>
Subject: Re: [PATCH v3 0/7] sched: support schedstats for RT sched class
Date: Tue, 31 Aug 2021 21:21:14 +0800	[thread overview]
Message-ID: <CALOAHbB53Mnqn-bfxY7EUt-MEDZ_NpjmHYMruf3D+WzN1JGAYg@mail.gmail.com> (raw)
In-Reply-To: <YS4IE1Dxf0ite81A@hirez.programming.kicks-ass.net>

On Tue, Aug 31, 2021 at 6:44 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Aug 31, 2021 at 12:08:15PM +0200, Peter Zijlstra wrote:
> > On Tue, Aug 24, 2021 at 11:29:39AM +0000, Yafang Shao wrote:
>
> > > After the patchset, schestats are orgnized as follows,
> > > struct task_struct {
> > >     ...
> > >     struct sched_statistics statistics;
> > >     ...
> > >     struct sched_entity *se;
> > >     struct sched_rt_entity *rt;
> > >     ...
> > > };
> > >
> > > struct task_group {                    |---> stats[0] : of CPU0
> > >     ...                                |
> > >     struct sched_statistics **stats; --|---> stats[1] : of CPU1
> > >     ...                                |
> > >                                        |---> stats[n] : of CPUn
> > >  #ifdef CONFIG_FAIR_GROUP_SCHED
> > >     struct sched_entity **se;
> > >  #endif
> > >  #ifdef CONFIG_RT_GROUP_SCHED
> > >     struct sched_rt_entity  **rt_se;
> > >  #endif
> > >     ...
> > > };
> >
> > Yeah, this seems to give a terrible mess, let me see if I can come up
> > with anything less horrible.
>
> Here, isn't this *MUCH* saner ?
>

Seems like a good idea.
I will verify it.


> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -521,7 +521,7 @@ struct sched_statistics {
>         u64                             nr_wakeups_passive;
>         u64                             nr_wakeups_idle;
>  #endif
> -};
> +} ____cacheline_aligned;
>
>  struct sched_entity {
>         /* For load-balancing: */
> @@ -537,8 +537,6 @@ struct sched_entity {
>
>         u64                             nr_migrations;
>
> -       struct sched_statistics         statistics;
> -
>  #ifdef CONFIG_FAIR_GROUP_SCHED
>         int                             depth;
>         struct sched_entity             *parent;
> @@ -802,6 +800,8 @@ struct task_struct {
>         struct uclamp_se                uclamp[UCLAMP_CNT];
>  #endif
>
> +       struct sched_statistics         stats;
> +

The stats was kept close to 'struct sched_entity se' before, because I
don't want to change the original layout of 'struct task_struct' too
much, in case the change may impact the cache line.
I'm not sure whether it is proper to place it here, I will verify it.

>  #ifdef CONFIG_PREEMPT_NOTIFIERS
>         /* List of struct preempt_notifier: */
>         struct hlist_head               preempt_notifiers;
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3489,11 +3489,11 @@ ttwu_stat(struct task_struct *p, int cpu
>  #ifdef CONFIG_SMP
>         if (cpu == rq->cpu) {
>                 __schedstat_inc(rq->ttwu_local);
> -               __schedstat_inc(p->se.statistics.nr_wakeups_local);
> +               __schedstat_inc(p->stats.nr_wakeups_local);
>         } else {
>                 struct sched_domain *sd;
>
> -               __schedstat_inc(p->se.statistics.nr_wakeups_remote);
> +               __schedstat_inc(p->stats.nr_wakeups_remote);
>                 rcu_read_lock();
>                 for_each_domain(rq->cpu, sd) {
>                         if (cpumask_test_cpu(cpu, sched_domain_span(sd))) {
> @@ -3505,14 +3505,14 @@ ttwu_stat(struct task_struct *p, int cpu
>         }
>
>         if (wake_flags & WF_MIGRATED)
> -               __schedstat_inc(p->se.statistics.nr_wakeups_migrate);
> +               __schedstat_inc(p->stats.nr_wakeups_migrate);
>  #endif /* CONFIG_SMP */
>
>         __schedstat_inc(rq->ttwu_count);
> -       __schedstat_inc(p->se.statistics.nr_wakeups);
> +       __schedstat_inc(p->stats.nr_wakeups);
>
>         if (wake_flags & WF_SYNC)
> -               __schedstat_inc(p->se.statistics.nr_wakeups_sync);
> +               __schedstat_inc(p->stats.nr_wakeups_sync);
>  }
>
>  /*
> @@ -4196,7 +4196,7 @@ static void __sched_fork(unsigned long c
>
>  #ifdef CONFIG_SCHEDSTATS
>         /* Even if schedstat is disabled, there should not be garbage */
> -       memset(&p->se.statistics, 0, sizeof(p->se.statistics));
> +       memset(&p->stats, 0, sizeof(p->stats));
>  #endif
>
>         RB_CLEAR_NODE(&p->dl.rb_node);
> @@ -9619,9 +9619,9 @@ void normalize_rt_tasks(void)
>                         continue;
>
>                 p->se.exec_start = 0;
> -               schedstat_set(p->se.statistics.wait_start,  0);
> -               schedstat_set(p->se.statistics.sleep_start, 0);
> -               schedstat_set(p->se.statistics.block_start, 0);
> +               schedstat_set(p->stats.wait_start,  0);
> +               schedstat_set(p->stats.sleep_start, 0);
> +               schedstat_set(p->stats.block_start, 0);
>
>                 if (!dl_task(p) && !rt_task(p)) {
>                         /*
> @@ -10467,7 +10467,7 @@ static int cpu_cfs_stat_show(struct seq_
>                 int i;
>
>                 for_each_possible_cpu(i)
> -                       ws += schedstat_val(tg->se[i]->statistics.wait_sum);
> +                       ws += schedstat_val(tg->stats[i]->wait_sum);
>
>                 seq_printf(sf, "wait_sum %llu\n", ws);
>         }
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -1265,8 +1265,8 @@ static void update_curr_dl(struct rq *rq
>                 return;
>         }
>
> -       schedstat_set(curr->se.statistics.exec_max,
> -                     max(curr->se.statistics.exec_max, delta_exec));
> +       schedstat_set(curr->stats.exec_max,
> +                     max(curr->stats.exec_max, delta_exec));
>
>         curr->se.sum_exec_runtime += delta_exec;
>         account_group_exec_runtime(curr, delta_exec);
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -819,6 +819,21 @@ static void update_tg_load_avg(struct cf
>  }
>  #endif /* CONFIG_SMP */
>
> +struct sched_entity_stats {
> +       struct sched_entity     se;
> +       struct sched_statistics stats;
> +} __no_randomize_layout;
> +
> +static inline struct sched_statistics *
> +__schedstats_from_se(struct sched_entity *se)
> +{
> +#ifdef CONFIG_FAIR_GROUP_SCHED
> +       if (!entity_is_task(se))
> +               return &container_of(se, struct sched_entity_stats, se)->stats;
> +#endif
> +       return &task_of(se)->stats;
> +}
> +
>  /*
>   * Update the current task's runtime statistics.
>   */
> @@ -837,8 +852,10 @@ static void update_curr(struct cfs_rq *c
>
>         curr->exec_start = now;
>
> -       schedstat_set(curr->statistics.exec_max,
> -                     max(delta_exec, curr->statistics.exec_max));
> +       if (schedstat_enabled()) {
> +               struct sched_statistics *stats = __schedstats_from_se(curr);
> +               __schedstat_set(stats->exec_max, max(delta_exec, stats->exec_max));
> +       }
>
>         curr->sum_exec_runtime += delta_exec;
>         schedstat_add(cfs_rq->exec_clock, delta_exec);
> @@ -866,39 +883,45 @@ static inline void
>  update_stats_wait_start(struct cfs_rq *cfs_rq, struct sched_entity *se)
>  {
>         u64 wait_start, prev_wait_start;
> +       struct sched_statistics *stats;
>
>         if (!schedstat_enabled())
>                 return;
>
> +       stats = __schedstats_from_se(se);
> +
>         wait_start = rq_clock(rq_of(cfs_rq));
> -       prev_wait_start = schedstat_val(se->statistics.wait_start);
> +       prev_wait_start = schedstat_val(stats->wait_start);
>
>         if (entity_is_task(se) && task_on_rq_migrating(task_of(se)) &&
>             likely(wait_start > prev_wait_start))
>                 wait_start -= prev_wait_start;
>
> -       __schedstat_set(se->statistics.wait_start, wait_start);
> +       __schedstat_set(stats->wait_start, wait_start);
>  }
>
>  static inline void
>  update_stats_wait_end(struct cfs_rq *cfs_rq, struct sched_entity *se)
>  {
> -       struct task_struct *p;
> +       struct sched_statistics *stats;
> +       struct task_struct *p = NULL;
>         u64 delta;
>
>         if (!schedstat_enabled())
>                 return;
>
> +       stats = __schedstats_from_se(se);
> +
>         /*
>          * When the sched_schedstat changes from 0 to 1, some sched se
>          * maybe already in the runqueue, the se->statistics.wait_start
>          * will be 0.So it will let the delta wrong. We need to avoid this
>          * scenario.
>          */
> -       if (unlikely(!schedstat_val(se->statistics.wait_start)))
> +       if (unlikely(!schedstat_val(stats->wait_start)))
>                 return;
>
> -       delta = rq_clock(rq_of(cfs_rq)) - schedstat_val(se->statistics.wait_start);
> +       delta = rq_clock(rq_of(cfs_rq)) - schedstat_val(stats->wait_start);
>
>         if (entity_is_task(se)) {
>                 p = task_of(se);
> @@ -908,30 +931,33 @@ update_stats_wait_end(struct cfs_rq *cfs
>                          * time stamp can be adjusted to accumulate wait time
>                          * prior to migration.
>                          */
> -                       __schedstat_set(se->statistics.wait_start, delta);
> +                       __schedstat_set(stats->wait_start, delta);
>                         return;
>                 }
>                 trace_sched_stat_wait(p, delta);
>         }
>
> -       __schedstat_set(se->statistics.wait_max,
> -                     max(schedstat_val(se->statistics.wait_max), delta));
> -       __schedstat_inc(se->statistics.wait_count);
> -       __schedstat_add(se->statistics.wait_sum, delta);
> -       __schedstat_set(se->statistics.wait_start, 0);
> +       __schedstat_set(stats->wait_max,
> +                     max(schedstat_val(stats->wait_max), delta));
> +       __schedstat_inc(stats->wait_count);
> +       __schedstat_add(stats->wait_sum, delta);
> +       __schedstat_set(stats->wait_start, 0);
>  }
>
>  static inline void
>  update_stats_enqueue_sleeper(struct cfs_rq *cfs_rq, struct sched_entity *se)
>  {
> +       struct sched_statistics *stats;
>         struct task_struct *tsk = NULL;
>         u64 sleep_start, block_start;
>
>         if (!schedstat_enabled())
>                 return;
>
> -       sleep_start = schedstat_val(se->statistics.sleep_start);
> -       block_start = schedstat_val(se->statistics.block_start);
> +       stats = __schedstats_from_se(se);
> +
> +       sleep_start = schedstat_val(stats->sleep_start);
> +       block_start = schedstat_val(stats->block_start);
>
>         if (entity_is_task(se))
>                 tsk = task_of(se);
> @@ -942,11 +968,11 @@ update_stats_enqueue_sleeper(struct cfs_
>                 if ((s64)delta < 0)
>                         delta = 0;
>
> -               if (unlikely(delta > schedstat_val(se->statistics.sleep_max)))
> -                       __schedstat_set(se->statistics.sleep_max, delta);
> +               if (unlikely(delta > schedstat_val(stats->sleep_max)))
> +                       __schedstat_set(stats->sleep_max, delta);
>
> -               __schedstat_set(se->statistics.sleep_start, 0);
> -               __schedstat_add(se->statistics.sum_sleep_runtime, delta);
> +               __schedstat_set(stats->sleep_start, 0);
> +               __schedstat_add(stats->sum_sleep_runtime, delta);
>
>                 if (tsk) {
>                         account_scheduler_latency(tsk, delta >> 10, 1);
> @@ -959,16 +985,16 @@ update_stats_enqueue_sleeper(struct cfs_
>                 if ((s64)delta < 0)
>                         delta = 0;
>
> -               if (unlikely(delta > schedstat_val(se->statistics.block_max)))
> -                       __schedstat_set(se->statistics.block_max, delta);
> +               if (unlikely(delta > schedstat_val(stats->block_max)))
> +                       __schedstat_set(stats->block_max, delta);
>
> -               __schedstat_set(se->statistics.block_start, 0);
> -               __schedstat_add(se->statistics.sum_sleep_runtime, delta);
> +               __schedstat_set(stats->block_start, 0);
> +               __schedstat_add(stats->sum_sleep_runtime, delta);
>
>                 if (tsk) {
>                         if (tsk->in_iowait) {
> -                               __schedstat_add(se->statistics.iowait_sum, delta);
> -                               __schedstat_inc(se->statistics.iowait_count);
> +                               __schedstat_add(stats->iowait_sum, delta);
> +                               __schedstat_inc(stats->iowait_count);
>                                 trace_sched_stat_iowait(tsk, delta);
>                         }
>
> @@ -1030,10 +1056,10 @@ update_stats_dequeue(struct cfs_rq *cfs_
>                 /* XXX racy against TTWU */
>                 state = READ_ONCE(tsk->__state);
>                 if (state & TASK_INTERRUPTIBLE)
> -                       __schedstat_set(se->statistics.sleep_start,
> +                       __schedstat_set(tsk->stats.sleep_start,
>                                       rq_clock(rq_of(cfs_rq)));
>                 if (state & TASK_UNINTERRUPTIBLE)
> -                       __schedstat_set(se->statistics.block_start,
> +                       __schedstat_set(tsk->stats.block_start,
>                                       rq_clock(rq_of(cfs_rq)));
>         }
>  }
> @@ -4502,9 +4528,10 @@ set_next_entity(struct cfs_rq *cfs_rq, s
>          */
>         if (schedstat_enabled() &&
>             rq_of(cfs_rq)->cfs.load.weight >= 2*se->load.weight) {
> -               __schedstat_set(se->statistics.slice_max,
> -                       max((u64)schedstat_val(se->statistics.slice_max),
> -                           se->sum_exec_runtime - se->prev_sum_exec_runtime));
> +               struct sched_statistics *stats = __schedstats_from_se(se);
> +               __schedstat_set(stats->slice_max,
> +                               max((u64)stats->slice_max,
> +                                   se->sum_exec_runtime - se->prev_sum_exec_runtime));
>         }
>
>         se->prev_sum_exec_runtime = se->sum_exec_runtime;
> @@ -5993,12 +6020,12 @@ static int wake_affine(struct sched_doma
>         if (sched_feat(WA_WEIGHT) && target == nr_cpumask_bits)
>                 target = wake_affine_weight(sd, p, this_cpu, prev_cpu, sync);
>
> -       schedstat_inc(p->se.statistics.nr_wakeups_affine_attempts);
> +       schedstat_inc(p->stats.nr_wakeups_affine_attempts);
>         if (target == nr_cpumask_bits)
>                 return prev_cpu;
>
>         schedstat_inc(sd->ttwu_move_affine);
> -       schedstat_inc(p->se.statistics.nr_wakeups_affine);
> +       schedstat_inc(p->stats.nr_wakeups_affine);
>         return target;
>  }
>
> @@ -7802,7 +7829,7 @@ int can_migrate_task(struct task_struct
>         if (!cpumask_test_cpu(env->dst_cpu, p->cpus_ptr)) {
>                 int cpu;
>
> -               schedstat_inc(p->se.statistics.nr_failed_migrations_affine);
> +               schedstat_inc(p->stats.nr_failed_migrations_affine);
>
>                 env->flags |= LBF_SOME_PINNED;
>
> @@ -7836,7 +7863,7 @@ int can_migrate_task(struct task_struct
>         env->flags &= ~LBF_ALL_PINNED;
>
>         if (task_running(env->src_rq, p)) {
> -               schedstat_inc(p->se.statistics.nr_failed_migrations_running);
> +               schedstat_inc(p->stats.nr_failed_migrations_running);
>                 return 0;
>         }
>
> @@ -7858,12 +7885,12 @@ int can_migrate_task(struct task_struct
>             env->sd->nr_balance_failed > env->sd->cache_nice_tries) {
>                 if (tsk_cache_hot == 1) {
>                         schedstat_inc(env->sd->lb_hot_gained[env->idle]);
> -                       schedstat_inc(p->se.statistics.nr_forced_migrations);
> +                       schedstat_inc(p->stats.nr_forced_migrations);
>                 }
>                 return 1;
>         }
>
> -       schedstat_inc(p->se.statistics.nr_failed_migrations_hot);
> +       schedstat_inc(p->stats.nr_failed_migrations_hot);
>         return 0;
>  }
>
> @@ -11390,7 +11417,7 @@ int alloc_fair_sched_group(struct task_g
>                 if (!cfs_rq)
>                         goto err;
>
> -               se = kzalloc_node(sizeof(struct sched_entity),
> +               se = kzalloc_node(sizeof(struct sched_entity_stats),
>                                   GFP_KERNEL, cpu_to_node(i));
>                 if (!se)
>                         goto err_free_rq;
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -1009,8 +1009,8 @@ static void update_curr_rt(struct rq *rq
>         if (unlikely((s64)delta_exec <= 0))
>                 return;
>
> -       schedstat_set(curr->se.statistics.exec_max,
> -                     max(curr->se.statistics.exec_max, delta_exec));
> +       schedstat_set(curr->stats.exec_max,
> +                     max(curr->stats.exec_max, delta_exec));
>
>         curr->se.sum_exec_runtime += delta_exec;
>         account_group_exec_runtime(curr, delta_exec);
> --- a/kernel/sched/stats.h
> +++ b/kernel/sched/stats.h
> @@ -41,6 +41,7 @@ rq_sched_info_dequeue(struct rq *rq, uns
>  #define   schedstat_val_or_zero(var)   ((schedstat_enabled()) ? (var) : 0)
>
>  #else /* !CONFIG_SCHEDSTATS: */
> +
>  static inline void rq_sched_info_arrive  (struct rq *rq, unsigned long long delta) { }
>  static inline void rq_sched_info_dequeue(struct rq *rq, unsigned long long delta) { }
>  static inline void rq_sched_info_depart  (struct rq *rq, unsigned long long delta) { }
> @@ -53,6 +54,7 @@ static inline void rq_sched_info_depart
>  # define   schedstat_set(var, val)     do { } while (0)
>  # define   schedstat_val(var)          0
>  # define   schedstat_val_or_zero(var)  0
> +
>  #endif /* CONFIG_SCHEDSTATS */
>
>  #ifdef CONFIG_PSI
> --- a/kernel/sched/stop_task.c
> +++ b/kernel/sched/stop_task.c
> @@ -78,8 +78,8 @@ static void put_prev_task_stop(struct rq
>         if (unlikely((s64)delta_exec < 0))
>                 delta_exec = 0;
>
> -       schedstat_set(curr->se.statistics.exec_max,
> -                       max(curr->se.statistics.exec_max, delta_exec));
> +       schedstat_set(curr->stats.exec_max,
> +                     max(curr->stats.exec_max, delta_exec));
>
>         curr->se.sum_exec_runtime += delta_exec;
>         account_group_exec_runtime(curr, delta_exec);



-- 
Thanks
Yafang

  reply	other threads:[~2021-08-31 13:21 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-24 11:29 Yafang Shao
2021-08-24 11:29 ` [PATCH v3 1/7] sched, fair: use __schedstat_set() in set_next_entity() Yafang Shao
2021-08-24 11:29 ` [PATCH v3 2/7] sched: make struct sched_statistics independent of fair sched class Yafang Shao
2021-08-31 10:14   ` Peter Zijlstra
2021-08-31 13:25     ` Yafang Shao
2021-08-31 10:19   ` Peter Zijlstra
2021-08-31 13:25     ` Yafang Shao
2021-08-24 11:29 ` [PATCH v3 3/7] sched: make schedstats helpers " Yafang Shao
2021-08-31 11:07   ` Peter Zijlstra
2021-08-31 13:27     ` Yafang Shao
2021-08-24 11:29 ` [PATCH v3 4/7] sched: make the output of schedstats " Yafang Shao
2021-08-31 11:08   ` Peter Zijlstra
2021-08-31 13:27     ` Yafang Shao
2021-08-24 11:29 ` [PATCH v3 5/7] sched: introduce task block time in schedstats Yafang Shao
2021-08-24 11:29 ` [PATCH v3 6/7] sched, rt: support sched_stat_runtime tracepoint for RT sched class Yafang Shao
2021-08-24 11:29 ` [PATCH v3 7/7] sched, rt: support schedstats " Yafang Shao
2021-08-31 10:08 ` [PATCH v3 0/7] sched: " Peter Zijlstra
2021-08-31 10:44   ` Peter Zijlstra
2021-08-31 13:21     ` Yafang Shao [this message]
2021-08-31 12:57   ` Yafang Shao

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CALOAHbB53Mnqn-bfxY7EUt-MEDZ_NpjmHYMruf3D+WzN1JGAYg@mail.gmail.com \
    --to=laoar.shao@gmail.com \
    --cc=achaiken@aurora.tech \
    --cc=bristot@redhat.com \
    --cc=bsegall@google.com \
    --cc=dietmar.eggemann@arm.com \
    --cc=juri.lelli@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-rt-users@vger.kernel.org \
    --cc=lkp@intel.com \
    --cc=mgorman@suse.de \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=vincent.guittot@linaro.org \
    --subject='Re: [PATCH v3 0/7] sched: support schedstats for RT sched class' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).