on_each_cpu_mask() runs the function on each CPU specified by cpumask,
which may include the local processor.
Replace smp_call_function_many() with on_each_cpu_mask() to simplify
the code.
Reviewed-by: Reinette Chatre <[email protected]>
Signed-off-by: Babu Moger <[email protected]>
---
arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 11 +++------
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 29 +++++++----------------
2 files changed, 11 insertions(+), 29 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index 1df0e3262bca..7eece3d2d0c3 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -310,7 +310,6 @@ int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid)
enum resctrl_conf_type t;
cpumask_var_t cpu_mask;
struct rdt_domain *d;
- int cpu;
u32 idx;
if (!zalloc_cpumask_var(&cpu_mask, GFP_KERNEL))
@@ -341,13 +340,9 @@ int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid)
if (cpumask_empty(cpu_mask))
goto done;
- cpu = get_cpu();
- /* Update resource control msr on this CPU if it's in cpu_mask. */
- if (cpumask_test_cpu(cpu, cpu_mask))
- rdt_ctrl_update(&msr_param);
- /* Update resource control msr on other CPUs. */
- smp_call_function_many(cpu_mask, rdt_ctrl_update, &msr_param, 1);
- put_cpu();
+
+ /* Update resource control msr on all the CPUs. */
+ on_each_cpu_mask(cpu_mask, rdt_ctrl_update, &msr_param, 1);
done:
free_cpumask_var(cpu_mask);
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index e5a48f05e787..e4e6cdc1ee62 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -325,12 +325,7 @@ static void update_cpu_closid_rmid(void *info)
static void
update_closid_rmid(const struct cpumask *cpu_mask, struct rdtgroup *r)
{
- int cpu = get_cpu();
-
- if (cpumask_test_cpu(cpu, cpu_mask))
- update_cpu_closid_rmid(r);
- smp_call_function_many(cpu_mask, update_cpu_closid_rmid, r, 1);
- put_cpu();
+ on_each_cpu_mask(cpu_mask, update_cpu_closid_rmid, r, 1);
}
static int cpus_mon_write(struct rdtgroup *rdtgrp, cpumask_var_t newmask,
@@ -1864,13 +1859,9 @@ static int set_cache_qos_cfg(int level, bool enable)
/* Pick one CPU from each domain instance to update MSR */
cpumask_set_cpu(cpumask_any(&d->cpu_mask), cpu_mask);
}
- cpu = get_cpu();
- /* Update QOS_CFG MSR on this cpu if it's in cpu_mask. */
- if (cpumask_test_cpu(cpu, cpu_mask))
- update(&enable);
- /* Update QOS_CFG MSR on all other cpus in cpu_mask. */
- smp_call_function_many(cpu_mask, update, &enable, 1);
- put_cpu();
+
+ /* Update QOS_CFG MSR on all the CPUs in cpu_mask */
+ on_each_cpu_mask(cpu_mask, update, &enable, 1);
free_cpumask_var(cpu_mask);
@@ -2347,7 +2338,7 @@ static int reset_all_ctrls(struct rdt_resource *r)
struct msr_param msr_param;
cpumask_var_t cpu_mask;
struct rdt_domain *d;
- int i, cpu;
+ int i;
if (!zalloc_cpumask_var(&cpu_mask, GFP_KERNEL))
return -ENOMEM;
@@ -2368,13 +2359,9 @@ static int reset_all_ctrls(struct rdt_resource *r)
for (i = 0; i < hw_res->num_closid; i++)
hw_dom->ctrl_val[i] = r->default_ctrl;
}
- cpu = get_cpu();
- /* Update CBM on this cpu if it's in cpu_mask. */
- if (cpumask_test_cpu(cpu, cpu_mask))
- rdt_ctrl_update(&msr_param);
- /* Update CBM on all other cpus in cpu_mask. */
- smp_call_function_many(cpu_mask, rdt_ctrl_update, &msr_param, 1);
- put_cpu();
+
+ /* Update CBM on all the CPUs in cpu_mask */
+ on_each_cpu_mask(cpu_mask, rdt_ctrl_update, &msr_param, 1);
free_cpumask_var(cpu_mask);
--
2.34.1
On Mon, Jan 09, 2023 at 10:43:53AM -0600, Babu Moger wrote:
> on_each_cpu_mask() runs the function on each CPU specified by cpumask,
> which may include the local processor.
>
> Replace smp_call_function_many() with on_each_cpu_mask() to simplify
> the code.
>
> Reviewed-by: Reinette Chatre <[email protected]>
> Signed-off-by: Babu Moger <[email protected]>
> ---
> arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 11 +++------
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 29 +++++++----------------
> 2 files changed, 11 insertions(+), 29 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index 1df0e3262bca..7eece3d2d0c3 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -310,7 +310,6 @@ int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid)
> enum resctrl_conf_type t;
> cpumask_var_t cpu_mask;
> struct rdt_domain *d;
> - int cpu;
> u32 idx;
>
> if (!zalloc_cpumask_var(&cpu_mask, GFP_KERNEL))
> @@ -341,13 +340,9 @@ int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid)
>
> if (cpumask_empty(cpu_mask))
> goto done;
> - cpu = get_cpu();
> - /* Update resource control msr on this CPU if it's in cpu_mask. */
> - if (cpumask_test_cpu(cpu, cpu_mask))
> - rdt_ctrl_update(&msr_param);
> - /* Update resource control msr on other CPUs. */
> - smp_call_function_many(cpu_mask, rdt_ctrl_update, &msr_param, 1);
> - put_cpu();
> +
> + /* Update resource control msr on all the CPUs. */
> + on_each_cpu_mask(cpu_mask, rdt_ctrl_update, &msr_param, 1);
Do you require these updates to done immediately via an IPI? or can they be
done bit lazy via schedule_on_each_cpu()?
Cheers,
Ashok
[AMD Official Use Only - General]
Hi Ashok,
> -----Original Message-----
> From: Ashok Raj <[email protected]>
> Sent: Monday, January 9, 2023 5:27 PM
> To: Moger, Babu <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; Das1, Sandipan <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; Ashok Raj
> <[email protected]>
> Subject: Re: [PATCH v11 01/13] x86/resctrl: Replace smp_call_function_many()
> with on_each_cpu_mask()
>
> On Mon, Jan 09, 2023 at 10:43:53AM -0600, Babu Moger wrote:
> > on_each_cpu_mask() runs the function on each CPU specified by cpumask,
> > which may include the local processor.
> >
> > Replace smp_call_function_many() with on_each_cpu_mask() to simplify
> > the code.
> >
> > Reviewed-by: Reinette Chatre <[email protected]>
> > Signed-off-by: Babu Moger <[email protected]>
> > ---
> > arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 11 +++------
> > arch/x86/kernel/cpu/resctrl/rdtgroup.c | 29 +++++++----------------
> > 2 files changed, 11 insertions(+), 29 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> > b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> > index 1df0e3262bca..7eece3d2d0c3 100644
> > --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> > +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> > @@ -310,7 +310,6 @@ int resctrl_arch_update_domains(struct rdt_resource
> *r, u32 closid)
> > enum resctrl_conf_type t;
> > cpumask_var_t cpu_mask;
> > struct rdt_domain *d;
> > - int cpu;
> > u32 idx;
> >
> > if (!zalloc_cpumask_var(&cpu_mask, GFP_KERNEL)) @@ -341,13
> +340,9 @@
> > int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid)
> >
> > if (cpumask_empty(cpu_mask))
> > goto done;
> > - cpu = get_cpu();
> > - /* Update resource control msr on this CPU if it's in cpu_mask. */
> > - if (cpumask_test_cpu(cpu, cpu_mask))
> > - rdt_ctrl_update(&msr_param);
> > - /* Update resource control msr on other CPUs. */
> > - smp_call_function_many(cpu_mask, rdt_ctrl_update, &msr_param, 1);
> > - put_cpu();
> > +
> > + /* Update resource control msr on all the CPUs. */
> > + on_each_cpu_mask(cpu_mask, rdt_ctrl_update, &msr_param, 1);
>
> Do you require these updates to done immediately via an IPI? or can they be
> done bit lazy via schedule_on_each_cpu()?
I have not experimented with lazy schedule. At least I know the call update_cpu_closid_rmid should be completed immediately. Otherwise, the result might be inconsistent as the tasks(or CPUs) could be running on two different closed/rmids before it is updated on all CPUs in the domain.
Thanks
Babu
> > > + /* Update resource control msr on all the CPUs. */
> > > + on_each_cpu_mask(cpu_mask, rdt_ctrl_update, &msr_param, 1);
> >
> > Do you require these updates to done immediately via an IPI? or can they be
> > done bit lazy via schedule_on_each_cpu()?
>
> I have not experimented with lazy schedule. At least I know the call
> update_cpu_closid_rmid should be completed immediately. Otherwise, the
> result might be inconsistent as the tasks(or CPUs) could be running on
> two different closed/rmids before it is updated on all CPUs in the domain.
I think this does need to happen somewhat urgently. Imagine trying to give
some extra resources to a CPU bound real-time process. That process will
keep running with the old resource allocation.
-Tony
On Tue, Jan 10, 2023 at 12:58:47PM -0800, Tony Luck wrote:
> > > > + /* Update resource control msr on all the CPUs. */
> > > > + on_each_cpu_mask(cpu_mask, rdt_ctrl_update, &msr_param, 1);
> > >
> > > Do you require these updates to done immediately via an IPI? or can they be
> > > done bit lazy via schedule_on_each_cpu()?
> >
> > I have not experimented with lazy schedule. At least I know the call
> > update_cpu_closid_rmid should be completed immediately. Otherwise, the
> > result might be inconsistent as the tasks(or CPUs) could be running on
> > two different closed/rmids before it is updated on all CPUs in the domain.
>
> I think this does need to happen somewhat urgently. Imagine trying to give
> some extra resources to a CPU bound real-time process. That process will
> keep running with the old resource allocation.
If the resctl was setup before spawning other threads then the thread
starts with the right values from the start, probably inheriting from the
parent?
I wasn't sure if the few ms difference is going to make much material
difference for that process. IPI's does shake things up and introduces
other overheads not related to this process.
Instead of victimizing just this process, we hurt everything else.
Does it make sense to do an experiment and see if there is any other
functional failures?
Cheers,
Ashok
> I wasn't sure if the few ms difference is going to make much material
> difference for that process. IPI's does shake things up and introduces
> other overheads not related to this process.
Is it just a few milli-seconds? What is the scheduler priority of the kernel
thread you wake to perform this action? How does that compare to the
priority of a RT thread? I may be wrong here, but I think an RT thread can
block a kernel thread from running indefinitely.
-Tony
On Wed, Jan 11, 2023 at 11:05:48AM -0800, Tony Luck wrote:
> > I wasn't sure if the few ms difference is going to make much material
> > difference for that process. IPI's does shake things up and introduces
> > other overheads not related to this process.
>
> Is it just a few milli-seconds? What is the scheduler priority of the kernel
> thread you wake to perform this action? How does that compare to the
> priority of a RT thread? I may be wrong here, but I think an RT thread can
> block a kernel thread from running indefinitely.
That's a good point. I don't have an idea about how the kernel thread
priority is compared to user space RT threads.
I assumed that since these threads have some work to be done from kernel,
user space shouldn't get higher priority than kernel tasks.
Uncharted territory for me ... Just a hunch :)
Maybe @peterz can screw my head in the right direction :-)