Intel is blessed with an abundance of monitors, one per RMID, that can be
read from any CPU in the domain. MPAMs monitors reside in the MMIO MSC,
the number implemented is up to the manufacturer. This means when there are
fewer monitors than needed, they need to be allocated and freed.
MPAM's CSU monitors are used to back the 'llc_occupancy' monitor file. The
CSU counter is allowed to return 'not ready' for a small number of
micro-seconds after programming. To allow one CSU hardware monitor to be
used for multiple control or monitor groups, the CPU accessing the
monitor needs to be able to block when configuring and reading the
counter.
Worse, the domain may be broken up into slices, and the MMIO accesses
for each slice may need performing from different CPUs.
These two details mean MPAMs monitor code needs to be able to sleep, and
IPI another CPU in the domain to read from a resource that has been sliced.
mon_event_read() already invokes mon_event_count() via IPI, which means
this isn't possible. On systems using nohz-full, some CPUs need to be
interrupted to run kernel work as they otherwise stay in user-space
running realtime workloads. Interrupting these CPUs should be avoided,
and scheduling work on them may never complete.
Change mon_event_read() to pick a housekeeping CPU, (one that is not using
nohz_full) and schedule mon_event_count() and wait. If all the CPUs
in a domain are using nohz-full, then an IPI is used as the fallback.
This function is only used in response to a user-space filesystem request
(not the timing sensitive overflow code).
This allows MPAM to hide the slice behaviour from resctrl, and to keep
the monitor-allocation in monitor.c. When the IPI fallback is used on
machines where MPAM needs to make an access on multiple CPUs, the counter
read will always fail.
Reviewed-by: Shaopeng Tan <[email protected]>
Tested-by: Shaopeng Tan <[email protected]>
Reviewed-by: Peter Newman <[email protected]>
Tested-by: Peter Newman <[email protected]>
Signed-off-by: James Morse <[email protected]>
---
Changes since v2:
* Use cpumask_any_housekeeping() and fallback to an IPI if needed.
Changes since v3:
* Actually include the IPI fallback code.
Changes since v4:
* Tinkered with existing capitalisation.
---
arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 28 +++++++++++++++++++++--
arch/x86/kernel/cpu/resctrl/monitor.c | 2 +-
2 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index b44c487727d4..bd263b9a0abd 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -19,6 +19,7 @@
#include <linux/kernfs.h>
#include <linux/seq_file.h>
#include <linux/slab.h>
+#include <linux/tick.h>
#include "internal.h"
/*
@@ -520,12 +521,24 @@ int rdtgroup_schemata_show(struct kernfs_open_file *of,
return ret;
}
+static int smp_mon_event_count(void *arg)
+{
+ mon_event_count(arg);
+
+ return 0;
+}
+
void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
struct rdt_domain *d, struct rdtgroup *rdtgrp,
int evtid, int first)
{
+ int cpu;
+
+ /* When picking a CPU from cpu_mask, ensure it can't race with cpuhp */
+ lockdep_assert_held(&rdtgroup_mutex);
+
/*
- * setup the parameters to send to the IPI to read the data.
+ * Setup the parameters to pass to mon_event_count() to read the data.
*/
rr->rgrp = rdtgrp;
rr->evtid = evtid;
@@ -534,7 +547,18 @@ void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
rr->val = 0;
rr->first = first;
- smp_call_function_any(&d->cpu_mask, mon_event_count, rr, 1);
+ cpu = cpumask_any_housekeeping(&d->cpu_mask);
+
+ /*
+ * cpumask_any_housekeeping() prefers housekeeping CPUs, but
+ * are all the CPUs nohz_full? If yes, pick a CPU to IPI.
+ * MPAM's resctrl_arch_rmid_read() is unable to read the
+ * counters on some platforms if its called in irq context.
+ */
+ if (tick_nohz_full_cpu(cpu))
+ smp_call_function_any(&d->cpu_mask, mon_event_count, rr, 1);
+ else
+ smp_call_on_cpu(cpu, smp_mon_event_count, rr, false);
}
int rdtgroup_mondata_show(struct seq_file *m, void *arg)
diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
index 993837e46db1..7749e6569a4a 100644
--- a/arch/x86/kernel/cpu/resctrl/monitor.c
+++ b/arch/x86/kernel/cpu/resctrl/monitor.c
@@ -587,7 +587,7 @@ static void mbm_bw_count(u32 closid, u32 rmid, struct rmid_read *rr)
}
/*
- * This is called via IPI to read the CQM/MBM counters
+ * This is scheduled by mon_event_read() to read the CQM/MBM counters
* on a domain.
*/
void mon_event_count(void *info)
--
2.39.2
Hi James,
On 9/14/2023 10:21 AM, James Morse wrote:
...
> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index b44c487727d4..bd263b9a0abd 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -19,6 +19,7 @@
> #include <linux/kernfs.h>
> #include <linux/seq_file.h>
> #include <linux/slab.h>
> +#include <linux/tick.h>
> #include "internal.h"
>
Please keep the empty line between groups of header files.
> /*
> @@ -520,12 +521,24 @@ int rdtgroup_schemata_show(struct kernfs_open_file *of,
> return ret;
> }
>
> +static int smp_mon_event_count(void *arg)
> +{
> + mon_event_count(arg);
> +
> + return 0;
> +}
> +
> void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
> struct rdt_domain *d, struct rdtgroup *rdtgrp,
> int evtid, int first)
> {
> + int cpu;
> +
> + /* When picking a CPU from cpu_mask, ensure it can't race with cpuhp */
This comment is not accurate at this point. It should accompany the code it applies to.
> + lockdep_assert_held(&rdtgroup_mutex);
> +
> /*
> - * setup the parameters to send to the IPI to read the data.
> + * Setup the parameters to pass to mon_event_count() to read the data.
> */
> rr->rgrp = rdtgrp;
> rr->evtid = evtid;
Reinette
Hi Reinette,
On 03/10/2023 22:17, Reinette Chatre wrote:
> On 9/14/2023 10:21 AM, James Morse wrote:
>> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> index b44c487727d4..bd263b9a0abd 100644
>> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>> @@ -19,6 +19,7 @@
>> #include <linux/kernfs.h>
>> #include <linux/seq_file.h>
>> #include <linux/slab.h>
>> +#include <linux/tick.h>
>> #include "internal.h"
>>
>
> Please keep the empty line between groups of header files.
(in this case, adding one, but sure)
>> @@ -520,12 +521,24 @@ int rdtgroup_schemata_show(struct kernfs_open_file *of,
>> return ret;
>> }
>>
>> +static int smp_mon_event_count(void *arg)
>> +{
>> + mon_event_count(arg);
>> +
>> + return 0;
>> +}
>> +
>> void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
>> struct rdt_domain *d, struct rdtgroup *rdtgrp,
>> int evtid, int first)
>> {
>> + int cpu;
>> +
>> + /* When picking a CPU from cpu_mask, ensure it can't race with cpuhp */
>
> This comment is not accurate at this point. It should accompany the code it applies to.
>
>> + lockdep_assert_held(&rdtgroup_mutex);
This refers to the d->cpu_mask calls further down this function. These are written to by
the cpuhp callbacks, rdtgroup_mutex is what prevents the cpuhp callback from running at
the same time as mon_event_read(). If that mutex weren't held, you could pick an offline CPU.
Patch 24 changes this to be lockdep_asser_cpus_held(), as the mutex is no longer used for
this purpose.
This got added here instead of patch-24 because I've added additional use of d->cpu_mask,
these things serve to document how that is safe. If you prefer I'll leave it unsaid here,
and add it with all the others in patch24.
Thanks,
James