[PATCH v5 0/3] Subject: x86/resctrl: Implement rename to help move containers' tasks

[PATCH v5 0/3] Subject: x86/resctrl: Implement rename to help move containers' tasks

[Peter Newman]

Hi Reinette, Fenghua,

This patch series implements the solution Reinette suggested in the
earlier RFD thread[1] for the problem of moving a container's tasks to a
different control group on systems that don't provide enough CLOSIDs to
give every container its own control group.

This patch series assumes that a MON group's CLOSID can simply be
changed to that of a new parent CTRL_MON group. This is allowed on Intel
and AMD, but not MPAM implementations. While we (Google) only foresee
needing this functionality on Intel and AMD systems, this series should
hopefully be a good starting point for supporting MPAM.

Thanks!
-Peter

Updates:

v5:
 - rebase to v6.3-rc4
 - dropped rdt_move_group_tasks() task filter patch
 - code/comment clarifications and errno updates requested by Reinette
 - added Documentation patch

v4:
 - rebase to v6.2
 - commit message updates suggested by Reinette
 - replace rdt_move_one_task() patch with rdt_move_group_tasks() filter
   function patch
 - prevent rename on files or renaming to "mon_groups"
 - optimize simple rename case
 - disallow renaming groups with non-empty cpumask
 - ensure source is a proper MON group directory
 - fix missing rdtgrp->closid update
 - add more last_command_status output

v3: use revised task CLOSID/RMID update IPI sync method from [3]
v2: reworded change logs based on what I've learned from review comments
    in another patch series[2]

[v1] https://lore.kernel.org/lkml/20221115154515.952783-1-peternewman@google.com/
[v2] https://lore.kernel.org/lkml/20221129120149.1035444-1-peternewman@google.com/
[v3] https://lore.kernel.org/lkml/20230125101334.1069060-1-peternewman@google.com/
[v4] https://lore.kernel.org/lkml/20230308131452.383914-1-peternewman@google.com/

[1] https://lore.kernel.org/lkml/7b09fb62-e61a-65b9-a71e-ab725f527ded@intel.com/
[2] https://lore.kernel.org/lkml/54e50a9b-268f-2020-f54c-d38312489e2f@intel.com/
[3] https://lore.kernel.org/lkml/20221220161123.432120-1-peternewman@google.com/

Peter Newman (3):
  x86/resctrl: Factor rdtgroup lock for multi-file ops
  x86/resctrl: Implement rename op for mon groups
  Documentation/x86: Documentation for MON group move feature

 Documentation/x86/resctrl.rst          |   7 ++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 163 +++++++++++++++++++++++--
 2 files changed, 157 insertions(+), 13 deletions(-)


base-commit: 197b6b60ae7bc51dd0814953c562833143b292aa
-- 
2.40.0.348.gf938b09366-goog

[PATCH v5 1/3] x86/resctrl: Factor rdtgroup lock for multi-file ops

[Peter Newman]

rdtgroup_kn_lock_live() can only release a kernfs reference for a single
file before waiting on the rdtgroup_mutex, limiting its usefulness for
operations on multiple files, such as rename.

Factor the work needed to respectively break and unbreak active
protection on an individual file into rdtgroup_kn_{get,put}().

No functional change.

Signed-off-by: Peter Newman <peternewman@google.com>
Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 35 ++++++++++++++++----------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 6ad33f355861..51b869149e76 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2301,6 +2301,26 @@ static struct rdtgroup *kernfs_to_rdtgroup(struct kernfs_node *kn)
 	}
 }
 
+static void rdtgroup_kn_get(struct rdtgroup *rdtgrp, struct kernfs_node *kn)
+{
+	atomic_inc(&rdtgrp->waitcount);
+	kernfs_break_active_protection(kn);
+}
+
+static void rdtgroup_kn_put(struct rdtgroup *rdtgrp, struct kernfs_node *kn)
+{
+	if (atomic_dec_and_test(&rdtgrp->waitcount) &&
+	    (rdtgrp->flags & RDT_DELETED)) {
+		if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP ||
+		    rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED)
+			rdtgroup_pseudo_lock_remove(rdtgrp);
+		kernfs_unbreak_active_protection(kn);
+		rdtgroup_remove(rdtgrp);
+	} else {
+		kernfs_unbreak_active_protection(kn);
+	}
+}
+
 struct rdtgroup *rdtgroup_kn_lock_live(struct kernfs_node *kn)
 {
 	struct rdtgroup *rdtgrp = kernfs_to_rdtgroup(kn);
@@ -2308,8 +2328,7 @@ struct rdtgroup *rdtgroup_kn_lock_live(struct kernfs_node *kn)
 	if (!rdtgrp)
 		return NULL;
 
-	atomic_inc(&rdtgrp->waitcount);
-	kernfs_break_active_protection(kn);
+	rdtgroup_kn_get(rdtgrp, kn);
 
 	mutex_lock(&rdtgroup_mutex);
 
@@ -2328,17 +2347,7 @@ void rdtgroup_kn_unlock(struct kernfs_node *kn)
 		return;
 
 	mutex_unlock(&rdtgroup_mutex);
-
-	if (atomic_dec_and_test(&rdtgrp->waitcount) &&
-	    (rdtgrp->flags & RDT_DELETED)) {
-		if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP ||
-		    rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED)
-			rdtgroup_pseudo_lock_remove(rdtgrp);
-		kernfs_unbreak_active_protection(kn);
-		rdtgroup_remove(rdtgrp);
-	} else {
-		kernfs_unbreak_active_protection(kn);
-	}
+	rdtgroup_kn_put(rdtgrp, kn);
 }
 
 static int mkdir_mondata_all(struct kernfs_node *parent_kn,
-- 
2.40.0.348.gf938b09366-goog

[PATCH v5 2/3] x86/resctrl: Implement rename op for mon groups

[Peter Newman]

To change the resources allocated to a large group of tasks, such as an
application container, a container manager must write all of the tasks'
IDs into the tasks file interface of the new control group.

If a container manager is additionally tracking containers' bandwidth
usage by placing tasks from each into their own monitoring group, it
must first move the tasks to the default monitoring group of the new
control group before it can move the tasks into their new monitoring
groups. This is undesirable because it makes bandwidth usage during the
move unattributable to the correct tasks and resets monitoring event
counters and cache usage information for the group.

Implement the rename operation only for resctrlfs monitor groups to
enable users to move a monitoring group from one control group to
another. This effects a change in resources allocated to all the tasks
in the monitoring group while otherwise leaving the monitoring data
intact.

Signed-off-by: Peter Newman <peternewman@google.com>
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 128 +++++++++++++++++++++++++
 1 file changed, 128 insertions(+)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 51b869149e76..86de22d8e23a 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -3514,6 +3514,133 @@ static int rdtgroup_rmdir(struct kernfs_node *kn)
 	return ret;
 }
 
