[PATCH v1 04/31] x86/resctrl: Add helper for setting CPU default properties

James Morse posted 31 patches 1 year, 10 months ago
There is a newer version of this series
[PATCH v1 04/31] x86/resctrl: Add helper for setting CPU default properties
Posted by James Morse 1 year, 10 months ago
rdtgroup_rmdir_ctrl() and rdtgroup_rmdir_mon() set the per-CPU
pqr_state for CPUs that were part of the rmdir()'d group.

Another architecture might not have a 'pqr_state', its hardware may
need the values in a different format. MPAM's equivalent of RMID values
are not unique, and always need the CLOSID to be provided too.

There is only one caller that modifies a single value,
(rdtgroup_rmdir_mon()). MPAM always needs both CLOSID and RMID
for the hardware value as these are written to the same system
register.

As rdtgroup_rmdir_mon() has the CLOSID on hand, only provide a
helper to set both values. These values are read by
__resctrl_sched_in(), but may be written by a different CPU without
any locking, add READ/WRTE_ONCE() to avoid torn values.

Signed-off-by: James Morse <james.morse@arm.com>
---
 arch/x86/include/asm/resctrl.h         | 14 +++++++++++---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 15 ++++++++++-----
 2 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
index 12dbd2588ca7..f61382258743 100644
--- a/arch/x86/include/asm/resctrl.h
+++ b/arch/x86/include/asm/resctrl.h
@@ -4,8 +4,9 @@
 
 #ifdef CONFIG_X86_CPU_RESCTRL
 
-#include <linux/sched.h>
 #include <linux/jump_label.h>
+#include <linux/percpu.h>
+#include <linux/sched.h>
 
 /*
  * This value can never be a valid CLOSID, and is used when mapping a
@@ -96,8 +97,8 @@ static inline void resctrl_arch_disable_mon(void)
 static inline void __resctrl_sched_in(struct task_struct *tsk)
 {
 	struct resctrl_pqr_state *state = this_cpu_ptr(&pqr_state);
-	u32 closid = state->default_closid;
-	u32 rmid = state->default_rmid;
+	u32 closid = READ_ONCE(state->default_closid);
+	u32 rmid = READ_ONCE(state->default_rmid);
 	u32 tmp;
 
 	/*
@@ -132,6 +133,13 @@ static inline unsigned int resctrl_arch_round_mon_val(unsigned int val)
 	return val * scale;
 }
 
+static inline void resctrl_arch_set_cpu_default_closid_rmid(int cpu, u32 closid,
+							    u32 rmid)
+{
+	WRITE_ONCE(per_cpu(pqr_state.default_closid, cpu), closid);
+	WRITE_ONCE(per_cpu(pqr_state.default_rmid, cpu), rmid);
+}
+
 static inline void resctrl_arch_set_closid_rmid(struct task_struct *tsk,
 						u32 closid, u32 rmid)
 {
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 45372b6a6215..5d2c1ce5b6b1 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -3623,14 +3623,18 @@ static int rdtgroup_mkdir(struct kernfs_node *parent_kn, const char *name,
 static int rdtgroup_rmdir_mon(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
 {
 	struct rdtgroup *prdtgrp = rdtgrp->mon.parent;
+	u32 closid, rmid;
 	int cpu;
 
 	/* Give any tasks back to the parent group */
 	rdt_move_group_tasks(rdtgrp, prdtgrp, tmpmask);
 
 	/* Update per cpu rmid of the moved CPUs first */
+	closid = rdtgrp->closid;
+	rmid = prdtgrp->mon.rmid;
 	for_each_cpu(cpu, &rdtgrp->cpu_mask)
-		per_cpu(pqr_state.default_rmid, cpu) = prdtgrp->mon.rmid;
+		resctrl_arch_set_cpu_default_closid_rmid(cpu, closid, rmid);
+
 	/*
 	 * Update the MSR on moved CPUs and CPUs which have moved
 	 * task running on them.
@@ -3663,6 +3667,7 @@ static int rdtgroup_ctrl_remove(struct rdtgroup *rdtgrp)
 
 static int rdtgroup_rmdir_ctrl(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
 {
+	u32 closid, rmid;
 	int cpu;
 
 	/* Give any tasks back to the default group */
