[PATCH v1 22/31] x86/resctrl: Make resctrl_arch_pseudo_lock_fn() take a plr

James Morse posted 31 patches 1 year, 10 months ago
There is a newer version of this series
[PATCH v1 22/31] x86/resctrl: Make resctrl_arch_pseudo_lock_fn() take a plr
Posted by James Morse 1 year, 10 months ago
resctrl_arch_pseudo_lock_fn() has architecture specific behaviour,
and takes a struct rdtgroup as an argument.

After the filesystem code moves to /fs/, the definition of struct
rdtgroup will not be available to the architecture code.

The only reason resctrl_arch_pseudo_lock_fn() wants the rdtgroup is
for the CLOSID. Embed that in the pseudo_lock_region as a hw_closid,
and move the definition of struct pseudo_lock_region to resctrl.h.

Signed-off-by: James Morse <james.morse@arm.com>
---
 arch/x86/include/asm/resctrl.h            |  2 +-
 arch/x86/kernel/cpu/resctrl/internal.h    | 37 ---------------------
 arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 13 ++++----
 include/linux/resctrl.h                   | 39 +++++++++++++++++++++++
 4 files changed, 47 insertions(+), 44 deletions(-)

diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
index a88af68f9fe2..9940398e367e 100644
--- a/arch/x86/include/asm/resctrl.h
+++ b/arch/x86/include/asm/resctrl.h
@@ -212,7 +212,7 @@ static inline void resctrl_arch_mon_ctx_free(struct rdt_resource *r, int evtid,
 					     void *ctx) { };
 
 u64 resctrl_arch_get_prefetch_disable_bits(void);
-int resctrl_arch_pseudo_lock_fn(void *_rdtgrp);
+int resctrl_arch_pseudo_lock_fn(void *_plr);
 int resctrl_arch_measure_cycles_lat_fn(void *_plr);
 int resctrl_arch_measure_l2_residency(void *_plr);
 int resctrl_arch_measure_l3_residency(void *_plr);
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index d6db15839dc4..be4e8f31b127 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -182,43 +182,6 @@ struct mongroup {
 	u32			rmid;
 };
 
-/**
- * struct pseudo_lock_region - pseudo-lock region information
- * @s:			Resctrl schema for the resource to which this
- *			pseudo-locked region belongs
- * @d:			RDT domain to which this pseudo-locked region
- *			belongs
- * @cbm:		bitmask of the pseudo-locked region
- * @lock_thread_wq:	waitqueue used to wait on the pseudo-locking thread
- *			completion
- * @thread_done:	variable used by waitqueue to test if pseudo-locking
- *			thread completed
- * @cpu:		core associated with the cache on which the setup code
- *			will be run
- * @line_size:		size of the cache lines
- * @size:		size of pseudo-locked region in bytes
- * @kmem:		the kernel memory associated with pseudo-locked region
- * @minor:		minor number of character device associated with this
- *			region
- * @debugfs_dir:	pointer to this region's directory in the debugfs
- *			filesystem
- * @pm_reqs:		Power management QoS requests related to this region
- */
-struct pseudo_lock_region {
-	struct resctrl_schema	*s;
-	struct rdt_domain	*d;
-	u32			cbm;
-	wait_queue_head_t	lock_thread_wq;
-	int			thread_done;
-	int			cpu;
-	unsigned int		line_size;
-	unsigned int		size;
-	void			*kmem;
-	unsigned int		minor;
-	struct dentry		*debugfs_dir;
-	struct list_head	pm_reqs;
-};
-
 /**
  * struct rdtgroup - store rdtgroup's data in resctrl file system.
  * @kn:				kernfs node
diff --git a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
index 5a66e3b2c2ea..ba51ab1f70e6 100644
--- a/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
+++ b/arch/x86/kernel/cpu/resctrl/pseudo_lock.c
@@ -416,7 +416,7 @@ static void pseudo_lock_free(struct rdtgroup *rdtgrp)
 
 /**
  * resctrl_arch_pseudo_lock_fn - Load kernel memory into cache
- * @_rdtgrp: resource group to which pseudo-lock region belongs
+ * @_plr: the pseudo-lock region descriptor
  *
  * This is the core pseudo-locking flow.
  *
@@ -433,10 +433,9 @@ static void pseudo_lock_free(struct rdtgroup *rdtgrp)
  *
  * Return: 0. Waiter on waitqueue will be woken on completion.
  */