+/**
+ * mongrp_reparent() - replace parent CTRL_MON group of a MON group
+ * @rdtgrp:		the MON group whose parent should be replaced
+ * @new_prdtgrp:	replacement parent CTRL_MON group for @rdtgrp
+ * @cpus:		cpumask provided by the caller for use during this call
+ *
+ * Replaces the parent CTRL_MON group for a MON group, resulting in all member
+ * tasks' CLOSID immediately changing to that of the new parent group.
+ * Monitoring data for the group is unaffected by this operation.
+ */
+static void mongrp_reparent(struct rdtgroup *rdtgrp,
+			    struct rdtgroup *new_prdtgrp,
+			    cpumask_var_t cpus)
+{
+	struct rdtgroup *prdtgrp = rdtgrp->mon.parent;
+
+	WARN_ON(rdtgrp->type != RDTMON_GROUP);
+	WARN_ON(new_prdtgrp->type != RDTCTRL_GROUP);
+
+	/* Nothing to do when simply renaming a MON group. */
+	if (prdtgrp == new_prdtgrp)
+		return;
+
+	WARN_ON(list_empty(&prdtgrp->mon.crdtgrp_list));
+	list_move_tail(&rdtgrp->mon.crdtgrp_list,
+		       &new_prdtgrp->mon.crdtgrp_list);
+
+	rdtgrp->mon.parent = new_prdtgrp;
+	rdtgrp->closid = new_prdtgrp->closid;
+
+	/* Propagate updated closid to all tasks in this group. */
+	rdt_move_group_tasks(rdtgrp, rdtgrp, cpus);
+
+	update_closid_rmid(cpus, NULL);
+}
+
+static int rdtgroup_rename(struct kernfs_node *kn,
+			   struct kernfs_node *new_parent, const char *new_name)
+{
+	struct rdtgroup *new_prdtgrp;
+	struct rdtgroup *rdtgrp;
+	cpumask_var_t tmpmask;
+	int ret;
+
+	rdtgrp = kernfs_to_rdtgroup(kn);
+	new_prdtgrp = kernfs_to_rdtgroup(new_parent);
+	if (!rdtgrp || !new_prdtgrp)
+		return -ENOENT;
+
+	/* Release both kernfs active_refs before obtaining rdtgroup mutex. */
+	rdtgroup_kn_get(rdtgrp, kn);
+	rdtgroup_kn_get(new_prdtgrp, new_parent);
+
+	mutex_lock(&rdtgroup_mutex);
+
+	rdt_last_cmd_clear();
+
+	/*
+	 * Don't allow kernfs_to_rdtgroup() to return a parent rdtgroup if
+	 * either kernfs_node is a file.
+	 */
+	if (kernfs_type(kn) != KERNFS_DIR ||
+	    kernfs_type(new_parent) != KERNFS_DIR) {
+		rdt_last_cmd_puts("Source and destination must be group directories");
+		ret = -EPERM;
+		goto out;
+	}
+
+	if ((rdtgrp->flags & RDT_DELETED) || (new_prdtgrp->flags & RDT_DELETED)) {
+		ret = -ENOENT;
+		goto out;
+	}
+
+	if (rdtgrp->type != RDTMON_GROUP || !kn->parent ||
+	    !is_mon_groups(kn->parent, kn->name)) {
+		rdt_last_cmd_puts("Source must be a MON group\n");
+		ret = -EPERM;
+		goto out;
+	}
+
+	if (!is_mon_groups(new_parent, new_name)) {
+		rdt_last_cmd_puts("Destination must be a mon_groups subdirectory\n");
+		ret = -EPERM;
+		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
+	 * the new parent, making the move illegal.
+	 */
+	if (!cpumask_empty(&rdtgrp->cpu_mask) &&
+	    (rdtgrp->mon.parent != new_prdtgrp)) {
+		rdt_last_cmd_puts("Cannot move a MON group that monitors CPUs\n");
+		ret = -EPERM;
+		goto out;
+	}
+
+	/*
+	 * Allocate the cpumask for use in mongrp_reparent() to avoid the
+	 * possibility of failing to allocate it after kernfs_rename() has
+	 * succeeded.
+	 */
+	if (!zalloc_cpumask_var(&tmpmask, GFP_KERNEL)) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	/*
+	 * Perform all input validation and allocations needed to ensure
+	 * mongrp_reparent() will succeed before calling kernfs_rename(),
+	 * otherwise it would be necessary to revert this call if
+	 * mongrp_reparent() failed.
+	 */
+	ret = kernfs_rename(kn, new_parent, new_name);
+	if (!ret)
+		mongrp_reparent(rdtgrp, new_prdtgrp, tmpmask);
+
+	free_cpumask_var(tmpmask);
+
+out:
+	mutex_unlock(&rdtgroup_mutex);
+	rdtgroup_kn_put(rdtgrp, kn);
+	rdtgroup_kn_put(new_prdtgrp, new_parent);
+	return ret;
+}
+
 static int rdtgroup_show_options(struct seq_file *seq, struct kernfs_root *kf)
 {
 	if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L3))