@@ -3673,10 +3678,10 @@ static int rdtgroup_rmdir_ctrl(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
 		   &rdtgroup_default.cpu_mask, &rdtgrp->cpu_mask);
 
 	/* Update per cpu closid and rmid of the moved CPUs first */
-	for_each_cpu(cpu, &rdtgrp->cpu_mask) {
-		per_cpu(pqr_state.default_closid, cpu) = rdtgroup_default.closid;
-		per_cpu(pqr_state.default_rmid, cpu) = rdtgroup_default.mon.rmid;
-	}
+	closid = rdtgroup_default.closid;
+	rmid = rdtgroup_default.mon.rmid;
+	for_each_cpu(cpu, &rdtgrp->cpu_mask)
+		resctrl_arch_set_cpu_default_closid_rmid(cpu, closid, rmid);
 
 	/*
 	 * Update the MSR on moved CPUs and CPUs which have moved
-- 
2.39.2
Re: [PATCH v1 04/31] x86/resctrl: Add helper for setting CPU default properties
Posted by Reinette Chatre 1 year, 10 months ago
Hi James,

On 3/21/2024 9:50 AM, James Morse wrote:

> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -3623,14 +3623,18 @@ static int rdtgroup_mkdir(struct kernfs_node *parent_kn, const char *name,
>  static int rdtgroup_rmdir_mon(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
>  {
>  	struct rdtgroup *prdtgrp = rdtgrp->mon.parent;
> +	u32 closid, rmid;
>  	int cpu;
>  
>  	/* Give any tasks back to the parent group */
>  	rdt_move_group_tasks(rdtgrp, prdtgrp, tmpmask);
>  
>  	/* Update per cpu rmid of the moved CPUs first */
> +	closid = rdtgrp->closid;
> +	rmid = prdtgrp->mon.rmid;
>  	for_each_cpu(cpu, &rdtgrp->cpu_mask)
> -		per_cpu(pqr_state.default_rmid, cpu) = prdtgrp->mon.rmid;
> +		resctrl_arch_set_cpu_default_closid_rmid(cpu, closid, rmid);
> +

While I understand that the CLOSIDs are the same, I do think it looks unexpected
for the CLOSID to be set to the CLOSID of the group being removed. Could this
be set to CLOSID of parent group instead?

