2024-03-25 18:08:23

by Peter Newman

Subject: [PATCH v1 3/6] x86/resctrl: Disallow mongroup rename on MPAM

Moving a monitoring group to a different parent control assumes that the
monitors will not be impacted. This is not the case on MPAM where the
PMG is an extension of the PARTID.

Detect this situation by requiring the change in CLOSID not to affect
the result of resctrl_arch_rmid_idx_encode(), otherwise return
-EOPNOTSUPP.

Signed-off-by: Peter Newman <[email protected]>
---
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 13 +++++++++++++
1 file changed, 13 insertions(+)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 9b1969e4235a..8d6979dbfd02 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -3879,6 +3879,19 @@ static int rdtgroup_rename(struct kernfs_node *kn,
goto out;
}

+ /*
+ * If changing the CLOSID impacts the RMID, this operation is not
+ * supported.
+ */
+ if (resctrl_arch_rmid_idx_encode(rdtgrp->mon.parent->closid,
+ rdtgrp->mon.rmid) !=
+ resctrl_arch_rmid_idx_encode(new_prdtgrp->closid,
+ rdtgrp->mon.rmid)) {
+ rdt_last_cmd_puts("changing parent control group not supported\n");
+ ret = -EOPNOTSUPP;
+ goto out;
+ }
+
/*
* If the MON group is monitoring CPUs, the CPUs must be assigned to the
* current parent CTRL_MON group and therefore cannot be assigned to
--
2.44.0.396.g6e790dbe36-goog

2024-04-04 23:11:52

by Reinette Chatre

Subject: Re: [PATCH v1 3/6] x86/resctrl: Disallow mongroup rename on MPAM

Hi Peter,

On 3/25/2024 10:27 AM, Peter Newman wrote:
> Moving a monitoring group to a different parent control assumes that the
> monitors will not be impacted. This is not the case on MPAM where the
> PMG is an extension of the PARTID.
>
> Detect this situation by requiring the change in CLOSID not to affect
> the result of resctrl_arch_rmid_idx_encode(), otherwise return
> -EOPNOTSUPP.
>

Thanks for catching this. This seems out of place in this series. It sounds
more like an independent fix that should go in separately.

> Signed-off-by: Peter Newman <[email protected]>
> ---
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 9b1969e4235a..8d6979dbfd02 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -3879,6 +3879,19 @@ static int rdtgroup_rename(struct kernfs_node *kn,
> goto out;
> }
>
> + /*
> + * If changing the CLOSID impacts the RMID, this operation is not
> + * supported.
> + */
> + if (resctrl_arch_rmid_idx_encode(rdtgrp->mon.parent->closid,
> + rdtgrp->mon.rmid) !=
> + resctrl_arch_rmid_idx_encode(new_prdtgrp->closid,
> + rdtgrp->mon.rmid)) {
> + rdt_last_cmd_puts("changing parent control group not supported\n");

Please start message with capital letter.

> + ret = -EOPNOTSUPP;
> + goto out;
> + }
> +
> /*
> * If the MON group is monitoring CPUs, the CPUs must be assigned to the
> * current parent CTRL_MON group and therefore cannot be assigned to

Reinette

2024-04-05 22:10:27

by Peter Newman

Subject: Re: [PATCH v1 3/6] x86/resctrl: Disallow mongroup rename on MPAM

Hi Reinette,

On Thu, Apr 4, 2024 at 4:11 PM Reinette Chatre
<[email protected]> wrote:
>
> Hi Peter,
>
> On 3/25/2024 10:27 AM, Peter Newman wrote:
> > Moving a monitoring group to a different parent control assumes that the
> > monitors will not be impacted. This is not the case on MPAM where the
> > PMG is an extension of the PARTID.
> >
> > Detect this situation by requiring the change in CLOSID not to affect
> > the result of resctrl_arch_rmid_idx_encode(), otherwise return
> > -EOPNOTSUPP.
> >
>
> Thanks for catching this. This seems out of place in this series. It sounds
> more like an independent fix that should go in separately.

I asserted in a comment in a patch later in the series that the
mongroup parent pointer never changes on MPAM, then decided to follow
up on whether it was actually true, so it's only here because this
series depends on it. I'll post it again separately with the fix you
requested below.

Thanks!
-Peter