@@ -3531,6 +3658,7 @@ static int rdtgroup_show_options(struct seq_file *seq, struct kernfs_root *kf)
 static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
 	.mkdir		= rdtgroup_mkdir,
 	.rmdir		= rdtgroup_rmdir,
+	.rename		= rdtgroup_rename,
 	.show_options	= rdtgroup_show_options,
 };
 
-- 
2.40.0.348.gf938b09366-goog

[PATCH v5 3/3] Documentation/x86: Documentation for MON group move feature

[Peter Newman]

Describe new support for moving MON groups to a new parent CTRL_MON
group and its restrictions.

Signed-off-by: Peter Newman <peternewman@google.com>
---
 Documentation/x86/resctrl.rst | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
index 387ccbcb558f..cb05d90111b4 100644
--- a/Documentation/x86/resctrl.rst
+++ b/Documentation/x86/resctrl.rst
@@ -287,6 +287,13 @@ Removing a directory will move all tasks and cpus owned by the group it
 represents to the parent. Removing one of the created CTRL_MON groups
 will automatically remove all MON groups below it.
 
+Moving MON group directories to a new parent CTRL_MON group is supported
+for the purpose of changing the resource allocations of a MON group
+without impacting its monitoring data or assigned tasks. This operation
+is not allowed for MON groups which monitor CPUs. No other move
+operation is currently allowed other than simply renaming a CTRL_MON or
+MON group.
+
 All groups contain the following files:
 
 "tasks":
-- 
2.40.0.348.gf938b09366-goog

Re: [PATCH v5 3/3] Documentation/x86: Documentation for MON group move feature

[Moger, Babu]

Hi Peter,

On 3/30/23 08:55, Peter Newman wrote:
> Describe new support for moving MON groups to a new parent CTRL_MON
> group and its restrictions.

Sorry for coming in late here. I am planning to test these patches. It
would be helpful to give a simple example to test this feature.
Thanks
Babu

> 
> Signed-off-by: Peter Newman <peternewman@google.com>
> ---
>  Documentation/x86/resctrl.rst | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
> index 387ccbcb558f..cb05d90111b4 100644
> --- a/Documentation/x86/resctrl.rst
> +++ b/Documentation/x86/resctrl.rst
> @@ -287,6 +287,13 @@ Removing a directory will move all tasks and cpus owned by the group it
>  represents to the parent. Removing one of the created CTRL_MON groups
>  will automatically remove all MON groups below it.
>  
> +Moving MON group directories to a new parent CTRL_MON group is supported
> +for the purpose of changing the resource allocations of a MON group
> +without impacting its monitoring data or assigned tasks. This operation
> +is not allowed for MON groups which monitor CPUs. No other move
> +operation is currently allowed other than simply renaming a CTRL_MON or
> +MON group.
> +
>  All groups contain the following files:
>  
>  "tasks":

Re: [PATCH v5 3/3] Documentation/x86: Documentation for MON group move feature

[Peter Newman]

Hi Babu,

On Thu, Mar 30, 2023 at 7:33 PM Moger, Babu <babu.moger@amd.com> wrote:
> On 3/30/23 08:55, Peter Newman wrote:
> > Describe new support for moving MON groups to a new parent CTRL_MON
> > group and its restrictions.
>
> Sorry for coming in late here. I am planning to test these patches. It
> would be helpful to give a simple example to test this feature.

Do you mean inline in the documentation?

For now, you can also try the patch below. These are the testcases I
used.

I'm planning to convert many of our internal, shell script-based test
cases into kernel selftests so I can try to upstream them.

Thanks!
-Peter


---8<-------
From f6d215e90db3c416bd39889b9fa1143d798245e0 Mon Sep 17 00:00:00 2001
From: Peter Newman <peternewman@google.com>
Date: Tue, 7 Mar 2023 11:57:03 +0100
Subject: [PATCH] selftests/resctrl: Test for MON group renaming