Reinette
Re: [PATCH v1 04/31] x86/resctrl: Add helper for setting CPU default properties
Posted by Dave Martin 1 year, 10 months ago
On Mon, Apr 08, 2024 at 08:15:03PM -0700, Reinette Chatre wrote:
> Hi James,
> 
> On 3/21/2024 9:50 AM, James Morse wrote:
> 
> > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > @@ -3623,14 +3623,18 @@ static int rdtgroup_mkdir(struct kernfs_node *parent_kn, const char *name,
> >  static int rdtgroup_rmdir_mon(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
> >  {
> >  	struct rdtgroup *prdtgrp = rdtgrp->mon.parent;
> > +	u32 closid, rmid;
> >  	int cpu;
> >  
> >  	/* Give any tasks back to the parent group */
> >  	rdt_move_group_tasks(rdtgrp, prdtgrp, tmpmask);
> >  
> >  	/* Update per cpu rmid of the moved CPUs first */
> > +	closid = rdtgrp->closid;
> > +	rmid = prdtgrp->mon.rmid;
> >  	for_each_cpu(cpu, &rdtgrp->cpu_mask)
> > -		per_cpu(pqr_state.default_rmid, cpu) = prdtgrp->mon.rmid;
> > +		resctrl_arch_set_cpu_default_closid_rmid(cpu, closid, rmid);
> > +
> 
> While I understand that the CLOSIDs are the same, I do think it looks unexpected
> for the CLOSID to be set to the CLOSID of the group being removed. Could this
> be set to CLOSID of parent group instead?
> 
> Reinette

That seems reasonable.  How about something like this?

-	closid = rdtgrp->closid;
+	closid = prdtgrp->closid; /* no change, but the arch code needs it */

Cheers
---Dave
Re: [PATCH v1 04/31] x86/resctrl: Add helper for setting CPU default properties
Posted by Reinette Chatre 1 year, 9 months ago
Hi Dave,

On 4/12/2024 9:13 AM, Dave Martin wrote:
> On Mon, Apr 08, 2024 at 08:15:03PM -0700, Reinette Chatre wrote:
>> Hi James,
>>
>> On 3/21/2024 9:50 AM, James Morse wrote:
>>
>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> @@ -3623,14 +3623,18 @@ static int rdtgroup_mkdir(struct kernfs_node *parent_kn, const char *name,
>>>  static int rdtgroup_rmdir_mon(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
>>>  {
>>>  	struct rdtgroup *prdtgrp = rdtgrp->mon.parent;
>>> +	u32 closid, rmid;
>>>  	int cpu;
>>>  
>>>  	/* Give any tasks back to the parent group */
>>>  	rdt_move_group_tasks(rdtgrp, prdtgrp, tmpmask);
>>>  
>>>  	/* Update per cpu rmid of the moved CPUs first */
>>> +	closid = rdtgrp->closid;
>>> +	rmid = prdtgrp->mon.rmid;
>>>  	for_each_cpu(cpu, &rdtgrp->cpu_mask)
>>> -		per_cpu(pqr_state.default_rmid, cpu) = prdtgrp->mon.rmid;
>>> +		resctrl_arch_set_cpu_default_closid_rmid(cpu, closid, rmid);
>>> +
>>
>> While I understand that the CLOSIDs are the same, I do think it looks unexpected
>> for the CLOSID to be set to the CLOSID of the group being removed. Could this
>> be set to CLOSID of parent group instead?
>>
>> Reinette
> 
> That seems reasonable.  How about something like this?
> 
> -	closid = rdtgrp->closid;
> +	closid = prdtgrp->closid; /* no change, but the arch code needs it */

Looks good. If the comment stays, please do replace the tail comment with a
freestanding comment (for reference you can search for "No tail comments"
in Documentation/process/maintainer-tip.rst). 

Thank you

Reinette
Re: [PATCH v1 04/31] x86/resctrl: Add helper for setting CPU default properties
Posted by Dave Martin 1 year, 9 months ago
On Mon, Apr 15, 2024 at 10:45:07AM -0700, Reinette Chatre wrote:
> Hi Dave,
> 
> On 4/12/2024 9:13 AM, Dave Martin wrote:
> > On Mon, Apr 08, 2024 at 08:15:03PM -0700, Reinette Chatre wrote:
> >> Hi James,
> >>
> >> On 3/21/2024 9:50 AM, James Morse wrote:
> >>
> >>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> >>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> >>> @@ -3623,14 +3623,18 @@ static int rdtgroup_mkdir(struct kernfs_node *parent_kn, const char *name,
> >>>  static int rdtgroup_rmdir_mon(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
> >>>  {
> >>>  	struct rdtgroup *prdtgrp = rdtgrp->mon.parent;
> >>> +	u32 closid, rmid;
> >>>  	int cpu;
> >>>  
> >>>  	/* Give any tasks back to the parent group */
> >>>  	rdt_move_group_tasks(rdtgrp, prdtgrp, tmpmask);
> >>>  
> >>>  	/* Update per cpu rmid of the moved CPUs first */
> >>> +	closid = rdtgrp->closid;
> >>> +	rmid = prdtgrp->mon.rmid;
> >>>  	for_each_cpu(cpu, &rdtgrp->cpu_mask)
> >>> -		per_cpu(pqr_state.default_rmid, cpu) = prdtgrp->mon.rmid;
> >>> +		resctrl_arch_set_cpu_default_closid_rmid(cpu, closid, rmid);
> >>> +
> >>
> >> While I understand that the CLOSIDs are the same, I do think it looks unexpected
> >> for the CLOSID to be set to the CLOSID of the group being removed. Could this
> >> be set to CLOSID of parent group instead?
> >>
> >> Reinette
> > 
> > That seems reasonable.  How about something like this?
> > 
> > -	closid = rdtgrp->closid;
> > +	closid = prdtgrp->closid; /* no change, but the arch code needs it */
> 
> Looks good. If the comment stays, please do replace the tail comment with a
> freestanding comment (for reference you can search for "No tail comments"
> in Documentation/process/maintainer-tip.rst). 

Ack, noted.

Cheers
---Dave