* [PATCH] kernel/sched: Update schedstats when migrating threads @ 2022-01-26 15:22 Carlos Bilbao 2022-02-22 16:32 ` Carlos Bilbao 2022-02-23 10:19 ` Peter Zijlstra 0 siblings, 2 replies; 12+ messages in thread From: Carlos Bilbao @ 2022-01-26 15:22 UTC (permalink / raw) To: peterz, juri.lelli Cc: vincent.guittot, mingo, dietmar.eggemann, rostedt, bsegall, mgorman, bristot, linux-kernel, Carlos Bilbao The kernel manages per-task scheduler statistics or schedstats. Such counters should be reinitialized when the thread is migrated to a different core rq, except for the values recording number of migrations. Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com> --- kernel/sched/core.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/kernel/sched/core.c b/kernel/sched/core.c index fe53e510e711..d64c2a290176 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -8757,6 +8757,7 @@ bool sched_smp_initialized __read_mostly; int migrate_task_to(struct task_struct *p, int target_cpu) { struct migration_arg arg = { p, target_cpu }; + uint64_t forced_migrations, migrations_cold; int curr_cpu = task_cpu(p); if (curr_cpu == target_cpu) @@ -8765,7 +8766,14 @@ int migrate_task_to(struct task_struct *p, int target_cpu) if (!cpumask_test_cpu(target_cpu, p->cpus_ptr)) return -EINVAL; - /* TODO: This is not properly updating schedstats */ + if (schedstat_enabled()) { + forced_migrations = schedstat_val(p->stats.nr_forced_migrations); + migrations_cold = schedstat_val(p->stats.nr_migrations_cold); + memset(&p->stats, 0, sizeof(p->stats)); + schedstat_set(p->stats.nr_forced_migrations, forced_migrations); + schedstat_set(p->stats.nr_migrations_cold, migrations_cold); + schedstat_inc(p->stats.nr_migrations_cold); + } trace_sched_move_numa(p, curr_cpu, target_cpu); return stop_one_cpu(curr_cpu, migration_cpu_stop, &arg); -- 2.27.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] kernel/sched: Update schedstats when migrating threads 2022-01-26 15:22 [PATCH] kernel/sched: Update schedstats when migrating threads Carlos Bilbao @ 2022-02-22 16:32 ` Carlos Bilbao 2022-02-22 17:15 ` Steven Rostedt 2022-02-23 10:19 ` Peter Zijlstra 1 sibling, 1 reply; 12+ messages in thread From: Carlos Bilbao @ 2022-02-22 16:32 UTC (permalink / raw) To: peterz, juri.lelli Cc: vincent.guittot, mingo, dietmar.eggemann, rostedt, bsegall, mgorman, bristot, linux-kernel Hi, On 1/26/2022 9:22 AM, Carlos Bilbao wrote: > The kernel manages per-task scheduler statistics or schedstats. Such > counters should be reinitialized when the thread is migrated to a > different core rq, except for the values recording number of migrations. > > Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com> > --- > kernel/sched/core.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index fe53e510e711..d64c2a290176 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -8757,6 +8757,7 @@ bool sched_smp_initialized __read_mostly; > int migrate_task_to(struct task_struct *p, int target_cpu) > { > struct migration_arg arg = { p, target_cpu }; > + uint64_t forced_migrations, migrations_cold; > int curr_cpu = task_cpu(p); > > if (curr_cpu == target_cpu) > @@ -8765,7 +8766,14 @@ int migrate_task_to(struct task_struct *p, int target_cpu) > if (!cpumask_test_cpu(target_cpu, p->cpus_ptr)) > return -EINVAL; > > - /* TODO: This is not properly updating schedstats */ > + if (schedstat_enabled()) { > + forced_migrations = schedstat_val(p->stats.nr_forced_migrations); > + migrations_cold = schedstat_val(p->stats.nr_migrations_cold); > + memset(&p->stats, 0, sizeof(p->stats)); > + schedstat_set(p->stats.nr_forced_migrations, forced_migrations); > + schedstat_set(p->stats.nr_migrations_cold, migrations_cold); > + schedstat_inc(p->stats.nr_migrations_cold); > + } > > trace_sched_move_numa(p, curr_cpu, target_cpu); > return stop_one_cpu(curr_cpu, migration_cpu_stop, &arg); I would love to hear some thoughts on this. Thanks, Carlos ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kernel/sched: Update schedstats when migrating threads 2022-02-22 16:32 ` Carlos Bilbao @ 2022-02-22 17:15 ` Steven Rostedt 0 siblings, 0 replies; 12+ messages in thread From: Steven Rostedt @ 2022-02-22 17:15 UTC (permalink / raw) To: Carlos Bilbao Cc: peterz, juri.lelli, vincent.guittot, mingo, dietmar.eggemann, bsegall, mgorman, bristot, linux-kernel On Tue, 22 Feb 2022 10:32:32 -0600 Carlos Bilbao <carlos.bilbao@amd.com> wrote: > > @@ -8765,7 +8766,14 @@ int migrate_task_to(struct task_struct *p, int target_cpu) > > if (!cpumask_test_cpu(target_cpu, p->cpus_ptr)) > > return -EINVAL; > > > > - /* TODO: This is not properly updating schedstats */ > > + if (schedstat_enabled()) { > > + forced_migrations = schedstat_val(p->stats.nr_forced_migrations); > > + migrations_cold = schedstat_val(p->stats.nr_migrations_cold); > > + memset(&p->stats, 0, sizeof(p->stats)); > > + schedstat_set(p->stats.nr_forced_migrations, forced_migrations); > > + schedstat_set(p->stats.nr_migrations_cold, migrations_cold); > > + schedstat_inc(p->stats.nr_migrations_cold); > > + } > > > > trace_sched_move_numa(p, curr_cpu, target_cpu); > > return stop_one_cpu(curr_cpu, migration_cpu_stop, &arg); > > > I would love to hear some thoughts on this. I have no issues with this. Peter? -- Steve ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kernel/sched: Update schedstats when migrating threads 2022-01-26 15:22 [PATCH] kernel/sched: Update schedstats when migrating threads Carlos Bilbao 2022-02-22 16:32 ` Carlos Bilbao @ 2022-02-23 10:19 ` Peter Zijlstra 2022-02-23 15:14 ` Carlos Bilbao 1 sibling, 1 reply; 12+ messages in thread From: Peter Zijlstra @ 2022-02-23 10:19 UTC (permalink / raw) To: Carlos Bilbao Cc: juri.lelli, vincent.guittot, mingo, dietmar.eggemann, rostedt, bsegall, mgorman, bristot, linux-kernel On Wed, Jan 26, 2022 at 09:22:23AM -0600, Carlos Bilbao wrote: > The kernel manages per-task scheduler statistics or schedstats. Such > counters should be reinitialized when the thread is migrated to a > different core rq, except for the values recording number of migrations. I'm confused, why should we reset schedstats on migrate? I'm thinking this breaks per-task, since tasks tend to bounce around quite a lot. > Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com> > --- > kernel/sched/core.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index fe53e510e711..d64c2a290176 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -8757,6 +8757,7 @@ bool sched_smp_initialized __read_mostly; > int migrate_task_to(struct task_struct *p, int target_cpu) > { > struct migration_arg arg = { p, target_cpu }; > + uint64_t forced_migrations, migrations_cold; > int curr_cpu = task_cpu(p); > > if (curr_cpu == target_cpu) > @@ -8765,7 +8766,14 @@ int migrate_task_to(struct task_struct *p, int target_cpu) > if (!cpumask_test_cpu(target_cpu, p->cpus_ptr)) > return -EINVAL; > > - /* TODO: This is not properly updating schedstats */ > + if (schedstat_enabled()) { > + forced_migrations = schedstat_val(p->stats.nr_forced_migrations); > + migrations_cold = schedstat_val(p->stats.nr_migrations_cold); > + memset(&p->stats, 0, sizeof(p->stats)); > + schedstat_set(p->stats.nr_forced_migrations, forced_migrations); > + schedstat_set(p->stats.nr_migrations_cold, migrations_cold); > + schedstat_inc(p->stats.nr_migrations_cold); > + } > > trace_sched_move_numa(p, curr_cpu, target_cpu); > return stop_one_cpu(curr_cpu, migration_cpu_stop, &arg); > -- > 2.27.0 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kernel/sched: Update schedstats when migrating threads 2022-02-23 10:19 ` Peter Zijlstra @ 2022-02-23 15:14 ` Carlos Bilbao 2022-02-23 15:28 ` Phil Auld 0 siblings, 1 reply; 12+ messages in thread From: Carlos Bilbao @ 2022-02-23 15:14 UTC (permalink / raw) To: Peter Zijlstra Cc: juri.lelli, vincent.guittot, mingo, dietmar.eggemann, rostedt, bsegall, mgorman, bristot, linux-kernel On 2/23/2022 4:19 AM, Peter Zijlstra wrote: > On Wed, Jan 26, 2022 at 09:22:23AM -0600, Carlos Bilbao wrote: >> The kernel manages per-task scheduler statistics or schedstats. Such >> counters should be reinitialized when the thread is migrated to a >> different core rq, except for the values recording number of migrations. > > I'm confused, why should we reset schedstats on migrate? I'm thinking > this breaks per-task, since tasks tend to bounce around quite a lot. > Thanks for your comments, Peter. Looking at the documentation of schedstats I see that most values are actually linked to the particular CPU: time spent on the cpu, timeslices run on this cpu, number of times _something_ was called when the cpu was idle, and so forth. Those values lose their meaning after migration and we should reinitialize their counters. However, reviewing sched_statistics I identify two fields that we should definitely keep increasing even after migration (nr_migrations_cold, nr_forced_migrations). So this patch will have to be upgraded if there's some other value(s) in schedstats that we do not want to reinitialize either. >> Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com> >> --- >> kernel/sched/core.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/kernel/sched/core.c b/kernel/sched/core.c >> index fe53e510e711..d64c2a290176 100644 >> --- a/kernel/sched/core.c >> +++ b/kernel/sched/core.c >> @@ -8757,6 +8757,7 @@ bool sched_smp_initialized __read_mostly; >> int migrate_task_to(struct task_struct *p, int target_cpu) >> { >> struct migration_arg arg = { p, target_cpu }; >> + uint64_t forced_migrations, migrations_cold; >> int curr_cpu = task_cpu(p); >> >> if (curr_cpu == target_cpu) >> @@ -8765,7 +8766,14 @@ int migrate_task_to(struct task_struct *p, int target_cpu) >> if (!cpumask_test_cpu(target_cpu, p->cpus_ptr)) >> return -EINVAL; >> >> - /* TODO: This is not properly updating schedstats */ >> + if (schedstat_enabled()) { >> + forced_migrations = schedstat_val(p->stats.nr_forced_migrations); >> + migrations_cold = schedstat_val(p->stats.nr_migrations_cold); >> + memset(&p->stats, 0, sizeof(p->stats)); >> + schedstat_set(p->stats.nr_forced_migrations, forced_migrations); >> + schedstat_set(p->stats.nr_migrations_cold, migrations_cold); >> + schedstat_inc(p->stats.nr_migrations_cold); >> + } >> >> trace_sched_move_numa(p, curr_cpu, target_cpu); >> return stop_one_cpu(curr_cpu, migration_cpu_stop, &arg); >> -- >> 2.27.0 >> Thanks, Carlos ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kernel/sched: Update schedstats when migrating threads 2022-02-23 15:14 ` Carlos Bilbao @ 2022-02-23 15:28 ` Phil Auld 2022-02-23 15:33 ` Carlos Bilbao 0 siblings, 1 reply; 12+ messages in thread From: Phil Auld @ 2022-02-23 15:28 UTC (permalink / raw) To: Carlos Bilbao Cc: Peter Zijlstra, juri.lelli, vincent.guittot, mingo, dietmar.eggemann, rostedt, bsegall, mgorman, bristot, linux-kernel On Wed, Feb 23, 2022 at 09:14:45AM -0600 Carlos Bilbao wrote: > On 2/23/2022 4:19 AM, Peter Zijlstra wrote: > > On Wed, Jan 26, 2022 at 09:22:23AM -0600, Carlos Bilbao wrote: > >> The kernel manages per-task scheduler statistics or schedstats. Such > >> counters should be reinitialized when the thread is migrated to a > >> different core rq, except for the values recording number of migrations. > > > > I'm confused, why should we reset schedstats on migrate? I'm thinking > > this breaks per-task, since tasks tend to bounce around quite a lot. > > > > Thanks for your comments, Peter. > > Looking at the documentation of schedstats I see that most values are > actually linked to the particular CPU: time spent on the cpu, timeslices > run on this cpu, number of times _something_ was called when the cpu was > idle, and so forth. Those values lose their meaning after migration and we > should reinitialize their counters. However, reviewing sched_statistics I > identify two fields that we should definitely keep increasing even after > migration (nr_migrations_cold, nr_forced_migrations). > The documentation is a little off. I think it should say "any cpu" instead of "this cpu". If you reset these per task counters (time on cpu, number of timeslices etc) on every migration then they become meaningless (and useless). Cheers, Phil > So this patch will have to be upgraded if there's some other value(s) in > schedstats that we do not want to reinitialize either. > > >> Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com> > >> --- > >> kernel/sched/core.c | 10 +++++++++- > >> 1 file changed, 9 insertions(+), 1 deletion(-) > >> > >> diff --git a/kernel/sched/core.c b/kernel/sched/core.c > >> index fe53e510e711..d64c2a290176 100644 > >> --- a/kernel/sched/core.c > >> +++ b/kernel/sched/core.c > >> @@ -8757,6 +8757,7 @@ bool sched_smp_initialized __read_mostly; > >> int migrate_task_to(struct task_struct *p, int target_cpu) > >> { > >> struct migration_arg arg = { p, target_cpu }; > >> + uint64_t forced_migrations, migrations_cold; > >> int curr_cpu = task_cpu(p); > >> > >> if (curr_cpu == target_cpu) > >> @@ -8765,7 +8766,14 @@ int migrate_task_to(struct task_struct *p, int target_cpu) > >> if (!cpumask_test_cpu(target_cpu, p->cpus_ptr)) > >> return -EINVAL; > >> > >> - /* TODO: This is not properly updating schedstats */ > >> + if (schedstat_enabled()) { > >> + forced_migrations = schedstat_val(p->stats.nr_forced_migrations); > >> + migrations_cold = schedstat_val(p->stats.nr_migrations_cold); > >> + memset(&p->stats, 0, sizeof(p->stats)); > >> + schedstat_set(p->stats.nr_forced_migrations, forced_migrations); > >> + schedstat_set(p->stats.nr_migrations_cold, migrations_cold); > >> + schedstat_inc(p->stats.nr_migrations_cold); > >> + } > >> > >> trace_sched_move_numa(p, curr_cpu, target_cpu); > >> return stop_one_cpu(curr_cpu, migration_cpu_stop, &arg); > >> -- > >> 2.27.0 > >> > > Thanks, > Carlos > -- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kernel/sched: Update schedstats when migrating threads 2022-02-23 15:28 ` Phil Auld @ 2022-02-23 15:33 ` Carlos Bilbao 2022-02-23 15:47 ` Phil Auld 0 siblings, 1 reply; 12+ messages in thread From: Carlos Bilbao @ 2022-02-23 15:33 UTC (permalink / raw) To: Phil Auld Cc: Peter Zijlstra, juri.lelli, vincent.guittot, mingo, dietmar.eggemann, rostedt, bsegall, mgorman, bristot, linux-kernel On 2/23/2022 9:28 AM, Phil Auld wrote: > On Wed, Feb 23, 2022 at 09:14:45AM -0600 Carlos Bilbao wrote: >> On 2/23/2022 4:19 AM, Peter Zijlstra wrote: >>> On Wed, Jan 26, 2022 at 09:22:23AM -0600, Carlos Bilbao wrote: >>>> The kernel manages per-task scheduler statistics or schedstats. Such >>>> counters should be reinitialized when the thread is migrated to a >>>> different core rq, except for the values recording number of migrations. >>> >>> I'm confused, why should we reset schedstats on migrate? I'm thinking >>> this breaks per-task, since tasks tend to bounce around quite a lot. >>> >> >> Thanks for your comments, Peter. >> >> Looking at the documentation of schedstats I see that most values are >> actually linked to the particular CPU: time spent on the cpu, timeslices >> run on this cpu, number of times _something_ was called when the cpu was >> idle, and so forth. Those values lose their meaning after migration and we >> should reinitialize their counters. However, reviewing sched_statistics I >> identify two fields that we should definitely keep increasing even after >> migration (nr_migrations_cold, nr_forced_migrations). >> > > The documentation is a little off. I think it should say "any cpu" instead > of "this cpu". If you reset these per task counters (time on cpu, number > of timeslices etc) on every migration then they become meaningless (and > useless). > > > Cheers, > Phil > Well that clarifies it! Then, let me ask the opposite question... What fields of schedstats should we clear when migrating? If there isn't any, I will just increase the number of migrations. >> So this patch will have to be upgraded if there's some other value(s) in >> schedstats that we do not want to reinitialize either. >> >>>> Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com> >>>> --- >>>> kernel/sched/core.c | 10 +++++++++- >>>> 1 file changed, 9 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c >>>> index fe53e510e711..d64c2a290176 100644 >>>> --- a/kernel/sched/core.c >>>> +++ b/kernel/sched/core.c >>>> @@ -8757,6 +8757,7 @@ bool sched_smp_initialized __read_mostly; >>>> int migrate_task_to(struct task_struct *p, int target_cpu) >>>> { >>>> struct migration_arg arg = { p, target_cpu }; >>>> + uint64_t forced_migrations, migrations_cold; >>>> int curr_cpu = task_cpu(p); >>>> >>>> if (curr_cpu == target_cpu) >>>> @@ -8765,7 +8766,14 @@ int migrate_task_to(struct task_struct *p, int target_cpu) >>>> if (!cpumask_test_cpu(target_cpu, p->cpus_ptr)) >>>> return -EINVAL; >>>> >>>> - /* TODO: This is not properly updating schedstats */ >>>> + if (schedstat_enabled()) { >>>> + forced_migrations = schedstat_val(p->stats.nr_forced_migrations); >>>> + migrations_cold = schedstat_val(p->stats.nr_migrations_cold); >>>> + memset(&p->stats, 0, sizeof(p->stats)); >>>> + schedstat_set(p->stats.nr_forced_migrations, forced_migrations); >>>> + schedstat_set(p->stats.nr_migrations_cold, migrations_cold); >>>> + schedstat_inc(p->stats.nr_migrations_cold); >>>> + } >>>> >>>> trace_sched_move_numa(p, curr_cpu, target_cpu); >>>> return stop_one_cpu(curr_cpu, migration_cpu_stop, &arg); >>>> -- >>>> 2.27.0 >>>> >> >> Thanks, >> Carlos >> > Thanks, Carlos ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kernel/sched: Update schedstats when migrating threads 2022-02-23 15:33 ` Carlos Bilbao @ 2022-02-23 15:47 ` Phil Auld 2022-02-23 15:59 ` Carlos Bilbao 0 siblings, 1 reply; 12+ messages in thread From: Phil Auld @ 2022-02-23 15:47 UTC (permalink / raw) To: Carlos Bilbao Cc: Peter Zijlstra, juri.lelli, vincent.guittot, mingo, dietmar.eggemann, rostedt, bsegall, mgorman, bristot, linux-kernel On Wed, Feb 23, 2022 at 09:33:59AM -0600 Carlos Bilbao wrote: > On 2/23/2022 9:28 AM, Phil Auld wrote: > > On Wed, Feb 23, 2022 at 09:14:45AM -0600 Carlos Bilbao wrote: > >> On 2/23/2022 4:19 AM, Peter Zijlstra wrote: > >>> On Wed, Jan 26, 2022 at 09:22:23AM -0600, Carlos Bilbao wrote: > >>>> The kernel manages per-task scheduler statistics or schedstats. Such > >>>> counters should be reinitialized when the thread is migrated to a > >>>> different core rq, except for the values recording number of migrations. > >>> > >>> I'm confused, why should we reset schedstats on migrate? I'm thinking > >>> this breaks per-task, since tasks tend to bounce around quite a lot. > >>> > >> > >> Thanks for your comments, Peter. > >> > >> Looking at the documentation of schedstats I see that most values are > >> actually linked to the particular CPU: time spent on the cpu, timeslices > >> run on this cpu, number of times _something_ was called when the cpu was > >> idle, and so forth. Those values lose their meaning after migration and we > >> should reinitialize their counters. However, reviewing sched_statistics I > >> identify two fields that we should definitely keep increasing even after > >> migration (nr_migrations_cold, nr_forced_migrations). > >> > > > > The documentation is a little off. I think it should say "any cpu" instead > > of "this cpu". If you reset these per task counters (time on cpu, number > > of timeslices etc) on every migration then they become meaningless (and > > useless). > > > > > > Cheers, > > Phil > > > > Well that clarifies it! Then, let me ask the opposite question... What > fields of schedstats should we clear when migrating? If there isn't any, > I will just increase the number of migrations. > I don't think any should be cleared on migration. They're per task and should be monotically increasing. If they ever reset it becomes hard to know what they mean when you read them. Cheers, Phil > >> So this patch will have to be upgraded if there's some other value(s) in > >> schedstats that we do not want to reinitialize either. > >> > >>>> Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com> > >>>> --- > >>>> kernel/sched/core.c | 10 +++++++++- > >>>> 1 file changed, 9 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c > >>>> index fe53e510e711..d64c2a290176 100644 > >>>> --- a/kernel/sched/core.c > >>>> +++ b/kernel/sched/core.c > >>>> @@ -8757,6 +8757,7 @@ bool sched_smp_initialized __read_mostly; > >>>> int migrate_task_to(struct task_struct *p, int target_cpu) > >>>> { > >>>> struct migration_arg arg = { p, target_cpu }; > >>>> + uint64_t forced_migrations, migrations_cold; > >>>> int curr_cpu = task_cpu(p); > >>>> > >>>> if (curr_cpu == target_cpu) > >>>> @@ -8765,7 +8766,14 @@ int migrate_task_to(struct task_struct *p, int target_cpu) > >>>> if (!cpumask_test_cpu(target_cpu, p->cpus_ptr)) > >>>> return -EINVAL; > >>>> > >>>> - /* TODO: This is not properly updating schedstats */ > >>>> + if (schedstat_enabled()) { > >>>> + forced_migrations = schedstat_val(p->stats.nr_forced_migrations); > >>>> + migrations_cold = schedstat_val(p->stats.nr_migrations_cold); > >>>> + memset(&p->stats, 0, sizeof(p->stats)); > >>>> + schedstat_set(p->stats.nr_forced_migrations, forced_migrations); > >>>> + schedstat_set(p->stats.nr_migrations_cold, migrations_cold); > >>>> + schedstat_inc(p->stats.nr_migrations_cold); > >>>> + } > >>>> > >>>> trace_sched_move_numa(p, curr_cpu, target_cpu); > >>>> return stop_one_cpu(curr_cpu, migration_cpu_stop, &arg); > >>>> -- > >>>> 2.27.0 > >>>> > >> > >> Thanks, > >> Carlos > >> > > > > Thanks, > Carlos > -- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] kernel/sched: Update schedstats when migrating threads 2022-02-23 15:47 ` Phil Auld @ 2022-02-23 15:59 ` Carlos Bilbao 2022-02-23 16:13 ` [PATCH v2] " Carlos Bilbao 0 siblings, 1 reply; 12+ messages in thread From: Carlos Bilbao @ 2022-02-23 15:59 UTC (permalink / raw) To: Phil Auld Cc: Peter Zijlstra, juri.lelli, vincent.guittot, mingo, dietmar.eggemann, rostedt, bsegall, mgorman, bristot, linux-kernel On 2/23/2022 9:47 AM, Phil Auld wrote: > On Wed, Feb 23, 2022 at 09:33:59AM -0600 Carlos Bilbao wrote: >> On 2/23/2022 9:28 AM, Phil Auld wrote: >>> On Wed, Feb 23, 2022 at 09:14:45AM -0600 Carlos Bilbao wrote: >>>> On 2/23/2022 4:19 AM, Peter Zijlstra wrote: >>>>> On Wed, Jan 26, 2022 at 09:22:23AM -0600, Carlos Bilbao wrote: >>>>>> The kernel manages per-task scheduler statistics or schedstats. Such >>>>>> counters should be reinitialized when the thread is migrated to a >>>>>> different core rq, except for the values recording number of migrations. >>>>> >>>>> I'm confused, why should we reset schedstats on migrate? I'm thinking >>>>> this breaks per-task, since tasks tend to bounce around quite a lot. >>>>> >>>> >>>> Thanks for your comments, Peter. >>>> >>>> Looking at the documentation of schedstats I see that most values are >>>> actually linked to the particular CPU: time spent on the cpu, timeslices >>>> run on this cpu, number of times _something_ was called when the cpu was >>>> idle, and so forth. Those values lose their meaning after migration and we >>>> should reinitialize their counters. However, reviewing sched_statistics I >>>> identify two fields that we should definitely keep increasing even after >>>> migration (nr_migrations_cold, nr_forced_migrations). >>>> >>> >>> The documentation is a little off. I think it should say "any cpu" instead >>> of "this cpu". If you reset these per task counters (time on cpu, number >>> of timeslices etc) on every migration then they become meaningless (and >>> useless). >>> >>> >>> Cheers, >>> Phil >>> >> >> Well that clarifies it! Then, let me ask the opposite question... What >> fields of schedstats should we clear when migrating? If there isn't any, >> I will just increase the number of migrations. >> > > I don't think any should be cleared on migration. They're per task and > should be monotically increasing. If they ever reset it becomes hard to > know what they mean when you read them. > > > Cheers, > Phil > > Agreed, I'll send a PATCH v2 only increasing the migration counter. >>>> So this patch will have to be upgraded if there's some other value(s) in >>>> schedstats that we do not want to reinitialize either. >>>> >>>>>> Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com> >>>>>> --- >>>>>> kernel/sched/core.c | 10 +++++++++- >>>>>> 1 file changed, 9 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c >>>>>> index fe53e510e711..d64c2a290176 100644 >>>>>> --- a/kernel/sched/core.c >>>>>> +++ b/kernel/sched/core.c >>>>>> @@ -8757,6 +8757,7 @@ bool sched_smp_initialized __read_mostly; >>>>>> int migrate_task_to(struct task_struct *p, int target_cpu) >>>>>> { >>>>>> struct migration_arg arg = { p, target_cpu }; >>>>>> + uint64_t forced_migrations, migrations_cold; >>>>>> int curr_cpu = task_cpu(p); >>>>>> >>>>>> if (curr_cpu == target_cpu) >>>>>> @@ -8765,7 +8766,14 @@ int migrate_task_to(struct task_struct *p, int target_cpu) >>>>>> if (!cpumask_test_cpu(target_cpu, p->cpus_ptr)) >>>>>> return -EINVAL; >>>>>> >>>>>> - /* TODO: This is not properly updating schedstats */ >>>>>> + if (schedstat_enabled()) { >>>>>> + forced_migrations = schedstat_val(p->stats.nr_forced_migrations); >>>>>> + migrations_cold = schedstat_val(p->stats.nr_migrations_cold); >>>>>> + memset(&p->stats, 0, sizeof(p->stats)); >>>>>> + schedstat_set(p->stats.nr_forced_migrations, forced_migrations); >>>>>> + schedstat_set(p->stats.nr_migrations_cold, migrations_cold); >>>>>> + schedstat_inc(p->stats.nr_migrations_cold); >>>>>> + } >>>>>> >>>>>> trace_sched_move_numa(p, curr_cpu, target_cpu); >>>>>> return stop_one_cpu(curr_cpu, migration_cpu_stop, &arg); >>>>>> -- >>>>>> 2.27.0 >>>>>> >>>> >>>> Thanks, >>>> Carlos >>>> >>> >> >> Thanks, >> Carlos >> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2] kernel/sched: Update schedstats when migrating threads 2022-02-23 15:59 ` Carlos Bilbao @ 2022-02-23 16:13 ` Carlos Bilbao 2022-02-24 14:03 ` Phil Auld 0 siblings, 1 reply; 12+ messages in thread From: Carlos Bilbao @ 2022-02-23 16:13 UTC (permalink / raw) To: Phil Auld, Peter Zijlstra Cc: Peter Zijlstra, juri.lelli, vincent.guittot, mingo, dietmar.eggemann, rostedt, bsegall, mgorman, bristot, linux-kernel The kernel manages per-task scheduler statistics or schedstats. Update function migrate_task_to() to increase the counter for migrations. Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com> --- Changelog: v2: Update commit message, don't reinitialize sched fields. --- kernel/sched/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kernel/sched/core.c b/kernel/sched/core.c index fcf0c180617c..1360e501c737 100644 --- a/kernel/sched/core.c +++ b/kernel/sched/core.c @@ -8751,7 +8751,7 @@ int migrate_task_to(struct task_struct *p, int target_cpu) if (!cpumask_test_cpu(target_cpu, p->cpus_ptr)) return -EINVAL; - /* TODO: This is not properly updating schedstats */ + schedstat_inc(p->stats.nr_migrations_cold); trace_sched_move_numa(p, curr_cpu, target_cpu); return stop_one_cpu(curr_cpu, migration_cpu_stop, &arg); -- 2.27.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2] kernel/sched: Update schedstats when migrating threads 2022-02-23 16:13 ` [PATCH v2] " Carlos Bilbao @ 2022-02-24 14:03 ` Phil Auld 2022-02-24 15:05 ` Carlos Bilbao 0 siblings, 1 reply; 12+ messages in thread From: Phil Auld @ 2022-02-24 14:03 UTC (permalink / raw) To: Carlos Bilbao Cc: Peter Zijlstra, juri.lelli, vincent.guittot, mingo, dietmar.eggemann, rostedt, bsegall, mgorman, bristot, linux-kernel On Wed, Feb 23, 2022 at 10:13:50AM -0600 Carlos Bilbao wrote: > The kernel manages per-task scheduler statistics or schedstats. Update > function migrate_task_to() to increase the counter for migrations. > > Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com> > --- > Changelog: > v2: Update commit message, don't reinitialize sched fields. > --- > kernel/sched/core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/kernel/sched/core.c b/kernel/sched/core.c > index fcf0c180617c..1360e501c737 100644 > --- a/kernel/sched/core.c > +++ b/kernel/sched/core.c > @@ -8751,7 +8751,7 @@ int migrate_task_to(struct task_struct *p, int target_cpu) > if (!cpumask_test_cpu(target_cpu, p->cpus_ptr)) > return -EINVAL; > > - /* TODO: This is not properly updating schedstats */ > + schedstat_inc(p->stats.nr_migrations_cold); > I was going to give a reviewed by since I was looking at this, but I can't convince myself that nr_migrations_cold is right. This looks more like a "hot" migration (using stop_cpu to force it). But nr_migrations_cold is not incremented anywhere else so maybe it's a terminology thing. Can you tell me why this is the right counter? Thanks, Phil > trace_sched_move_numa(p, curr_cpu, target_cpu); > return stop_one_cpu(curr_cpu, migration_cpu_stop, &arg); > -- > 2.27.0 > -- ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] kernel/sched: Update schedstats when migrating threads 2022-02-24 14:03 ` Phil Auld @ 2022-02-24 15:05 ` Carlos Bilbao 0 siblings, 0 replies; 12+ messages in thread From: Carlos Bilbao @ 2022-02-24 15:05 UTC (permalink / raw) To: Phil Auld, mingo Cc: Peter Zijlstra, juri.lelli, vincent.guittot, mingo, dietmar.eggemann, rostedt, bsegall, mgorman, bristot, linux-kernel On 2/24/2022 8:03 AM, Phil Auld wrote: > On Wed, Feb 23, 2022 at 10:13:50AM -0600 Carlos Bilbao wrote: >> The kernel manages per-task scheduler statistics or schedstats. Update >> function migrate_task_to() to increase the counter for migrations. >> >> Signed-off-by: Carlos Bilbao <carlos.bilbao@amd.com> >> --- >> Changelog: >> v2: Update commit message, don't reinitialize sched fields. >> --- >> kernel/sched/core.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/kernel/sched/core.c b/kernel/sched/core.c >> index fcf0c180617c..1360e501c737 100644 >> --- a/kernel/sched/core.c >> +++ b/kernel/sched/core.c >> @@ -8751,7 +8751,7 @@ int migrate_task_to(struct task_struct *p, int target_cpu) >> if (!cpumask_test_cpu(target_cpu, p->cpus_ptr)) >> return -EINVAL; >> >> - /* TODO: This is not properly updating schedstats */ >> + schedstat_inc(p->stats.nr_migrations_cold); >> > > I was going to give a reviewed by since I was looking at this, > but I can't convince myself that nr_migrations_cold is right. > This looks more like a "hot" migration (using stop_cpu to force > it). But nr_migrations_cold is not incremented anywhere else so > maybe it's a terminology thing. > > Can you tell me why this is the right counter? > > The alternative to nr_migrations_cold is nr_forced_migrations, and since we call this function in the context of NUMA balancing it didn't seem like an appropriate fit at first glance. But documentation in this is scarce and I'm not very confident with the terminology either. Maybe Ingo Molar -the original author of the structure- can clarify the specifics of these two counters so we can be sure about this? Maybe we need a new counter? > Thanks, > Phil > > >> trace_sched_move_numa(p, curr_cpu, target_cpu); >> return stop_one_cpu(curr_cpu, migration_cpu_stop, &arg); >> -- >> 2.27.0 >> > Thanks, Carlos ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-02-24 15:05 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-26 15:22 [PATCH] kernel/sched: Update schedstats when migrating threads Carlos Bilbao 2022-02-22 16:32 ` Carlos Bilbao 2022-02-22 17:15 ` Steven Rostedt 2022-02-23 10:19 ` Peter Zijlstra 2022-02-23 15:14 ` Carlos Bilbao 2022-02-23 15:28 ` Phil Auld 2022-02-23 15:33 ` Carlos Bilbao 2022-02-23 15:47 ` Phil Auld 2022-02-23 15:59 ` Carlos Bilbao 2022-02-23 16:13 ` [PATCH v2] " Carlos Bilbao 2022-02-24 14:03 ` Phil Auld 2022-02-24 15:05 ` Carlos Bilbao
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).