Signed-off-by: Peter Newman <peternewman@google.com>
---
 tools/testing/selftests/resctrl/Makefile      |   2 +
 .../selftests/resctrl/test_mongrp_move.sh     | 120 ++++++++++++++++++
 2 files changed, 122 insertions(+)
 create mode 100755 tools/testing/selftests/resctrl/test_mongrp_move.sh

diff --git a/tools/testing/selftests/resctrl/Makefile b/tools/testing/selftests/resctrl/Makefile
index 73d53257df42..0b696e7cf19b 100644
--- a/tools/testing/selftests/resctrl/Makefile
+++ b/tools/testing/selftests/resctrl/Makefile
@@ -5,6 +5,8 @@ CFLAGS += $(KHDR_INCLUDES)
 
 TEST_GEN_PROGS := resctrl_tests
 
+TEST_PROGS := test_mongrp_move.sh
+
 include ../lib.mk
 
 $(OUTPUT)/resctrl_tests: $(wildcard *.c)
diff --git a/tools/testing/selftests/resctrl/test_mongrp_move.sh b/tools/testing/selftests/resctrl/test_mongrp_move.sh
new file mode 100755
index 000000000000..6d9bfc4e0c8d
--- /dev/null
+++ b/tools/testing/selftests/resctrl/test_mongrp_move.sh
@@ -0,0 +1,120 @@
+#!/bin/sh
+
+set -e
+
+rc=0
+
+cleanup()
+{
+	rmdir _test_*
+	rmdir mon_groups/_test_*
+}
+
+skip_all()
+{
+	echo Bail out! $1
+
+	cleanup
+
+	# SKIP code is 4.
+	exit 4
+}
+
+expect_success()
+{
+	if [ "$1" -eq 0 ]; then
+		echo ok $2
+	else
+		echo not ok $2
+		rc=1
+	fi
+}
+
+expect_fail()
+{
+	if [ "$1" -eq 0 ]; then
+		echo not ok $2
+		rc=1
+	else
+		echo ok $2
+	fi
+}
+
+if [ "$(id -u)" != 0 ]; then
+	skip_all "must be run as root"
+fi
+
+if [ ! -d /sys/fs/resctrl/info ]; then
+	mount -t resctrl resctrl /sys/fs/resctrl || skip_all "no resctrlfs"
+fi
+
+cd /sys/fs/resctrl
+
+if [ ! -f info/L3_MON/mon_features ]; then
+	skip_all "no monitoring support"
+fi
+
+if [ ! -f schemata ]; then
+	skip_all "no allocation support"
+fi
+
+echo "1..11"
+
+if [ -d _test_c1 ] || [ -d _test_c2 ] || [ -d mon_groups/_test_m1 ]; then
+	skip_all "test directories already exist"
+fi
+
+mkdir _test_c1
+mkdir _test_c2
+mkdir _test_c3
+
+mkdir mon_groups/_test_m1
+echo 1 > mon_groups/_test_m1/cpus
+
+mkdir _test_c1/mon_groups/_test_m1
+echo $$ > _test_c1/tasks
+echo $$ > _test_c1/mon_groups/_test_m1/tasks
+
+if mv _test_c1/mon_groups/_test_m1 _test_c2/mon_groups; then
+	echo "ok 1 # MON group move to new parent succeeded"
+else
+	echo "1..0 # skip because MON group move to new parent not supported"
+	cleanup
+	exit 4
+fi
+
+set +e
+
+grep -q $$ _test_c2/tasks
+expect_success $? "2 # PID in new CTRL_MON group"
+
+grep -q $$ _test_c2/mon_groups/_test_m1/tasks
+expect_success $? "3 # PID remains in MON group after move"
+
+grep -q $$ _test_c1/tasks
+expect_fail $? "4 # PID no longer in previous CTRL_MON group"
+
+mv _test_c2/mon_groups/_test_m1/cpus mon_groups
+expect_fail $? "5 # moving files not allowed"
+
+mv _test_c2/mon_groups/_test_m1 _test_c2/mon_groups/_test_m2
+expect_success $? "6 # simple MON directory rename"
+
+mv _test_c2/mon_groups/_test_m2 info
+expect_fail $? "7 # move to info not allowed"
+
+mv _test_c2/mon_groups/_test_m2 _test_c2/mon_groups/mon_groups
+expect_fail $? "8 # rename to mon_groups not allowed"
+
+mv mon_groups/_test_m1 _test_c1/mon_groups
+expect_fail $? "9 # cannot move groups monitoring CPUs"
+
+mv mon_groups/_test_m1 mon_groups/_test_m2
+expect_success $? "10 # groups monitoring CPUs can be renamed"
+
+mv mon_groups/_test_m2/mon_data _test_c1/mon_groups
+expect_fail $? "11 # cannot move subdirectories of a mon_group"
+
+cleanup
+
+exit $rc
-- 
2.40.0.423.gd6c402a77b-goog

RE: [PATCH v5 3/3] Documentation/x86: Documentation for MON group move feature

[Moger, Babu]

[AMD Official Use Only - General]

Hi Peter,