-int resctrl_arch_pseudo_lock_fn(void *_rdtgrp)
+int resctrl_arch_pseudo_lock_fn(void *_plr)
 {
-	struct rdtgroup *rdtgrp = _rdtgrp;
-	struct pseudo_lock_region *plr = rdtgrp->plr;
+	struct pseudo_lock_region *plr = _plr;
 	u32 rmid_p, closid_p;
 	unsigned long i;
 	u64 saved_msr;
@@ -496,7 +495,8 @@ int resctrl_arch_pseudo_lock_fn(void *_rdtgrp)
 	 * pseudo-locked followed by reading of kernel memory to load it
 	 * into the cache.
 	 */
-	__wrmsr(MSR_IA32_PQR_ASSOC, rmid_p, rdtgrp->closid);
+	__wrmsr(MSR_IA32_PQR_ASSOC, rmid_p, plr->closid);
+
 	/*
 	 * Cache was flushed earlier. Now access kernel memory to read it
 	 * into cache region associated with just activated plr->closid.
@@ -1327,7 +1327,8 @@ int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp)
 
 	plr->thread_done = 0;
 
-	thread = kthread_create_on_node(resctrl_arch_pseudo_lock_fn, rdtgrp,
+	plr->closid = rdtgrp->closid;
+	thread = kthread_create_on_node(resctrl_arch_pseudo_lock_fn, plr,
 					cpu_to_node(plr->cpu),
 					"pseudo_lock/%u", plr->cpu);
 	if (IS_ERR(thread)) {
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 6705d7960dfd..3de5bc63ace0 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -25,6 +25,45 @@ int proc_resctrl_show(struct seq_file *m,
 /* max value for struct rdt_domain's mbps_val */
 #define MBA_MAX_MBPS   U32_MAX
 
+/**
+ * struct pseudo_lock_region - pseudo-lock region information
+ * @s:			Resctrl schema for the resource to which this
+ *			pseudo-locked region belongs
+ * @closid:		The closid that this pseudo-locked region uses
+ * @d:			RDT domain to which this pseudo-locked region
+ *			belongs
+ * @cbm:		bitmask of the pseudo-locked region
+ * @lock_thread_wq:	waitqueue used to wait on the pseudo-locking thread
+ *			completion
+ * @thread_done:	variable used by waitqueue to test if pseudo-locking
+ *			thread completed
+ * @cpu:		core associated with the cache on which the setup code
+ *			will be run
+ * @line_size:		size of the cache lines
+ * @size:		size of pseudo-locked region in bytes
+ * @kmem:		the kernel memory associated with pseudo-locked region
+ * @minor:		minor number of character device associated with this
+ *			region
+ * @debugfs_dir:	pointer to this region's directory in the debugfs
+ *			filesystem
+ * @pm_reqs:		Power management QoS requests related to this region
+ */
+struct pseudo_lock_region {
+	struct resctrl_schema	*s;
+	u32			closid;
+	struct rdt_domain	*d;
+	u32			cbm;
+	wait_queue_head_t	lock_thread_wq;
+	int			thread_done;
+	int			cpu;
+	unsigned int		line_size;
+	unsigned int		size;
+	void			*kmem;
+	unsigned int		minor;
+	struct dentry		*debugfs_dir;
+	struct list_head	pm_reqs;
+};
+
 /**
  * struct resctrl_staged_config - parsed configuration to be applied
  * @new_ctrl:		new ctrl value to be loaded
-- 
2.39.2
Re: [PATCH v1 22/31] x86/resctrl: Make resctrl_arch_pseudo_lock_fn() take a plr
Posted by Reinette Chatre 1 year, 10 months ago
Hi James,

On 3/21/2024 9:50 AM, James Morse wrote:
> resctrl_arch_pseudo_lock_fn() has architecture specific behaviour,
> and takes a struct rdtgroup as an argument.
> 
> After the filesystem code moves to /fs/, the definition of struct
> rdtgroup will not be available to the architecture code.
> 
> The only reason resctrl_arch_pseudo_lock_fn() wants the rdtgroup is
> for the CLOSID. Embed that in the pseudo_lock_region as a hw_closid,

Above creates expectation that the new member will be named hw_closid,
but that is not what the code does.

> and move the definition of struct pseudo_lock_region to resctrl.h.
> 
> Signed-off-by: James Morse <james.morse@arm.com>

...

> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 6705d7960dfd..3de5bc63ace0 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -25,6 +25,45 @@ int proc_resctrl_show(struct seq_file *m,
>  /* max value for struct rdt_domain's mbps_val */
>  #define MBA_MAX_MBPS   U32_MAX
>  
> +/**
> + * struct pseudo_lock_region - pseudo-lock region information
> + * @s:			Resctrl schema for the resource to which this
> + *			pseudo-locked region belongs
> + * @closid:		The closid that this pseudo-locked region uses
> + * @d:			RDT domain to which this pseudo-locked region
> + *			belongs
> + * @cbm:		bitmask of the pseudo-locked region
> + * @lock_thread_wq:	waitqueue used to wait on the pseudo-locking thread
> + *			completion
> + * @thread_done:	variable used by waitqueue to test if pseudo-locking
> + *			thread completed
> + * @cpu:		core associated with the cache on which the setup code
> + *			will be run
> + * @line_size:		size of the cache lines
> + * @size:		size of pseudo-locked region in bytes
> + * @kmem:		the kernel memory associated with pseudo-locked region
> + * @minor:		minor number of character device associated with this
> + *			region
> + * @debugfs_dir:	pointer to this region's directory in the debugfs
> + *			filesystem
> + * @pm_reqs:		Power management QoS requests related to this region
> + */
> +struct pseudo_lock_region {
> +	struct resctrl_schema	*s;
> +	u32			closid;
> +	struct rdt_domain	*d;
> +	u32			cbm;
> +	wait_queue_head_t	lock_thread_wq;
> +	int			thread_done;
> +	int			cpu;
> +	unsigned int		line_size;
> +	unsigned int		size;
> +	void			*kmem;
> +	unsigned int		minor;
> +	struct dentry		*debugfs_dir;
> +	struct list_head	pm_reqs;
> +};
> +
>  /**
>   * struct resctrl_staged_config - parsed configuration to be applied
>   * @new_ctrl:		new ctrl value to be loaded


Reinette
Re: [PATCH v1 22/31] x86/resctrl: Make resctrl_arch_pseudo_lock_fn() take a plr
Posted by Dave Martin 1 year, 10 months ago
On Mon, Apr 08, 2024 at 08:24:35PM -0700, Reinette Chatre wrote:
> Hi James,
> 
> On 3/21/2024 9:50 AM, James Morse wrote:
> > resctrl_arch_pseudo_lock_fn() has architecture specific behaviour,
> > and takes a struct rdtgroup as an argument.
> > 
> > After the filesystem code moves to /fs/, the definition of struct
> > rdtgroup will not be available to the architecture code.
> > 
> > The only reason resctrl_arch_pseudo_lock_fn() wants the rdtgroup is
> > for the CLOSID. Embed that in the pseudo_lock_region as a hw_closid,
> 
> Above creates expectation that the new member will be named hw_closid,
> but that is not what the code does.

I'll flag this for review, but I'd guess that this can probably just be
"closid".  I'll make a note to consider what needs to change to make
things consistent between the patch and commit message.

James might have had other ideas, connected with the remapping done for
CDP emulation causing the resctrl closid being different from the actual
value used by the hardware, at least for MPAM (see my response on
patch 24).  I don't fully understand how this works for x86 though.

So long as functionality is unaffected, and this patch is introducing no
new confusion that wasn't there beforehand, the exact name may not
matter too much(?)

Did you have other concerns here?

Cheers
---Dave
Re: [PATCH v1 22/31] x86/resctrl: Make resctrl_arch_pseudo_lock_fn() take a plr
Posted by Reinette Chatre 1 year, 10 months ago
Hi Dave,

On 4/11/2024 7:38 AM, Dave Martin wrote:
> On Mon, Apr 08, 2024 at 08:24:35PM -0700, Reinette Chatre wrote:
>> Hi James,
>>
>> On 3/21/2024 9:50 AM, James Morse wrote:
>>> resctrl_arch_pseudo_lock_fn() has architecture specific behaviour,
>>> and takes a struct rdtgroup as an argument.
>>>
>>> After the filesystem code moves to /fs/, the definition of struct
>>> rdtgroup will not be available to the architecture code.
>>>
>>> The only reason resctrl_arch_pseudo_lock_fn() wants the rdtgroup is
>>> for the CLOSID. Embed that in the pseudo_lock_region as a hw_closid,
>>
>> Above creates expectation that the new member will be named hw_closid,
>> but that is not what the code does.
> 
> I'll flag this for review, but I'd guess that this can probably just be
> "closid".  I'll make a note to consider what needs to change to make
> things consistent between the patch and commit message.
> 
> James might have had other ideas, connected with the remapping done for
> CDP emulation causing the resctrl closid being different from the actual
> value used by the hardware, at least for MPAM (see my response on
> patch 24).  I don't fully understand how this works for x86 though.
> 
> So long as functionality is unaffected, and this patch is introducing no
> new confusion that wasn't there beforehand, the exact name may not
> matter too much(?)

closid sounds good. It may be a good match for what is expected to be in
general/fs code. 

> 
> Did you have other concerns here?

No.

Reinette