update_cpu_closid_rmid() takes a struct rdtgroup as an argument, which
it uses to update the local CPUs default pqr values. This is a problem
once the resctrl parts move out to /fs/, as the arch code cannot
poke around inside struct rdtgroup.

Rename update_cpu_closid_rmid() as resctrl_arch_sync_cpus_defaults()
to be used as the target of an IPI, and pass the effective CLOSID
and RMID in a new struct.

Signed-off-by: James Morse <james.morse@arm.com>
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 19 +++++++++++++++----
 include/linux/resctrl.h                | 11 +++++++++++
 2 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 5d2c1ce5b6b1..18f097fce51e 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -341,13 +341,13 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of,
  * from update_closid_rmid() is protected against __switch_to() because
  * preemption is disabled.
  */
-static void update_cpu_closid_rmid(void *info)
+void resctrl_arch_sync_cpu_defaults(void *info)
 {
-	struct rdtgroup *r = info;
+	struct resctrl_cpu_sync *r = info;
 
 	if (r) {
 		this_cpu_write(pqr_state.default_closid, r->closid);
-		this_cpu_write(pqr_state.default_rmid, r->mon.rmid);
+		this_cpu_write(pqr_state.default_rmid, r->rmid);
 	}
 
 	/*
@@ -362,11 +362,22 @@ static void update_cpu_closid_rmid(void *info)
  * Update the PGR_ASSOC MSR on all cpus in @cpu_mask,
  *
  * Per task closids/rmids must have been set up before calling this function.
+ * @r may be NULL.
  */
 static void
 update_closid_rmid(const struct cpumask *cpu_mask, struct rdtgroup *r)
 {
-	on_each_cpu_mask(cpu_mask, update_cpu_closid_rmid, r, 1);
+	struct resctrl_cpu_sync defaults;
+	struct resctrl_cpu_sync *defaults_p = NULL;
+
+	if (r) {
+		defaults.closid = r->closid;
+		defaults.rmid = r->mon.rmid;
+		defaults_p = &defaults;
+	}
+
+	on_each_cpu_mask(cpu_mask, resctrl_arch_sync_cpu_defaults, defaults_p,
+			 1);
 }
 
 static int cpus_mon_write(struct rdtgroup *rdtgrp, cpumask_var_t newmask,
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 6e87bc95f5ea..2b79e4159507 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -220,6 +220,17 @@ struct resctrl_schema {
 	u32				num_closid;
 };
 
+struct resctrl_cpu_sync {
+	u32 closid;
+	u32 rmid;
+};
+
+/*
+ * Update and re-load this CPUs defaults. Called via IPI, takes a pointer to
+ * struct resctrl_cpu_sync, or NULL.
+ */
+void resctrl_arch_sync_cpu_defaults(void *info);
+
 /* The number of closid supported by this resource regardless of CDP */
 u32 resctrl_arch_get_num_closid(struct rdt_resource *r);
 int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid);
-- 
2.39.2

Hi James/Dave,

On 3/21/24 11:50, James Morse wrote:
> update_cpu_closid_rmid() takes a struct rdtgroup as an argument, which
> it uses to update the local CPUs default pqr values. This is a problem
> once the resctrl parts move out to /fs/, as the arch code cannot
> poke around inside struct rdtgroup.
> 
> Rename update_cpu_closid_rmid() as resctrl_arch_sync_cpus_defaults()
> to be used as the target of an IPI, and pass the effective CLOSID
> and RMID in a new struct.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 19 +++++++++++++++----
>  include/linux/resctrl.h                | 11 +++++++++++
>  2 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 5d2c1ce5b6b1..18f097fce51e 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -341,13 +341,13 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of,
>   * from update_closid_rmid() is protected against __switch_to() because
>   * preemption is disabled.
>   */
> -static void update_cpu_closid_rmid(void *info)
> +void resctrl_arch_sync_cpu_defaults(void *info)

How about keeping the name similar to the old name?

resctrl_arch_update_cpu_defaults

>  {
> -	struct rdtgroup *r = info;
> +	struct resctrl_cpu_sync *r = info;
>  
>  	if (r) {
>  		this_cpu_write(pqr_state.default_closid, r->closid);
> -		this_cpu_write(pqr_state.default_rmid, r->mon.rmid);
> +		this_cpu_write(pqr_state.default_rmid, r->rmid);
>  	}
>  
>  	/*
> @@ -362,11 +362,22 @@ static void update_cpu_closid_rmid(void *info)
>   * Update the PGR_ASSOC MSR on all cpus in @cpu_mask,
>   *
>   * Per task closids/rmids must have been set up before calling this function.
> + * @r may be NULL.
>   */
>  static void
>  update_closid_rmid(const struct cpumask *cpu_mask, struct rdtgroup *r)
>  {
> -	on_each_cpu_mask(cpu_mask, update_cpu_closid_rmid, r, 1);
> +	struct resctrl_cpu_sync defaults;
> +	struct resctrl_cpu_sync *defaults_p = NULL;
> +
> +	if (r) {
> +		defaults.closid = r->closid;
> +		defaults.rmid = r->mon.rmid;
> +		defaults_p = &defaults;
> +	}
> +
> +	on_each_cpu_mask(cpu_mask, resctrl_arch_sync_cpu_defaults, defaults_p,
> +			 1);
>  }
>  
>  static int cpus_mon_write(struct rdtgroup *rdtgrp, cpumask_var_t newmask,
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 6e87bc95f5ea..2b79e4159507 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -220,6 +220,17 @@ struct resctrl_schema {
>  	u32				num_closid;
>  };
>  
> +struct resctrl_cpu_sync {

How about changing it to  resctrl_cpu_defaults?

> +	u32 closid;
> +	u32 rmid;
> +};
> +
> +/*
> + * Update and re-load this CPUs defaults. Called via IPI, takes a pointer to
> + * struct resctrl_cpu_sync, or NULL.
> + */
> +void resctrl_arch_sync_cpu_defaults(void *info);
> +
>  /* The number of closid supported by this resource regardless of CDP */
>  u32 resctrl_arch_get_num_closid(struct rdt_resource *r);
>  int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid);

-- 
Thanks
Babu Moger

On Mon, Apr 15, 2024 at 03:40:46PM -0500, Moger, Babu wrote:
> Hi James/Dave,
> 
> On 3/21/24 11:50, James Morse wrote:
> > update_cpu_closid_rmid() takes a struct rdtgroup as an argument, which
> > it uses to update the local CPUs default pqr values. This is a problem
> > once the resctrl parts move out to /fs/, as the arch code cannot
> > poke around inside struct rdtgroup.
> > 
> > Rename update_cpu_closid_rmid() as resctrl_arch_sync_cpus_defaults()
> > to be used as the target of an IPI, and pass the effective CLOSID
> > and RMID in a new struct.
> > 
> > Signed-off-by: James Morse <james.morse@arm.com>
> > ---
> >  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 19 +++++++++++++++----
> >  include/linux/resctrl.h                | 11 +++++++++++
> >  2 files changed, 26 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > index 5d2c1ce5b6b1..18f097fce51e 100644
> > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > @@ -341,13 +341,13 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of,
> >   * from update_closid_rmid() is protected against __switch_to() because
> >   * preemption is disabled.
> >   */
> > -static void update_cpu_closid_rmid(void *info)
> > +void resctrl_arch_sync_cpu_defaults(void *info)
> 
> How about keeping the name similar to the old name?
> 
> resctrl_arch_update_cpu_defaults

Ack (Reinette made a similar comment.)

> 
> >  {
> > -	struct rdtgroup *r = info;
> > +	struct resctrl_cpu_sync *r = info;
> >  
> >  	if (r) {
> >  		this_cpu_write(pqr_state.default_closid, r->closid);
> > -		this_cpu_write(pqr_state.default_rmid, r->mon.rmid);
> > +		this_cpu_write(pqr_state.default_rmid, r->rmid);
> >  	}
> >  
> >  	/*

[...]

> > diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> > index 6e87bc95f5ea..2b79e4159507 100644
> > --- a/include/linux/resctrl.h
> > +++ b/include/linux/resctrl.h
> > @@ -220,6 +220,17 @@ struct resctrl_schema {
> >  	u32				num_closid;
> >  };
> >  
> > +struct resctrl_cpu_sync {
> 
> How about changing it to  resctrl_cpu_defaults?
> 
> > +	u32 closid;
> > +	u32 rmid;
> > +};

[...]

> -- 
> Thanks
> Babu Moger
> 

Yes, your name describes what the struct means, so renaming it as per
your suggestion does make sense.

I'll make a note.

Cheers
---Dave

Hi James,

On 3/21/2024 9:50 AM, James Morse wrote:
> update_cpu_closid_rmid() takes a struct rdtgroup as an argument, which
> it uses to update the local CPUs default pqr values. This is a problem
> once the resctrl parts move out to /fs/, as the arch code cannot
> poke around inside struct rdtgroup.
> 
> Rename update_cpu_closid_rmid() as resctrl_arch_sync_cpus_defaults()
> to be used as the target of an IPI, and pass the effective CLOSID
> and RMID in a new struct.
> 
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 19 +++++++++++++++----
>  include/linux/resctrl.h                | 11 +++++++++++
>  2 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 5d2c1ce5b6b1..18f097fce51e 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -341,13 +341,13 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of,
>   * from update_closid_rmid() is protected against __switch_to() because
>   * preemption is disabled.
>   */
> -static void update_cpu_closid_rmid(void *info)
> +void resctrl_arch_sync_cpu_defaults(void *info)
>  {
> -	struct rdtgroup *r = info;
> +	struct resctrl_cpu_sync *r = info;
>  
>  	if (r) {
>  		this_cpu_write(pqr_state.default_closid, r->closid);
> -		this_cpu_write(pqr_state.default_rmid, r->mon.rmid);
> +		this_cpu_write(pqr_state.default_rmid, r->rmid);
>  	}
>  
>  	/*
> @@ -362,11 +362,22 @@ static void update_cpu_closid_rmid(void *info)
>   * Update the PGR_ASSOC MSR on all cpus in @cpu_mask,
>   *
>   * Per task closids/rmids must have been set up before calling this function.
> + * @r may be NULL.
>   */
>  static void
>  update_closid_rmid(const struct cpumask *cpu_mask, struct rdtgroup *r)
>  {
> -	on_each_cpu_mask(cpu_mask, update_cpu_closid_rmid, r, 1);
> +	struct resctrl_cpu_sync defaults;
> +	struct resctrl_cpu_sync *defaults_p = NULL;

Please maintain reverse fir order.

> +
> +	if (r) {
> +		defaults.closid = r->closid;
> +		defaults.rmid = r->mon.rmid;
> +		defaults_p = &defaults;
> +	}
> +
> +	on_each_cpu_mask(cpu_mask, resctrl_arch_sync_cpu_defaults, defaults_p,
> +			 1);
>  }
>  
>  static int cpus_mon_write(struct rdtgroup *rdtgrp, cpumask_var_t newmask,
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 6e87bc95f5ea..2b79e4159507 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -220,6 +220,17 @@ struct resctrl_schema {
>  	u32				num_closid;
>  };
>  
> +struct resctrl_cpu_sync {
> +	u32 closid;
> +	u32 rmid;
> +};
> +
> +/*
> + * Update and re-load this CPUs defaults. Called via IPI, takes a pointer to

"this CPU's defaults"?

> + * struct resctrl_cpu_sync, or NULL.
> + */

Updating the CPU's defaults is not the primary goal of this function and because
of that I do not think this should be the focus with the main goal (updating
RMID and CLOSID on CPU) ignored. Specifically, this function only updates
the defaults if *info is set but it _always_ ensures CPU is running with
appropriate CLOSID/RMID (which may or may not be from a CPU default).

I think resctrl_arch_sync_cpu_closid_rmid() may be more appropriate
and the comment needs to elaborate what the function does.

> +void resctrl_arch_sync_cpu_defaults(void *info);
> +
>  /* The number of closid supported by this resource regardless of CDP */
>  u32 resctrl_arch_get_num_closid(struct rdt_resource *r);
>  int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid);

Reinette

On Mon, Apr 08, 2024 at 08:16:08PM -0700, Reinette Chatre wrote:
> Hi James,
> 
> On 3/21/2024 9:50 AM, James Morse wrote:
> > update_cpu_closid_rmid() takes a struct rdtgroup as an argument, which
> > it uses to update the local CPUs default pqr values. This is a problem
> > once the resctrl parts move out to /fs/, as the arch code cannot
> > poke around inside struct rdtgroup.
> > 
> > Rename update_cpu_closid_rmid() as resctrl_arch_sync_cpus_defaults()
> > to be used as the target of an IPI, and pass the effective CLOSID
> > and RMID in a new struct.
> > 
> > Signed-off-by: James Morse <james.morse@arm.com>
> > ---
> >  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 19 +++++++++++++++----
> >  include/linux/resctrl.h                | 11 +++++++++++
> >  2 files changed, 26 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > index 5d2c1ce5b6b1..18f097fce51e 100644
> > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > @@ -341,13 +341,13 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of,
> >   * from update_closid_rmid() is protected against __switch_to() because
> >   * preemption is disabled.
> >   */
> > -static void update_cpu_closid_rmid(void *info)
> > +void resctrl_arch_sync_cpu_defaults(void *info)
> >  {
> > -	struct rdtgroup *r = info;
> > +	struct resctrl_cpu_sync *r = info;
> >  
> >  	if (r) {
> >  		this_cpu_write(pqr_state.default_closid, r->closid);
> > -		this_cpu_write(pqr_state.default_rmid, r->mon.rmid);
> > +		this_cpu_write(pqr_state.default_rmid, r->rmid);
> >  	}
> >  
> >  	/*
> > @@ -362,11 +362,22 @@ static void update_cpu_closid_rmid(void *info)
> >   * Update the PGR_ASSOC MSR on all cpus in @cpu_mask,
> >   *
> >   * Per task closids/rmids must have been set up before calling this function.
> > + * @r may be NULL.
> >   */
> >  static void
> >  update_closid_rmid(const struct cpumask *cpu_mask, struct rdtgroup *r)
> >  {
> > -	on_each_cpu_mask(cpu_mask, update_cpu_closid_rmid, r, 1);
> > +	struct resctrl_cpu_sync defaults;
> > +	struct resctrl_cpu_sync *defaults_p = NULL;
> 
> Please maintain reverse fir order.

Or, more tersely as follows?

	struct resctrl_cpu_sync defaults, *defaults_p = NULL;

"Reverse fir order" seems to be documented as a preference rather than a
rule.

The declarations can be swapped, but defaults_p is in some sense a weak
pointer to defaults, so it feels a bit strange to declare them backwards.

Alternatively, could we rename defaults_p to p?  Given the size of this
function I don't think that impacts clarity.

I'll wait for your opinion on this.


> > +
> > +	if (r) {
> > +		defaults.closid = r->closid;
> > +		defaults.rmid = r->mon.rmid;
> > +		defaults_p = &defaults;
> > +	}
> > +
> > +	on_each_cpu_mask(cpu_mask, resctrl_arch_sync_cpu_defaults, defaults_p,
> > +			 1);
> >  }
> >  
> >  static int cpus_mon_write(struct rdtgroup *rdtgrp, cpumask_var_t newmask,
> > diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> > index 6e87bc95f5ea..2b79e4159507 100644
> > --- a/include/linux/resctrl.h
> > +++ b/include/linux/resctrl.h
> > @@ -220,6 +220,17 @@ struct resctrl_schema {
> >  	u32				num_closid;
> >  };
> >  
> > +struct resctrl_cpu_sync {
> > +	u32 closid;
> > +	u32 rmid;
> > +};
> > +
> > +/*
> > + * Update and re-load this CPUs defaults. Called via IPI, takes a pointer to
> 
> "this CPU's defaults"?

Ack (also in the commit message).

> 
> > + * struct resctrl_cpu_sync, or NULL.
> > + */
> 
> Updating the CPU's defaults is not the primary goal of this function and because
> of that I do not think this should be the focus with the main goal (updating
> RMID and CLOSID on CPU) ignored. Specifically, this function only updates
> the defaults if *info is set but it _always_ ensures CPU is running with
> appropriate CLOSID/RMID (which may or may not be from a CPU default).
> 
> I think resctrl_arch_sync_cpu_closid_rmid() may be more appropriate
> and the comment needs to elaborate what the function does.
>
> > +void resctrl_arch_sync_cpu_defaults(void *info);

That seems reasonable, and follows the original naming and what the
code does:

What about:

/**
 * resctrl_arch_sync_cpu_defaults() - Refresh the CPU's CLOSID and RMID.
 *				      Call via IPI.
 * @info:	If non-NULL, a pointer to a struct resctrl_cpu_sync specifying
 *		the new CLOSID and RMID for tasks in the default resctrl ctrl
 *		and mon group when running on this CPU.  If NULL, the default
 *		CLOSID and RMID are not changed.
 *
 * This is how reassignment of CPUs and/or tasks to different resctrl groups
 * is propagated when requested by the resctrl fs core code.
 *
 * This function should typically record the per-cpu defaults specified by
 * @info (if any), and then reconfigure the CPU's hardware CLOSID and RMID
 * for subsequent execution based on @current, in the same way as during a
 * task switch.
 */

...?


Cheers
---Dave

Hi Dave,

On 4/12/2024 9:12 AM, Dave Martin wrote:
> On Mon, Apr 08, 2024 at 08:16:08PM -0700, Reinette Chatre wrote:
>> Hi James,
>>
>> On 3/21/2024 9:50 AM, James Morse wrote:
>>> update_cpu_closid_rmid() takes a struct rdtgroup as an argument, which
>>> it uses to update the local CPUs default pqr values. This is a problem
>>> once the resctrl parts move out to /fs/, as the arch code cannot
>>> poke around inside struct rdtgroup.
>>>
>>> Rename update_cpu_closid_rmid() as resctrl_arch_sync_cpus_defaults()
>>> to be used as the target of an IPI, and pass the effective CLOSID
>>> and RMID in a new struct.
>>>
>>> Signed-off-by: James Morse <james.morse@arm.com>
>>> ---
>>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 19 +++++++++++++++----
>>>  include/linux/resctrl.h                | 11 +++++++++++
>>>  2 files changed, 26 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> index 5d2c1ce5b6b1..18f097fce51e 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> @@ -341,13 +341,13 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of,
>>>   * from update_closid_rmid() is protected against __switch_to() because
>>>   * preemption is disabled.
>>>   */
>>> -static void update_cpu_closid_rmid(void *info)
>>> +void resctrl_arch_sync_cpu_defaults(void *info)
>>>  {
>>> -	struct rdtgroup *r = info;
>>> +	struct resctrl_cpu_sync *r = info;
>>>  
>>>  	if (r) {
>>>  		this_cpu_write(pqr_state.default_closid, r->closid);
>>> -		this_cpu_write(pqr_state.default_rmid, r->mon.rmid);
>>> +		this_cpu_write(pqr_state.default_rmid, r->rmid);
>>>  	}
>>>  
>>>  	/*
>>> @@ -362,11 +362,22 @@ static void update_cpu_closid_rmid(void *info)
>>>   * Update the PGR_ASSOC MSR on all cpus in @cpu_mask,
>>>   *
>>>   * Per task closids/rmids must have been set up before calling this function.
>>> + * @r may be NULL.
>>>   */
>>>  static void
>>>  update_closid_rmid(const struct cpumask *cpu_mask, struct rdtgroup *r)
>>>  {
>>> -	on_each_cpu_mask(cpu_mask, update_cpu_closid_rmid, r, 1);
>>> +	struct resctrl_cpu_sync defaults;
>>> +	struct resctrl_cpu_sync *defaults_p = NULL;
>>
>> Please maintain reverse fir order.
> 
> Or, more tersely as follows?
> 
> 	struct resctrl_cpu_sync defaults, *defaults_p = NULL;

Sure.

> 
> "Reverse fir order" seems to be documented as a preference rather than a
> rule.

This does not seem to be a place that warrants an exception to this
preference. Note how this function is not consistent with any other
in the file.

> The declarations can be swapped, but defaults_p is in some sense a weak
> pointer to defaults, so it feels a bit strange to declare them backwards.
> 
> Alternatively, could we rename defaults_p to p?  Given the size of this
> function I don't think that impacts clarity.

Do you imply that this would maintain the order in this patch? It does
not look to me that it would but I may be looking wrong.

sidenote: the "on_each_cpu_mask()" in update_closid_rmid() can be on
one line.

> 
> I'll wait for your opinion on this.
> 
> 

...

>>> + * struct resctrl_cpu_sync, or NULL.
>>> + */
>>
>> Updating the CPU's defaults is not the primary goal of this function and because
>> of that I do not think this should be the focus with the main goal (updating
>> RMID and CLOSID on CPU) ignored. Specifically, this function only updates
>> the defaults if *info is set but it _always_ ensures CPU is running with
>> appropriate CLOSID/RMID (which may or may not be from a CPU default).
>>
>> I think resctrl_arch_sync_cpu_closid_rmid() may be more appropriate
>> and the comment needs to elaborate what the function does.
>>
>>> +void resctrl_arch_sync_cpu_defaults(void *info);
> 
> That seems reasonable, and follows the original naming and what the
> code does:
> 
> What about:
> 
> /**
>  * resctrl_arch_sync_cpu_defaults() - Refresh the CPU's CLOSID and RMID.
>  *				      Call via IPI.

Did you intend to change function name?

How about "Refresh the CPU's ..." -> "Refresh this CPU's ..." I think it
makes it more obvious how this function is called.

>  * @info:	If non-NULL, a pointer to a struct resctrl_cpu_sync specifying
>  *		the new CLOSID and RMID for tasks in the default resctrl ctrl
>  *		and mon group when running on this CPU.  If NULL, the default
>  *		CLOSID and RMID are not changed.

"If NULL, this CPU is not re-assigned to a different group." ?

>  *
>  * This is how reassignment of CPUs and/or tasks to different resctrl groups
>  * is propagated when requested by the resctrl fs core code.

Could you please use imperative tone here?  For example, "Propagates reassignment
of CPUs and/or tasks to different resctrl groups."

>  *
>  * This function should typically record the per-cpu defaults specified by

"should" sounds like there may be cases when this is not done? Maybe just
"Records the per-CPU defaults specified ..."

>  * @info (if any), and then reconfigure the CPU's hardware CLOSID and RMID
>  * for subsequent execution based on @current, in the same way as during a
>  * task switch.
>  */
> 
> ...?

Reinette

On Mon, Apr 15, 2024 at 10:47:55AM -0700, Reinette Chatre wrote:
> Hi Dave,
> 
> On 4/12/2024 9:12 AM, Dave Martin wrote:
> > On Mon, Apr 08, 2024 at 08:16:08PM -0700, Reinette Chatre wrote:
> >> Hi James,
> >>
> >> On 3/21/2024 9:50 AM, James Morse wrote:
> >>> update_cpu_closid_rmid() takes a struct rdtgroup as an argument, which
> >>> it uses to update the local CPUs default pqr values. This is a problem
> >>> once the resctrl parts move out to /fs/, as the arch code cannot
> >>> poke around inside struct rdtgroup.
> >>>
> >>> Rename update_cpu_closid_rmid() as resctrl_arch_sync_cpus_defaults()
> >>> to be used as the target of an IPI, and pass the effective CLOSID
> >>> and RMID in a new struct.
> >>>
> >>> Signed-off-by: James Morse <james.morse@arm.com>
> >>> ---
> >>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 19 +++++++++++++++----
> >>>  include/linux/resctrl.h                | 11 +++++++++++
> >>>  2 files changed, 26 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> >>> index 5d2c1ce5b6b1..18f097fce51e 100644
> >>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> >>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> >>> @@ -341,13 +341,13 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of,
> >>>   * from update_closid_rmid() is protected against __switch_to() because
> >>>   * preemption is disabled.
> >>>   */
> >>> -static void update_cpu_closid_rmid(void *info)
> >>> +void resctrl_arch_sync_cpu_defaults(void *info)
> >>>  {
> >>> -	struct rdtgroup *r = info;
> >>> +	struct resctrl_cpu_sync *r = info;
> >>>  
> >>>  	if (r) {
> >>>  		this_cpu_write(pqr_state.default_closid, r->closid);
> >>> -		this_cpu_write(pqr_state.default_rmid, r->mon.rmid);
> >>> +		this_cpu_write(pqr_state.default_rmid, r->rmid);
> >>>  	}
> >>>  
> >>>  	/*
> >>> @@ -362,11 +362,22 @@ static void update_cpu_closid_rmid(void *info)
> >>>   * Update the PGR_ASSOC MSR on all cpus in @cpu_mask,
> >>>   *
> >>>   * Per task closids/rmids must have been set up before calling this function.
> >>> + * @r may be NULL.
> >>>   */
> >>>  static void
> >>>  update_closid_rmid(const struct cpumask *cpu_mask, struct rdtgroup *r)
> >>>  {
> >>> -	on_each_cpu_mask(cpu_mask, update_cpu_closid_rmid, r, 1);
> >>> +	struct resctrl_cpu_sync defaults;
> >>> +	struct resctrl_cpu_sync *defaults_p = NULL;
> >>
> >> Please maintain reverse fir order.
> > 
> > Or, more tersely as follows?
> > 
> > 	struct resctrl_cpu_sync defaults, *defaults_p = NULL;
> 
> Sure.

[*]

> > 
> > "Reverse fir order" seems to be documented as a preference rather than a
> > rule.
> 
> This does not seem to be a place that warrants an exception to this
> preference. Note how this function is not consistent with any other
> in the file.

Ack (just bikeshedding here TBH).

> 
> > The declarations can be swapped, but defaults_p is in some sense a weak
> > pointer to defaults, so it feels a bit strange to declare them backwards.
> > 
> > Alternatively, could we rename defaults_p to p?  Given the size of this
> > function I don't think that impacts clarity.

[...]

> > 
> > I'll wait for your opinion on this.
> > 
> > 
> 
> Do you imply that this would maintain the order in this patch? It does
> not look to me that it would but I may be looking wrong.

I'm not sure without looking again, but since this discussion is not a
good use of your time I'll just go ahead and implement the change at
[*] above, while restoring referse FIR order, if that is good for you.

> 
> sidenote: the "on_each_cpu_mask()" in update_closid_rmid() can be on
> one line.

I guess that might have been split to stick to the 80-char limit.

Due the the small size of this function, shall I just rename defaults_p to p?
Alternatively, there are already a few non-printk lines over 80 chars, so
maybe we can tolerate one more here?

> 
> ..
> 
> >>> + * struct resctrl_cpu_sync, or NULL.
> >>> + */
> >>
> >> Updating the CPU's defaults is not the primary goal of this function and because
> >> of that I do not think this should be the focus with the main goal (updating
> >> RMID and CLOSID on CPU) ignored. Specifically, this function only updates
> >> the defaults if *info is set but it _always_ ensures CPU is running with
> >> appropriate CLOSID/RMID (which may or may not be from a CPU default).
> >>
> >> I think resctrl_arch_sync_cpu_closid_rmid() may be more appropriate
> >> and the comment needs to elaborate what the function does.
> >>
> >>> +void resctrl_arch_sync_cpu_defaults(void *info);
> > 
> > That seems reasonable, and follows the original naming and what the
> > code does:
> > 
> > What about:
> > 
> > /**
> >  * resctrl_arch_sync_cpu_defaults() - Refresh the CPU's CLOSID and RMID.
> >  *				      Call via IPI.
> 
> Did you intend to change function name?

Er, yes, I meant to use your suggestion here, so:
resctrl_arch_sync_cpu_closid_rmid().

Also, Babu Moger's suggestion to rename struct resctrl_cpu_sync
to resctrl_cpu_defaults seems good, since that accurately describes what
is specified in the struct (and what is *not* specified if NULL is
passed).

> 
> How about "Refresh the CPU's ..." -> "Refresh this CPU's ..." I think it
> makes it more obvious how this function is called.

Agreed.

> 
> >  * @info:	If non-NULL, a pointer to a struct resctrl_cpu_sync specifying
> >  *		the new CLOSID and RMID for tasks in the default resctrl ctrl
> >  *		and mon group when running on this CPU.  If NULL, the default
> >  *		CLOSID and RMID are not changed.
> 
> "If NULL, this CPU is not re-assigned to a different group." ?

Agreed.

> >  *
> >  * This is how reassignment of CPUs and/or tasks to different resctrl groups
> >  * is propagated when requested by the resctrl fs core code.
> 
> Could you please use imperative tone here?  For example, "Propagates reassignment
> of CPUs and/or tasks to different resctrl groups."

Yes, that's better (and shorter).

> 
> >  *
> >  * This function should typically record the per-cpu defaults specified by
> 
> "should" sounds like there may be cases when this is not done? Maybe just
> "Records the per-CPU defaults specified ..."

I didn't want to pre-judge what implementation-specific cruft the arch
code needs here, so I was intentionally vague.  But the arch would need
to put the CPU defaults into effect somehow or other, so yes, I think
your text is better here.

I'll make a note of those changes.

[...]

Cheers
---Dave

Hi Dave,

On 4/16/2024 9:16 AM, Dave Martin wrote:
> On Mon, Apr 15, 2024 at 10:47:55AM -0700, Reinette Chatre wrote:
>> On 4/12/2024 9:12 AM, Dave Martin wrote:
>>> On Mon, Apr 08, 2024 at 08:16:08PM -0700, Reinette Chatre wrote:
>>>> On 3/21/2024 9:50 AM, James Morse wrote:
...

>> Do you imply that this would maintain the order in this patch? It does
>> not look to me that it would but I may be looking wrong.
> 
> I'm not sure without looking again, but since this discussion is not a
> good use of your time I'll just go ahead and implement the change at
> [*] above, while restoring referse FIR order, if that is good for you.
> 
>>
>> sidenote: the "on_each_cpu_mask()" in update_closid_rmid() can be on
>> one line.
> 
> I guess that might have been split to stick to the 80-char limit.
> 
> Due the the small size of this function, shall I just rename defaults_p to p?
> Alternatively, there are already a few non-printk lines over 80 chars, so
> maybe we can tolerate one more here?

80 chars are not enforced so strictly that it impacts readability. You
may refer to how update_task_closid_rmid() looks for more confidence in/
motivation for placing this on one line.

> 
>>
>> ..
>>
>>>>> + * struct resctrl_cpu_sync, or NULL.
>>>>> + */
>>>>
>>>> Updating the CPU's defaults is not the primary goal of this function and because
>>>> of that I do not think this should be the focus with the main goal (updating
>>>> RMID and CLOSID on CPU) ignored. Specifically, this function only updates
>>>> the defaults if *info is set but it _always_ ensures CPU is running with
>>>> appropriate CLOSID/RMID (which may or may not be from a CPU default).
>>>>
>>>> I think resctrl_arch_sync_cpu_closid_rmid() may be more appropriate
>>>> and the comment needs to elaborate what the function does.
>>>>
>>>>> +void resctrl_arch_sync_cpu_defaults(void *info);
>>>
>>> That seems reasonable, and follows the original naming and what the
>>> code does:
>>>
>>> What about:
>>>
>>> /**
>>>  * resctrl_arch_sync_cpu_defaults() - Refresh the CPU's CLOSID and RMID.
>>>  *				      Call via IPI.
>>
>> Did you intend to change function name?
> 
> Er, yes, I meant to use your suggestion here, so:
> resctrl_arch_sync_cpu_closid_rmid().
> 

I'm a bit confused here when comparing with your response in [1] mentioning
a change to another name. 

[1] https://lore.kernel.org/lkml/Zh6kgs1%2fbji1P1Hl@e133380.arm.com/

> Also, Babu Moger's suggestion to rename struct resctrl_cpu_sync
> to resctrl_cpu_defaults seems good, since that accurately describes what
> is specified in the struct (and what is *not* specified if NULL is
> passed).

Sounds good, yes.

Reinette

On Wed, Apr 17, 2024 at 10:12:35PM -0700, Reinette Chatre wrote:
> Hi Dave,
> 
> On 4/16/2024 9:16 AM, Dave Martin wrote:
> > On Mon, Apr 15, 2024 at 10:47:55AM -0700, Reinette Chatre wrote:
> >> On 4/12/2024 9:12 AM, Dave Martin wrote:
> >>> On Mon, Apr 08, 2024 at 08:16:08PM -0700, Reinette Chatre wrote:
> >>>> On 3/21/2024 9:50 AM, James Morse wrote:
> ..
> 
> >> Do you imply that this would maintain the order in this patch? It does
> >> not look to me that it would but I may be looking wrong.
> > 
> > I'm not sure without looking again, but since this discussion is not a
> > good use of your time I'll just go ahead and implement the change at
> > [*] above, while restoring referse FIR order, if that is good for you.
> > 
> >>
> >> sidenote: the "on_each_cpu_mask()" in update_closid_rmid() can be on
> >> one line.
> > 
> > I guess that might have been split to stick to the 80-char limit.
> > 
> > Due the the small size of this function, shall I just rename defaults_p to p?
> > Alternatively, there are already a few non-printk lines over 80 chars, so
> > maybe we can tolerate one more here?
> 
> 80 chars are not enforced so strictly that it impacts readability. You
> may refer to how update_task_closid_rmid() looks for more confidence in/
> motivation for placing this on one line.

Agreed.

(I did in fact rename default_p to p in my fixup, which shortens the
affected line as a side-effect anyway.)


<aside>

Although probably out of scope for this series, I wonder whether these
two paths can be combined?

update_task_closid_rmid() selects the cross_call target by task, where
update_closid_rmid() selects the cross_call target(s) by cpu.  But the
backend work that the arch code needs to do seems basically the same:
possibly update the the CPU default group membership, the reconfigure
the MSRs for the running task to ensure that they aren't stale (with a
possible optimisation not to bother if we sure that the MSRs are not
stale for the task actually running, or if we know they wouldn't be
changed by the write).

Even the check to see whether the right task is running seems somewhat
redundant: we already paid the cost of taking the IPI, and we have to
cope with spurious, idempotent updates to the MSRs anyway since this is
all racy.

Is there a high overhead to writing the MSRs on x86?

For arm64, the relevant system register only affects EL0 (i.e.,
userspace) execution, so we defer synchronisation of a whole bunch of
stuff until the return to userspace.

</aside>


> 
> > 
> >>
> >> ..
> >>
> >>>>> + * struct resctrl_cpu_sync, or NULL.
> >>>>> + */
> >>>>
> >>>> Updating the CPU's defaults is not the primary goal of this function and because
> >>>> of that I do not think this should be the focus with the main goal (updating
> >>>> RMID and CLOSID on CPU) ignored. Specifically, this function only updates
> >>>> the defaults if *info is set but it _always_ ensures CPU is running with
> >>>> appropriate CLOSID/RMID (which may or may not be from a CPU default).
> >>>>
> >>>> I think resctrl_arch_sync_cpu_closid_rmid() may be more appropriate
> >>>> and the comment needs to elaborate what the function does.
> >>>>
> >>>>> +void resctrl_arch_sync_cpu_defaults(void *info);
> >>>
> >>> That seems reasonable, and follows the original naming and what the
> >>> code does:
> >>>
> >>> What about:
> >>>
> >>> /**
> >>>  * resctrl_arch_sync_cpu_defaults() - Refresh the CPU's CLOSID and RMID.
> >>>  *				      Call via IPI.
> >>
> >> Did you intend to change function name?
> > 
> > Er, yes, I meant to use your suggestion here, so:
> > resctrl_arch_sync_cpu_closid_rmid().
> > 
> 
> I'm a bit confused here when comparing with your response in [1] mentioning
> a change to another name. 
> 
> [1] https://lore.kernel.org/lkml/Zh6kgs1%2fbji1P1Hl@e133380.arm.com/

My bad (sorry Babu!).

I read that suggestion carelessly and assumed it was aligned with
Reinette's.

The most important thing seems to be to transfer the "defaults" from the
name of the function to the name of the struct, since the struct is
about defaults (and only about defaults), while the function is about
defaults and the running task.

To avoid extra busy-work, I'll stick with
resctrl_arch_sync_cpu_closid_rmid() for now, but I don't mind changing
it if people prefer.


> > Also, Babu Moger's suggestion to rename struct resctrl_cpu_sync
> > to resctrl_cpu_defaults seems good, since that accurately describes what
> > is specified in the struct (and what is *not* specified if NULL is
> > passed).
> 
> Sounds good, yes.
> 
> Reinette
> 

Cheers
---Dave

Hi Dave,

On 4/18/2024 8:21 AM, Dave Martin wrote:
> On Wed, Apr 17, 2024 at 10:12:35PM -0700, Reinette Chatre wrote:
>> On 4/16/2024 9:16 AM, Dave Martin wrote:
>>> On Mon, Apr 15, 2024 at 10:47:55AM -0700, Reinette Chatre wrote:
>>>> On 4/12/2024 9:12 AM, Dave Martin wrote:
>>>>> On Mon, Apr 08, 2024 at 08:16:08PM -0700, Reinette Chatre wrote:
>>>>>> On 3/21/2024 9:50 AM, James Morse wrote:
...


> <aside>
> 
> Although probably out of scope for this series, I wonder whether these
> two paths can be combined?
> 
> update_task_closid_rmid() selects the cross_call target by task, where
> update_closid_rmid() selects the cross_call target(s) by cpu.  But the
> backend work that the arch code needs to do seems basically the same:
> possibly update the the CPU default group membership, the reconfigure
> the MSRs for the running task to ensure that they aren't stale (with a
> possible optimisation not to bother if we sure that the MSRs are not
> stale for the task actually running, or if we know they wouldn't be
> changed by the write).
> 
> Even the check to see whether the right task is running seems somewhat
> redundant: we already paid the cost of taking the IPI, and we have to
> cope with spurious, idempotent updates to the MSRs anyway since this is
> all racy.
> 
> Is there a high overhead to writing the MSRs on x86?

The MSRs do not all have the same overhead. MSR_IA32_PQR_ASSOC is intended to
be updated quickly but it is not free and I'd prefer to keep avoiding
unnecessary updates where possible.

> 
> For arm64, the relevant system register only affects EL0 (i.e.,
> userspace) execution, so we defer synchronisation of a whole bunch of
> stuff until the return to userspace.
> 
> </aside>
> 
> 
>>
>>>
>>>>
>>>> ..
>>>>
>>>>>>> + * struct resctrl_cpu_sync, or NULL.
>>>>>>> + */
>>>>>>
>>>>>> Updating the CPU's defaults is not the primary goal of this function and because
>>>>>> of that I do not think this should be the focus with the main goal (updating
>>>>>> RMID and CLOSID on CPU) ignored. Specifically, this function only updates
>>>>>> the defaults if *info is set but it _always_ ensures CPU is running with
>>>>>> appropriate CLOSID/RMID (which may or may not be from a CPU default).
>>>>>>
>>>>>> I think resctrl_arch_sync_cpu_closid_rmid() may be more appropriate
>>>>>> and the comment needs to elaborate what the function does.
>>>>>>
>>>>>>> +void resctrl_arch_sync_cpu_defaults(void *info);
>>>>>
>>>>> That seems reasonable, and follows the original naming and what the
>>>>> code does:
>>>>>
>>>>> What about:
>>>>>
>>>>> /**
>>>>>  * resctrl_arch_sync_cpu_defaults() - Refresh the CPU's CLOSID and RMID.
>>>>>  *				      Call via IPI.
>>>>
>>>> Did you intend to change function name?
>>>
>>> Er, yes, I meant to use your suggestion here, so:
>>> resctrl_arch_sync_cpu_closid_rmid().
>>>
>>
>> I'm a bit confused here when comparing with your response in [1] mentioning
>> a change to another name. 
>>
>> [1] https://lore.kernel.org/lkml/Zh6kgs1%2fbji1P1Hl@e133380.arm.com/
> 
> My bad (sorry Babu!).
> 
> I read that suggestion carelessly and assumed it was aligned with
> Reinette's.
> 
> The most important thing seems to be to transfer the "defaults" from the
> name of the function to the name of the struct, since the struct is
> about defaults (and only about defaults), while the function is about
> defaults and the running task.
> 
> To avoid extra busy-work, I'll stick with
> resctrl_arch_sync_cpu_closid_rmid() for now, but I don't mind changing
> it if people prefer.

resctrl_arch_sync_cpu_closid_rmid() sounds good to me.

Reinette