> -----Original Message-----
> From: Peter Newman <peternewman@google.com>
> Sent: Friday, March 31, 2023 3:12 AM
> To: Moger, Babu <Babu.Moger@amd.com>
> Cc: bp@alien8.de; dave.hansen@linux.intel.com; eranian@google.com;
> fenghua.yu@intel.com; gupasani@google.com; hpa@zytor.com;
> james.morse@arm.com; linux-kernel@vger.kernel.org; mingo@redhat.com;
> peternewman@google.com; reinette.chatre@intel.com; skodak@google.com;
> tglx@linutronix.de; tony.luck@intel.com; x86@kernel.org
> Subject: Re: [PATCH v5 3/3] Documentation/x86: Documentation for MON
> group move feature
> 
> Hi Babu,
> 
> On Thu, Mar 30, 2023 at 7:33 PM Moger, Babu <babu.moger@amd.com>
> wrote:
> > On 3/30/23 08:55, Peter Newman wrote:
> > > Describe new support for moving MON groups to a new parent CTRL_MON
> > > group and its restrictions.
> >
> > Sorry for coming in late here. I am planning to test these patches. It
> > would be helpful to give a simple example to test this feature.
> 
> Do you mean inline in the documentation?

Yes. It would be helpful.  I used these simple steps from your script.
# mount  -t resctrl resctrl /sys/fs/resctrl/
# cd /sys/fs/resctrl/
# mkdir _test_c1
# mkdir _test_c2
l# mkdir mon_groups/_test_m1
# echo 1 > mon_groups/_test_m1/cpus
# mkdir _test_c1/mon_groups/_test_c1_m1
l#  echo $$ > _test_c1/tasks
l#  echo $$ > _test_c1/mon_groups/_test_c1_m1/tasks
l#  mv _test_c1/mon_groups/_test_c1_m1/ _test_c2/mon_groups/
# cat info/last_cmd_status
ok
l#  mv mon_groups/_test_m1 _test_c1/mon_groups/
mv: cannot move 'mon_groups/_test_m1' to '_test_c1/mon_groups/_test_m1': Operation not permitted
# cat info/last_cmd_status
Cannot move a MON group that monitors CPUs

> 
> For now, you can also try the patch below. These are the testcases I used.
> 
> I'm planning to convert many of our internal, shell script-based test cases into
> kernel selftests so I can try to upstream them.

Yes. That will be great.
Thanks
Babu


> 
> Thanks!
> -Peter
> 
> 
> ---8<-------
> From f6d215e90db3c416bd39889b9fa1143d798245e0 Mon Sep 17 00:00:00
> 2001
> From: Peter Newman <peternewman@google.com>
> Date: Tue, 7 Mar 2023 11:57:03 +0100
> Subject: [PATCH] selftests/resctrl: Test for MON group renaming
> 
> Signed-off-by: Peter Newman <peternewman@google.com>
> ---
>  tools/testing/selftests/resctrl/Makefile      |   2 +
>  .../selftests/resctrl/test_mongrp_move.sh     | 120 ++++++++++++++++++
>  2 files changed, 122 insertions(+)
>  create mode 100755 tools/testing/selftests/resctrl/test_mongrp_move.sh
> 
> diff --git a/tools/testing/selftests/resctrl/Makefile
> b/tools/testing/selftests/resctrl/Makefile
> index 73d53257df42..0b696e7cf19b 100644
> --- a/tools/testing/selftests/resctrl/Makefile
> +++ b/tools/testing/selftests/resctrl/Makefile
> @@ -5,6 +5,8 @@ CFLAGS += $(KHDR_INCLUDES)
> 
>  TEST_GEN_PROGS := resctrl_tests
> 
> +TEST_PROGS := test_mongrp_move.sh
> +
>  include ../lib.mk
> 
>  $(OUTPUT)/resctrl_tests: $(wildcard *.c) diff --git
> a/tools/testing/selftests/resctrl/test_mongrp_move.sh
> b/tools/testing/selftests/resctrl/test_mongrp_move.sh
> new file mode 100755
> index 000000000000..6d9bfc4e0c8d
> --- /dev/null
> +++ b/tools/testing/selftests/resctrl/test_mongrp_move.sh
> @@ -0,0 +1,120 @@
> +#!/bin/sh
> +
> +set -e
> +
> +rc=0
> +
> +cleanup()
> +{
> +	rmdir _test_*
> +	rmdir mon_groups/_test_*
> +}
> +
> +skip_all()
> +{
> +	echo Bail out! $1
> +
> +	cleanup
> +
> +	# SKIP code is 4.
> +	exit 4
> +}
> +
> +expect_success()
> +{
> +	if [ "$1" -eq 0 ]; then
> +		echo ok $2
> +	else
> +		echo not ok $2
> +		rc=1
> +	fi
> +}
> +
> +expect_fail()
> +{
> +	if [ "$1" -eq 0 ]; then
> +		echo not ok $2
> +		rc=1
> +	else
> +		echo ok $2
> +	fi
> +}
> +
> +if [ "$(id -u)" != 0 ]; then
> +	skip_all "must be run as root"
> +fi
> +
> +if [ ! -d /sys/fs/resctrl/info ]; then
> +	mount -t resctrl resctrl /sys/fs/resctrl || skip_all "no resctrlfs"
> +fi
> +
> +cd /sys/fs/resctrl
> +
> +if [ ! -f info/L3_MON/mon_features ]; then
> +	skip_all "no monitoring support"
> +fi
> +
> +if [ ! -f schemata ]; then
> +	skip_all "no allocation support"
> +fi
> +
> +echo "1..11"
> +
> +if [ -d _test_c1 ] || [ -d _test_c2 ] || [ -d mon_groups/_test_m1 ]; then
> +	skip_all "test directories already exist"
> +fi
> +
> +mkdir _test_c1
> +mkdir _test_c2
> +mkdir _test_c3
> +
> +mkdir mon_groups/_test_m1
> +echo 1 > mon_groups/_test_m1/cpus
> +
> +mkdir _test_c1/mon_groups/_test_m1
> +echo $$ > _test_c1/tasks
> +echo $$ > _test_c1/mon_groups/_test_m1/tasks
> +
> +if mv _test_c1/mon_groups/_test_m1 _test_c2/mon_groups; then
> +	echo "ok 1 # MON group move to new parent succeeded"
> +else
> +	echo "1..0 # skip because MON group move to new parent not
> supported"
> +	cleanup
> +	exit 4
> +fi
> +
> +set +e
> +
> +grep -q $$ _test_c2/tasks
> +expect_success $? "2 # PID in new CTRL_MON group"
> +
> +grep -q $$ _test_c2/mon_groups/_test_m1/tasks
> +expect_success $? "3 # PID remains in MON group after move"
> +
> +grep -q $$ _test_c1/tasks
> +expect_fail $? "4 # PID no longer in previous CTRL_MON group"
> +
> +mv _test_c2/mon_groups/_test_m1/cpus mon_groups expect_fail $? "5 #
> +moving files not allowed"
> +
> +mv _test_c2/mon_groups/_test_m1 _test_c2/mon_groups/_test_m2
> +expect_success $? "6 # simple MON directory rename"
> +
> +mv _test_c2/mon_groups/_test_m2 info
> +expect_fail $? "7 # move to info not allowed"
> +
> +mv _test_c2/mon_groups/_test_m2 _test_c2/mon_groups/mon_groups
> +expect_fail $? "8 # rename to mon_groups not allowed"
> +
> +mv mon_groups/_test_m1 _test_c1/mon_groups expect_fail $? "9 # cannot
> +move groups monitoring CPUs"
> +
> +mv mon_groups/_test_m1 mon_groups/_test_m2 expect_success $? "10 #
> +groups monitoring CPUs can be renamed"
> +
> +mv mon_groups/_test_m2/mon_data _test_c1/mon_groups expect_fail $? "11
> +# cannot move subdirectories of a mon_group"
> +
> +cleanup
> +
> +exit $rc
> --
> 2.40.0.423.gd6c402a77b-goog

