Messages in this thread | | | Date | Fri, 10 Mar 2023 12:00:02 -0800 | Subject | Re: [PATCH v2 07/18] x86/resctrl: Move CLOSID/RMID matching and setting to use helpers | From | Reinette Chatre <> |
| |
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
| |