When switching tasks, the CLOSID and RMID that the new task should
use are stored in struct task_struct. For x86 the CLOSID known by resctrl,
the value in task_struct, and the value written to the CPU register are
all the same thing.
MPAM's CPU interface has two different PARTID's one for data accesses
the other for instruction fetch. Storing resctrl's CLOSID value in
struct task_struct implies the arch code knows whether resctrl is using
CDP.
Move the matching and setting of the struct task_struct properties
to use helpers. This allows arm64 to store the hardware format of
the register, instead of having to convert it each time.
__rdtgroup_move_task()s use of READ_ONCE()/WRITE_ONCE() ensures torn
values aren't seen as another CPU may schedule the task being moved
while the value is being changed. MPAM has an additional corner-case
here as the PMG bits extend the PARTID space. If the scheduler sees a
new-CLOSID but old-RMID, the task will dirty an RMID that the limbo code
is not watching causing an inaccurate count. x86's RMID are independent
values, so the limbo code will still be watching the old-RMID in this
circumstance.
To avoid this, arm64 needs both the CLOSID/RMID WRITE_ONCE()d together.
Both values must be provided together.
Because MPAM's RMID values are not unique, the CLOSID must be provided
when matching the RMID.
CC: Valentin Schneider <[email protected]>
Tested-by: Shaopeng Tan <[email protected]>
Signed-off-by: James Morse <[email protected]>
---
arch/x86/include/asm/resctrl.h | 18 ++++++++
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 57 +++++++++++++++-----------
2 files changed, 51 insertions(+), 24 deletions(-)
diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
index 44d568f3577c..d589a82995ac 100644
--- a/arch/x86/include/asm/resctrl.h
+++ b/arch/x86/include/asm/resctrl.h
@@ -95,6 +95,24 @@ static inline unsigned int resctrl_arch_round_mon_val(unsigned int val)
return val * scale;
}
+static inline void resctrl_arch_set_closid_rmid(struct task_struct *tsk,
+ u32 closid, u32 rmid)
+{
+ WRITE_ONCE(tsk->closid, closid);
+ WRITE_ONCE(tsk->rmid, rmid);
+}
+
+static inline bool resctrl_arch_match_closid(struct task_struct *tsk, u32 closid)
+{
+ return READ_ONCE(tsk->closid) == closid;
+}
+
+static inline bool resctrl_arch_match_rmid(struct task_struct *tsk, u32 ignored,
+ u32 rmid)
+{
+ return READ_ONCE(tsk->rmid) == rmid;
+}
+
static inline void resctrl_sched_in(void)
{
if (static_branch_likely(&rdt_enable_key))
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index e1f879e13823..ced7400decae 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -84,7 +84,7 @@ void rdt_last_cmd_printf(const char *fmt, ...)
*
* Using a global CLOSID across all resources has some advantages and
* some drawbacks:
- * + We can simply set "current->closid" to assign a task to a resource
+ * + We can simply set current's closid to assign a task to a resource
* group.
* + Context switch code can avoid extra memory references deciding which
* CLOSID to load into the PQR_ASSOC MSR
@@ -549,14 +549,26 @@ static void update_task_closid_rmid(struct task_struct *t)
_update_task_closid_rmid(t);
}
+static bool task_in_rdtgroup(struct task_struct *tsk, struct rdtgroup *rdtgrp)
+{
+ u32 closid, rmid = rdtgrp->mon.rmid;
+
+ if (rdtgrp->type == RDTCTRL_GROUP)
+ closid = rdtgrp->closid;
+ else if (rdtgrp->type == RDTMON_GROUP)
+ closid = rdtgrp->mon.parent->closid;
+ else
+ return false;
+
+ return resctrl_arch_match_closid(tsk, closid) &&
+ resctrl_arch_match_rmid(tsk, closid, rmid);
+}
+
static int __rdtgroup_move_task(struct task_struct *tsk,
struct rdtgroup *rdtgrp)
{
/* If the task is already in rdtgrp, no need to move the task. */
- if ((rdtgrp->type == RDTCTRL_GROUP && tsk->closid == rdtgrp->closid &&
- tsk->rmid == rdtgrp->mon.rmid) ||
- (rdtgrp->type == RDTMON_GROUP && tsk->rmid == rdtgrp->mon.rmid &&
- tsk->closid == rdtgrp->mon.parent->closid))
+ if (task_in_rdtgroup(tsk, rdtgrp))
return 0;
/*
@@ -567,19 +579,14 @@ static int __rdtgroup_move_task(struct task_struct *tsk,
* For monitor groups, can move the tasks only from
* their parent CTRL group.
*/
-
- if (rdtgrp->type == RDTCTRL_GROUP) {
- WRITE_ONCE(tsk->closid, rdtgrp->closid);
- WRITE_ONCE(tsk->rmid, rdtgrp->mon.rmid);
- } else if (rdtgrp->type == RDTMON_GROUP) {
- if (rdtgrp->mon.parent->closid == tsk->closid) {
- WRITE_ONCE(tsk->rmid, rdtgrp->mon.rmid);
- } else {
- rdt_last_cmd_puts("Can't move task to different control group\n");
- return -EINVAL;
- }
+ if (rdtgrp->type == RDTMON_GROUP &&
+ !resctrl_arch_match_closid(tsk, rdtgrp->mon.parent->closid)) {
+ rdt_last_cmd_puts("Can't move task to different control group\n");
+ return -EINVAL;
}
+ resctrl_arch_set_closid_rmid(tsk, rdtgrp->closid, rdtgrp->mon.rmid);
+
/*
* Ensure the task's closid and rmid are written before determining if
* the task is current that will decide if it will be interrupted.
@@ -599,14 +606,15 @@ static int __rdtgroup_move_task(struct task_struct *tsk,
static bool is_closid_match(struct task_struct *t, struct rdtgroup *r)
{
- return (rdt_alloc_capable &&
- (r->type == RDTCTRL_GROUP) && (t->closid == r->closid));
+ return (rdt_alloc_capable && (r->type == RDTCTRL_GROUP) &&
+ resctrl_arch_match_closid(t, r->closid));
}
static bool is_rmid_match(struct task_struct *t, struct rdtgroup *r)
{
- return (rdt_mon_capable &&
- (r->type == RDTMON_GROUP) && (t->rmid == r->mon.rmid));
+ return (rdt_mon_capable && (r->type == RDTMON_GROUP) &&
+ resctrl_arch_match_rmid(t, r->mon.parent->closid,
+ r->mon.rmid));
}
/**
@@ -802,7 +810,7 @@ int proc_resctrl_show(struct seq_file *s, struct pid_namespace *ns,
rdtg->mode != RDT_MODE_EXCLUSIVE)
continue;
- if (rdtg->closid != tsk->closid)
+ if (!resctrl_arch_match_closid(tsk, rdtg->closid))
continue;
seq_printf(s, "res:%s%s\n", (rdtg == &rdtgroup_default) ? "/" : "",
@@ -810,7 +818,8 @@ int proc_resctrl_show(struct seq_file *s, struct pid_namespace *ns,
seq_puts(s, "mon:");
list_for_each_entry(crg, &rdtg->mon.crdtgrp_list,
mon.crdtgrp_list) {
- if (tsk->rmid != crg->mon.rmid)
+ if (!resctrl_arch_match_rmid(tsk, crg->mon.parent->closid,
+ crg->mon.rmid))
continue;
seq_printf(s, "%s", crg->kn->name);
break;
@@ -2401,8 +2410,8 @@ static void rdt_move_group_tasks(struct rdtgroup *from, struct rdtgroup *to,
for_each_process_thread(p, t) {
if (!from || is_closid_match(t, from) ||
is_rmid_match(t, from)) {
- WRITE_ONCE(t->closid, to->closid);
- WRITE_ONCE(t->rmid, to->mon.rmid);
+ resctrl_arch_set_closid_rmid(t, to->closid,
+ to->mon.rmid);
/*
* If the task is on a CPU, set the CPU in the mask.
--
2.30.2
Hi, James,
> When switching tasks, the CLOSID and RMID that the new task should use are
> stored in struct task_struct. For x86 the CLOSID known by resctrl, the value in
> task_struct, and the value written to the CPU register are all the same thing.
>
> MPAM's CPU interface has two different PARTID's one for data accesses the
> other for instruction fetch. Storing resctrl's CLOSID value in struct task_struct
> implies the arch code knows whether resctrl is using CDP.
>
> Move the matching and setting of the struct task_struct properties to use
> helpers. This allows arm64 to store the hardware format of the register, instead
> of having to convert it each time.
>
> __rdtgroup_move_task()s use of READ_ONCE()/WRITE_ONCE() ensures torn
> values aren't seen as another CPU may schedule the task being moved while the
> value is being changed. MPAM has an additional corner-case here as the PMG
> bits extend the PARTID space. If the scheduler sees a new-CLOSID but old-RMID,
> the task will dirty an RMID that the limbo code is not watching causing an
> inaccurate count. x86's RMID are independent values, so the limbo code will still
> be watching the old-RMID in this circumstance.
> To avoid this, arm64 needs both the CLOSID/RMID WRITE_ONCE()d together.
> Both values must be provided together.
>
> Because MPAM's RMID values are not unique, the CLOSID must be provided
> when matching the RMID.
>
> CC: Valentin Schneider <[email protected]>
> Tested-by: Shaopeng Tan <[email protected]>
> Signed-off-by: James Morse <[email protected]>
> ---
> arch/x86/include/asm/resctrl.h | 18 ++++++++
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 57 +++++++++++++++-----------
> 2 files changed, 51 insertions(+), 24 deletions(-)
>
> diff --git a/arch/x86/include/asm/resctrl.h b/arch/x86/include/asm/resctrl.h
> index 44d568f3577c..d589a82995ac 100644
> --- a/arch/x86/include/asm/resctrl.h
> +++ b/arch/x86/include/asm/resctrl.h
> @@ -95,6 +95,24 @@ static inline unsigned int
> resctrl_arch_round_mon_val(unsigned int val)
> return val * scale;
> }
>
> +static inline void resctrl_arch_set_closid_rmid(struct task_struct *tsk,
> + u32 closid, u32 rmid)
> +{
> + WRITE_ONCE(tsk->closid, closid);
> + WRITE_ONCE(tsk->rmid, rmid);
> +}
> +
> +static inline bool resctrl_arch_match_closid(struct task_struct *tsk,
> +u32 closid) {
> + return READ_ONCE(tsk->closid) == closid; }
> +
> +static inline bool resctrl_arch_match_rmid(struct task_struct *tsk, u32 ignored,
> + u32 rmid)
> +{
> + return READ_ONCE(tsk->rmid) == rmid;
> +}
> +
> static inline void resctrl_sched_in(void) {
> if (static_branch_likely(&rdt_enable_key))
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index e1f879e13823..ced7400decae 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -84,7 +84,7 @@ void rdt_last_cmd_printf(const char *fmt, ...)
> *
> * Using a global CLOSID across all resources has some advantages and
> * some drawbacks:
> - * + We can simply set "current->closid" to assign a task to a resource
> + * + We can simply set current's closid to assign a task to a resource
> * group.
Seems this change doesn't gain anything. Maybe this change can be removed?
> * + Context switch code can avoid extra memory references deciding which
> * CLOSID to load into the PQR_ASSOC MSR
> @@ -549,14 +549,26 @@ static void update_task_closid_rmid(struct
> task_struct *t)
> _update_task_closid_rmid(t);
> }
>
> +static bool task_in_rdtgroup(struct task_struct *tsk, struct rdtgroup
> +*rdtgrp) {
> + u32 closid, rmid = rdtgrp->mon.rmid;
> +
> + if (rdtgrp->type == RDTCTRL_GROUP)
> + closid = rdtgrp->closid;
> + else if (rdtgrp->type == RDTMON_GROUP)
> + closid = rdtgrp->mon.parent->closid;
> + else
> + return false;
> +
> + return resctrl_arch_match_closid(tsk, closid) &&
> + resctrl_arch_match_rmid(tsk, closid, rmid); }
> +
> static int __rdtgroup_move_task(struct task_struct *tsk,
> struct rdtgroup *rdtgrp)
> {
> /* If the task is already in rdtgrp, no need to move the task. */
> - if ((rdtgrp->type == RDTCTRL_GROUP && tsk->closid == rdtgrp->closid
> &&
> - tsk->rmid == rdtgrp->mon.rmid) ||
> - (rdtgrp->type == RDTMON_GROUP && tsk->rmid == rdtgrp-
> >mon.rmid &&
> - tsk->closid == rdtgrp->mon.parent->closid))
> + if (task_in_rdtgroup(tsk, rdtgrp))
> return 0;
>
> /*
> @@ -567,19 +579,14 @@ static int __rdtgroup_move_task(struct task_struct
> *tsk,
> * For monitor groups, can move the tasks only from
> * their parent CTRL group.
> */
> -
> - if (rdtgrp->type == RDTCTRL_GROUP) {
> - WRITE_ONCE(tsk->closid, rdtgrp->closid);
> - WRITE_ONCE(tsk->rmid, rdtgrp->mon.rmid);
> - } else if (rdtgrp->type == RDTMON_GROUP) {
> - if (rdtgrp->mon.parent->closid == tsk->closid) {
> - WRITE_ONCE(tsk->rmid, rdtgrp->mon.rmid);
> - } else {
> - rdt_last_cmd_puts("Can't move task to different
> control group\n");
> - return -EINVAL;
> - }
> + if (rdtgrp->type == RDTMON_GROUP &&
> + !resctrl_arch_match_closid(tsk, rdtgrp->mon.parent->closid)) {
> + rdt_last_cmd_puts("Can't move task to different control
> group\n");
> + return -EINVAL;
> }
>
> + resctrl_arch_set_closid_rmid(tsk, rdtgrp->closid, rdtgrp->mon.rmid);
> +
> /*
> * Ensure the task's closid and rmid are written before determining if
> * the task is current that will decide if it will be interrupted.
> @@ -599,14 +606,15 @@ static int __rdtgroup_move_task(struct task_struct
> *tsk,
>
> static bool is_closid_match(struct task_struct *t, struct rdtgroup *r) {
> - return (rdt_alloc_capable &&
> - (r->type == RDTCTRL_GROUP) && (t->closid == r->closid));
> + return (rdt_alloc_capable && (r->type == RDTCTRL_GROUP) &&
> + resctrl_arch_match_closid(t, r->closid));
> }
>
> static bool is_rmid_match(struct task_struct *t, struct rdtgroup *r) {
> - return (rdt_mon_capable &&
> - (r->type == RDTMON_GROUP) && (t->rmid == r->mon.rmid));
> + return (rdt_mon_capable && (r->type == RDTMON_GROUP) &&
> + resctrl_arch_match_rmid(t, r->mon.parent->closid,
> + r->mon.rmid));
> }
>
> /**
> @@ -802,7 +810,7 @@ int proc_resctrl_show(struct seq_file *s, struct
> pid_namespace *ns,
> rdtg->mode != RDT_MODE_EXCLUSIVE)
> continue;
>
> - if (rdtg->closid != tsk->closid)
> + if (!resctrl_arch_match_closid(tsk, rdtg->closid))
> continue;
>
> seq_printf(s, "res:%s%s\n", (rdtg == &rdtgroup_default) ? "/" : "",
> @@ -810,7 +818,8 @@ int proc_resctrl_show(struct seq_file *s, struct
> pid_namespace *ns,
> seq_puts(s, "mon:");
> list_for_each_entry(crg, &rdtg->mon.crdtgrp_list,
> mon.crdtgrp_list) {
> - if (tsk->rmid != crg->mon.rmid)
> + if (!resctrl_arch_match_rmid(tsk, crg->mon.parent-
> >closid,
> + crg->mon.rmid))
> continue;
> seq_printf(s, "%s", crg->kn->name);
> break;
> @@ -2401,8 +2410,8 @@ static void rdt_move_group_tasks(struct rdtgroup
> *from, struct rdtgroup *to,
> for_each_process_thread(p, t) {
> if (!from || is_closid_match(t, from) ||
> is_rmid_match(t, from)) {
> - WRITE_ONCE(t->closid, to->closid);
> - WRITE_ONCE(t->rmid, to->mon.rmid);
> + resctrl_arch_set_closid_rmid(t, to->closid,
> + to->mon.rmid);
>
> /*
> * If the task is on a CPU, set the CPU in the mask.
> --
> 2.30.2
Thanks.
-Fenghua
Hi James,
On 1/13/2023 9:54 AM, James Morse wrote:
...
> @@ -567,19 +579,14 @@ static int __rdtgroup_move_task(struct task_struct *tsk,
> * For monitor groups, can move the tasks only from
> * their parent CTRL group.
> */
> -
> - if (rdtgrp->type == RDTCTRL_GROUP) {
> - WRITE_ONCE(tsk->closid, rdtgrp->closid);
> - WRITE_ONCE(tsk->rmid, rdtgrp->mon.rmid);
> - } else if (rdtgrp->type == RDTMON_GROUP) {
> - if (rdtgrp->mon.parent->closid == tsk->closid) {
> - WRITE_ONCE(tsk->rmid, rdtgrp->mon.rmid);
> - } else {
> - rdt_last_cmd_puts("Can't move task to different control group\n");
> - return -EINVAL;
> - }
> + if (rdtgrp->type == RDTMON_GROUP &&
> + !resctrl_arch_match_closid(tsk, rdtgrp->mon.parent->closid)) {
> + rdt_last_cmd_puts("Can't move task to different control group\n");
> + return -EINVAL;
> }
>
> + resctrl_arch_set_closid_rmid(tsk, rdtgrp->closid, rdtgrp->mon.rmid);
This does not use the intended closid when rdtgrp->type == RDTMON_GROUP.
Reinette
Hi Fenghua,
On 17/01/2023 19:10, Yu, Fenghua wrote:
>> When switching tasks, the CLOSID and RMID that the new task should use are
>> stored in struct task_struct. For x86 the CLOSID known by resctrl, the value in
>> task_struct, and the value written to the CPU register are all the same thing.
>>
>> MPAM's CPU interface has two different PARTID's one for data accesses the
>> other for instruction fetch. Storing resctrl's CLOSID value in struct task_struct
>> implies the arch code knows whether resctrl is using CDP.
>>
>> Move the matching and setting of the struct task_struct properties to use
>> helpers. This allows arm64 to store the hardware format of the register, instead
>> of having to convert it each time.
>>
>> __rdtgroup_move_task()s use of READ_ONCE()/WRITE_ONCE() ensures torn
>> values aren't seen as another CPU may schedule the task being moved while the
>> value is being changed. MPAM has an additional corner-case here as the PMG
>> bits extend the PARTID space. If the scheduler sees a new-CLOSID but old-RMID,
>> the task will dirty an RMID that the limbo code is not watching causing an
>> inaccurate count. x86's RMID are independent values, so the limbo code will still
>> be watching the old-RMID in this circumstance.
>> To avoid this, arm64 needs both the CLOSID/RMID WRITE_ONCE()d together.
>> Both values must be provided together.
>>
>> Because MPAM's RMID values are not unique, the CLOSID must be provided
>> when matching the RMID.
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index e1f879e13823..ced7400decae 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -84,7 +84,7 @@ void rdt_last_cmd_printf(const char *fmt, ...)
>> *
>> * Using a global CLOSID across all resources has some advantages and
>> * some drawbacks:
>> - * + We can simply set "current->closid" to assign a task to a resource
>> + * + We can simply set current's closid to assign a task to a resource
>> * group.
>
> Seems this change doesn't gain anything. Maybe this change can be removed?
After this patch the CLOSID might not be in current at all, this comment would be the only
thing that suggests it is. I'd prefer not to suggest anyone access 'current->closid'
directly in resctrl, as such code wouldn't compile on arm64.
This is a 'saves bugs in the future' change.
>> * + Context switch code can avoid extra memory references deciding which
>> * CLOSID to load into the PQR_ASSOC MSR
(please trim your replies!)
Thanks,
James
Hi Reinette,
On 02/02/2023 23:47, Reinette Chatre wrote:
> On 1/13/2023 9:54 AM, James Morse wrote:
>
> ...
>
>> @@ -567,19 +579,14 @@ static int __rdtgroup_move_task(struct task_struct *tsk,
>> * For monitor groups, can move the tasks only from
>> * their parent CTRL group.
>> */
>> -
>> - if (rdtgrp->type == RDTCTRL_GROUP) {
>> - WRITE_ONCE(tsk->closid, rdtgrp->closid);
>> - WRITE_ONCE(tsk->rmid, rdtgrp->mon.rmid);
>> - } else if (rdtgrp->type == RDTMON_GROUP) {
>> - if (rdtgrp->mon.parent->closid == tsk->closid) {
>> - WRITE_ONCE(tsk->rmid, rdtgrp->mon.rmid);
>> - } else {
>> - rdt_last_cmd_puts("Can't move task to different control group\n");
>> - return -EINVAL;
>> - }
>> + if (rdtgrp->type == RDTMON_GROUP &&
>> + !resctrl_arch_match_closid(tsk, rdtgrp->mon.parent->closid)) {
>> + rdt_last_cmd_puts("Can't move task to different control group\n");
>> + return -EINVAL;
>> }
>>
>> + resctrl_arch_set_closid_rmid(tsk, rdtgrp->closid, rdtgrp->mon.rmid);
>
> This does not use the intended closid when rdtgrp->type == RDTMON_GROUP.
Yes, it should be rdtgrp->mon.parent->closid.
rdtgroup_mkdir_mon() initialises them to be the same, I guess its Peter's monitor-group
rename that means this could get the wrong value?
I've fixed it as:
---------%<---------
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index c7392d10dc5b..30d8961b833c 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -585,7 +585,12 @@ static int __rdtgroup_move_task(struct task_struct *tsk,
return -EINVAL;
}
- resctrl_arch_set_closid_rmid(tsk, rdtgrp->closid, rdtgrp->mon.rmid);
+ if (rdtgrp->type == RDTMON_GROUP)
+ resctrl_arch_set_closid_rmid(tsk, rdtgrp->mon.parent->closid,
+ rdtgrp->mon.rmid);
+ else
+ resctrl_arch_set_closid_rmid(tsk, rdtgrp->closid,
+ rdtgrp->mon.rmid);
/*
* Ensure the task's closid and rmid are written before determining if
---------%<---------
Thanks,
James
Hi James,
On Mon, Mar 6, 2023 at 12:32 PM James Morse <[email protected]> wrote:
> On 02/02/2023 23:47, Reinette Chatre wrote:
> > On 1/13/2023 9:54 AM, James Morse wrote:
> >> + resctrl_arch_set_closid_rmid(tsk, rdtgrp->closid, rdtgrp->mon.rmid);
> >
> > This does not use the intended closid when rdtgrp->type == RDTMON_GROUP.
>
> Yes, it should be rdtgrp->mon.parent->closid.
>
> rdtgroup_mkdir_mon() initialises them to be the same, I guess its Peter's monitor-group
> rename that means this could get the wrong value?
I noticed this earlier. The next revision of my MON group rename patch
series updates rdtgrp->closid to that of the new parent so that for
MON groups, rdtgrp->closid == rdtgrp->mon.parent->closid continues to
hold.
It looks like rdt_move_group_tasks() assumes this, as it always sets
t->closid to rdtgrp->closid.
-Peter
Hi James,
On 3/6/2023 3:32 AM, James Morse wrote:
> On 02/02/2023 23:47, Reinette Chatre wrote:
>> On 1/13/2023 9:54 AM, James Morse wrote:
>>
>> ...
>>
>>> @@ -567,19 +579,14 @@ static int __rdtgroup_move_task(struct task_struct *tsk,
>>> * For monitor groups, can move the tasks only from
>>> * their parent CTRL group.
>>> */
>>> -
>>> - if (rdtgrp->type == RDTCTRL_GROUP) {
>>> - WRITE_ONCE(tsk->closid, rdtgrp->closid);
>>> - WRITE_ONCE(tsk->rmid, rdtgrp->mon.rmid);
>>> - } else if (rdtgrp->type == RDTMON_GROUP) {
>>> - if (rdtgrp->mon.parent->closid == tsk->closid) {
>>> - WRITE_ONCE(tsk->rmid, rdtgrp->mon.rmid);
>>> - } else {
>>> - rdt_last_cmd_puts("Can't move task to different control group\n");
>>> - return -EINVAL;
>>> - }
>>> + if (rdtgrp->type == RDTMON_GROUP &&
>>> + !resctrl_arch_match_closid(tsk, rdtgrp->mon.parent->closid)) {
>>> + rdt_last_cmd_puts("Can't move task to different control group\n");
>>> + return -EINVAL;
>>> }
>>>
>>> + resctrl_arch_set_closid_rmid(tsk, rdtgrp->closid, rdtgrp->mon.rmid);
>>
>> This does not use the intended closid when rdtgrp->type == RDTMON_GROUP.
>
> Yes, it should be rdtgrp->mon.parent->closid.
>
> rdtgroup_mkdir_mon() initialises them to be the same
Even so, I do find the code to be confusing when it jumps from one to
the other within the same function.
>, I guess its Peter's monitor-group
> rename that means this could get the wrong value?
>
> I've fixed it as:
> ---------%<---------
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index c7392d10dc5b..30d8961b833c 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -585,7 +585,12 @@ static int __rdtgroup_move_task(struct task_struct *tsk,
> return -EINVAL;
> }
>
> - resctrl_arch_set_closid_rmid(tsk, rdtgrp->closid, rdtgrp->mon.rmid);
> + if (rdtgrp->type == RDTMON_GROUP)
> + resctrl_arch_set_closid_rmid(tsk, rdtgrp->mon.parent->closid,
> + rdtgrp->mon.rmid);
> + else
> + resctrl_arch_set_closid_rmid(tsk, rdtgrp->closid,
> + rdtgrp->mon.rmid);
>
> /*
> * Ensure the task's closid and rmid are written before determining if
> ---------%<---------
>
>
ok, thank you.
Reinette