Re: [PATCH v5 2/3] x86/resctrl: Implement rename op for mon groups

[Reinette Chatre]

Hi Peter,

On 3/30/2023 6:55 AM, Peter Newman wrote:
> To change the resources allocated to a large group of tasks, such as an
> application container, a container manager must write all of the tasks'
> IDs into the tasks file interface of the new control group.
> 
> If a container manager is additionally tracking containers' bandwidth
> usage by placing tasks from each into their own monitoring group, it

The above sentence seems to be missing something after the "for each".
It seems to still parse if "for each" is removed.

> must first move the tasks to the default monitoring group of the new
> control group before it can move the tasks into their new monitoring
> groups. This is undesirable because it makes bandwidth usage during the
> move unattributable to the correct tasks and resets monitoring event
> counters and cache usage information for the group.
> 
> Implement the rename operation only for resctrlfs monitor groups to
> enable users to move a monitoring group from one control group to
> another. This effects a change in resources allocated to all the tasks
> in the monitoring group while otherwise leaving the monitoring data
> intact.
> 
> Signed-off-by: Peter Newman <peternewman@google.com>
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 128 +++++++++++++++++++++++++
>  1 file changed, 128 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 51b869149e76..86de22d8e23a 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -3514,6 +3514,133 @@ static int rdtgroup_rmdir(struct kernfs_node *kn)
>  	return ret;
>  }
>  
> +/**
> + * mongrp_reparent() - replace parent CTRL_MON group of a MON group
> + * @rdtgrp:		the MON group whose parent should be replaced
> + * @new_prdtgrp:	replacement parent CTRL_MON group for @rdtgrp
> + * @cpus:		cpumask provided by the caller for use during this call
> + *
> + * Replaces the parent CTRL_MON group for a MON group, resulting in all member
> + * tasks' CLOSID immediately changing to that of the new parent group.
> + * Monitoring data for the group is unaffected by this operation.
> + */
> +static void mongrp_reparent(struct rdtgroup *rdtgrp,
> +			    struct rdtgroup *new_prdtgrp,
> +			    cpumask_var_t cpus)
> +{
> +	struct rdtgroup *prdtgrp = rdtgrp->mon.parent;
> +
> +	WARN_ON(rdtgrp->type != RDTMON_GROUP);
> +	WARN_ON(new_prdtgrp->type != RDTCTRL_GROUP);
> +
> +	/* Nothing to do when simply renaming a MON group. */
> +	if (prdtgrp == new_prdtgrp)
> +		return;
> +
> +	WARN_ON(list_empty(&prdtgrp->mon.crdtgrp_list));
> +	list_move_tail(&rdtgrp->mon.crdtgrp_list,
> +		       &new_prdtgrp->mon.crdtgrp_list);
> +
> +	rdtgrp->mon.parent = new_prdtgrp;
> +	rdtgrp->closid = new_prdtgrp->closid;
> +
> +	/* Propagate updated closid to all tasks in this group. */
> +	rdt_move_group_tasks(rdtgrp, rdtgrp, cpus);
> +
> +	update_closid_rmid(cpus, NULL);
> +}
> +
> +static int rdtgroup_rename(struct kernfs_node *kn,
> +			   struct kernfs_node *new_parent, const char *new_name)
> +{
> +	struct rdtgroup *new_prdtgrp;
> +	struct rdtgroup *rdtgrp;
> +	cpumask_var_t tmpmask;
> +	int ret;
> +
> +	rdtgrp = kernfs_to_rdtgroup(kn);
> +	new_prdtgrp = kernfs_to_rdtgroup(new_parent);
> +	if (!rdtgrp || !new_prdtgrp)
> +		return -ENOENT;
> +
> +	/* Release both kernfs active_refs before obtaining rdtgroup mutex. */
> +	rdtgroup_kn_get(rdtgrp, kn);
> +	rdtgroup_kn_get(new_prdtgrp, new_parent);
> +
> +	mutex_lock(&rdtgroup_mutex);
> +
> +	rdt_last_cmd_clear();
> +
> +	/*
> +	 * Don't allow kernfs_to_rdtgroup() to return a parent rdtgroup if
> +	 * either kernfs_node is a file.
> +	 */
> +	if (kernfs_type(kn) != KERNFS_DIR ||
> +	    kernfs_type(new_parent) != KERNFS_DIR) {
> +		rdt_last_cmd_puts("Source and destination must be group directories");

I do not think it is obvious what a "group directory" is. The source must be a
monitoring group and the destination must be the "mon_groups" directory. Maybe
the "group" term can just be dropped to read "Source and destination must be
directories" (which is exactly what is tested for).

> +		ret = -EPERM;
> +		goto out;
> +	}
> +
> +	if ((rdtgrp->flags & RDT_DELETED) || (new_prdtgrp->flags & RDT_DELETED)) {
> +		ret = -ENOENT;
> +		goto out;
> +	}
> +
> +	if (rdtgrp->type != RDTMON_GROUP || !kn->parent ||
> +	    !is_mon_groups(kn->parent, kn->name)) {
> +		rdt_last_cmd_puts("Source must be a MON group\n");
> +		ret = -EPERM;
> +		goto out;
> +	}
> +
> +	if (!is_mon_groups(new_parent, new_name)) {
> +		rdt_last_cmd_puts("Destination must be a mon_groups subdirectory\n");
> +		ret = -EPERM;
> +		goto out;
> +	}
> +

Thanks. I think using these terms ("MON" and "mon_groups") in the error messages
are useful since it gives the user something to search for in the documentation.

> +	/*
> +	 * 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
> +	 * the new parent, making the move illegal.
> +	 */
> +	if (!cpumask_empty(&rdtgrp->cpu_mask) &&
> +	    (rdtgrp->mon.parent != new_prdtgrp)) {

You can remove the extra parentheses so that this patch can get a clean slate
from "checkpatch.pl --strict" done as this work moves to the next level.

> +		rdt_last_cmd_puts("Cannot move a MON group that monitors CPUs\n");
> +		ret = -EPERM;
> +		goto out;
> +	}
> +
> +	/*
> +	 * Allocate the cpumask for use in mongrp_reparent() to avoid the
> +	 * possibility of failing to allocate it after kernfs_rename() has
> +	 * succeeded.
> +	 */
> +	if (!zalloc_cpumask_var(&tmpmask, GFP_KERNEL)) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
> +	/*
> +	 * Perform all input validation and allocations needed to ensure
> +	 * mongrp_reparent() will succeed before calling kernfs_rename(),
> +	 * otherwise it would be necessary to revert this call if
> +	 * mongrp_reparent() failed.
> +	 */
> +	ret = kernfs_rename(kn, new_parent, new_name);
> +	if (!ret)
> +		mongrp_reparent(rdtgrp, new_prdtgrp, tmpmask);
> +
> +	free_cpumask_var(tmpmask);
> +
> +out:
> +	mutex_unlock(&rdtgroup_mutex);
> +	rdtgroup_kn_put(rdtgrp, kn);
> +	rdtgroup_kn_put(new_prdtgrp, new_parent);
> +	return ret;
> +}
> +
>  static int rdtgroup_show_options(struct seq_file *seq, struct kernfs_root *kf)
>  {
>  	if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L3))
> @@ -3531,6 +3658,7 @@ static int rdtgroup_show_options(struct seq_file *seq, struct kernfs_root *kf)
>  static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
>  	.mkdir		= rdtgroup_mkdir,
>  	.rmdir		= rdtgroup_rmdir,
> +	.rename		= rdtgroup_rename,
>  	.show_options	= rdtgroup_show_options,
>  };
>  

Thank you very much.

Just the few minor comments. With those addressed:

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

Re: [PATCH v5 3/3] Documentation/x86: Documentation for MON group move feature

[Reinette Chatre]

Hi Peter,

On 3/30/2023 6:55 AM, Peter Newman wrote:
> Describe new support for moving MON groups to a new parent CTRL_MON
> group and its restrictions.
> 
> Signed-off-by: Peter Newman <peternewman@google.com>
> ---
>  Documentation/x86/resctrl.rst | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/Documentation/x86/resctrl.rst b/Documentation/x86/resctrl.rst
> index 387ccbcb558f..cb05d90111b4 100644
> --- a/Documentation/x86/resctrl.rst
> +++ b/Documentation/x86/resctrl.rst
> @@ -287,6 +287,13 @@ Removing a directory will move all tasks and cpus owned by the group it
>  represents to the parent. Removing one of the created CTRL_MON groups
>  will automatically remove all MON groups below it.
>  
> +Moving MON group directories to a new parent CTRL_MON group is supported
> +for the purpose of changing the resource allocations of a MON group
> +without impacting its monitoring data or assigned tasks. This operation
> +is not allowed for MON groups which monitor CPUs. No other move
> +operation is currently allowed other than simply renaming a CTRL_MON or
> +MON group.
> +
>  All groups contain the following files:
>  
>  "tasks":

Thank you.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

Re: [PATCH v5 2/3] x86/resctrl: Implement rename op for mon groups

[Peter Newman]

Hi Reinette,

On Tue, Apr 18, 2023 at 11:53 PM Reinette Chatre
<reinette.chatre@intel.com> wrote:
> On 3/30/2023 6:55 AM, Peter Newman wrote:
> > If a container manager is additionally tracking containers' bandwidth
> > usage by placing tasks from each into their own monitoring group, it
>
> The above sentence seems to be missing something after the "for each".
> It seems to still parse if "for each" is removed.

Did you mean "from each"? In any case, I'll further disambiguate to
this in my next update:

"If the container manager is using monitoring groups to separately
track the bandwidth of containers assigned to the same control group,
it must first move the container's tasks to the default monitoring
group of the new control group before it can move these tasks into the
container's replacement monitoring groups under the destination
control group."

> > +     /*
> > +      * Don't allow kernfs_to_rdtgroup() to return a parent rdtgroup if
> > +      * either kernfs_node is a file.
> > +      */
> > +     if (kernfs_type(kn) != KERNFS_DIR ||
> > +         kernfs_type(new_parent) != KERNFS_DIR) {
> > +             rdt_last_cmd_puts("Source and destination must be group directories");
>
> I do not think it is obvious what a "group directory" is. The source must be a
> monitoring group and the destination must be the "mon_groups" directory. Maybe
> the "group" term can just be dropped to read "Source and destination must be
> directories" (which is exactly what is tested for).

Sounds good.

>
> > +             ret = -EPERM;
> > +             goto out;
> > +     }
> > +
> > +     if ((rdtgrp->flags & RDT_DELETED) || (new_prdtgrp->flags & RDT_DELETED)) {
> > +             ret = -ENOENT;
> > +             goto out;
> > +     }
> > +
> > +     if (rdtgrp->type != RDTMON_GROUP || !kn->parent ||
> > +         !is_mon_groups(kn->parent, kn->name)) {
> > +             rdt_last_cmd_puts("Source must be a MON group\n");
> > +             ret = -EPERM;
> > +             goto out;
> > +     }
> > +
> > +     if (!is_mon_groups(new_parent, new_name)) {
> > +             rdt_last_cmd_puts("Destination must be a mon_groups subdirectory\n");
> > +             ret = -EPERM;
> > +             goto out;
> > +     }
> > +
>
> Thanks. I think using these terms ("MON" and "mon_groups") in the error messages
> are useful since it gives the user something to search for in the documentation.
>
> > +     /*
> > +      * 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
> > +      * the new parent, making the move illegal.
> > +      */
> > +     if (!cpumask_empty(&rdtgrp->cpu_mask) &&
> > +         (rdtgrp->mon.parent != new_prdtgrp)) {
>
> You can remove the extra parentheses so that this patch can get a clean slate
> from "checkpatch.pl --strict" done as this work moves to the next level.

Ok

>
> Thank you very much.
>
> Just the few minor comments. With those addressed:
>
> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Thanks again for your careful review. Also thank you for suggesting
this solution. It's a big improvement in maintainability over what
we've been using downstream.

-Peter

Re: [PATCH v5 2/3] x86/resctrl: Implement rename op for mon groups

[Reinette Chatre]

Hi Peter,

On 4/19/2023 2:38 AM, Peter Newman wrote:
> On Tue, Apr 18, 2023 at 11:53 PM Reinette Chatre
> <reinette.chatre@intel.com> wrote:
>> On 3/30/2023 6:55 AM, Peter Newman wrote:
>>> If a container manager is additionally tracking containers' bandwidth
>>> usage by placing tasks from each into their own monitoring group, it
>>
>> The above sentence seems to be missing something after the "for each".
>> It seems to still parse if "for each" is removed.
> 
> Did you mean "from each"?

I did, yes. Thanks for converting it to what I intended to write.

> In any case, I'll further disambiguate to
> this in my next update:
> 
> "If the container manager is using monitoring groups to separately
> track the bandwidth of containers assigned to the same control group,
> it must first move the container's tasks to the default monitoring
> group of the new control group before it can move these tasks into the
> container's replacement monitoring groups under the destination
> control group."

Looks good to me. Thank you.

Reinette