Received-SPF: pass (google.com: domain of linux-kernel+bounces-134088-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <a45c2ece-acb4-4cff-9d53-f5c007c9b905@collabora.com>
Date: Sun, 7 Apr 2024 02:37:44 +0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Cc: Muhammad Usama Anjum <usama.anjum@collabora.com>,
 Tejun Heo <tj@kernel.org>, Zefan Li <lizefan.x@bytedance.com>,
 Johannes Weiner <hannes@cmpxchg.org>, Jonathan Corbet <corbet@lwn.net>,
 Shuah Khan <shuah@kernel.org>
Subject: Re: [RFC PATCH v3 6/9] selftests: cgroup: Add basic tests for pids
 controller
To: =?UTF-8?Q?Michal_Koutn=C3=BD?= <mkoutny@suse.com>,
 cgroups@vger.kernel.org, linux-doc@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org
References: <20240405170548.15234-1-mkoutny@suse.com>
 <20240405170548.15234-7-mkoutny@suse.com>
Content-Language: en-US
From: Muhammad Usama Anjum <usama.anjum@collabora.com>
In-Reply-To: <20240405170548.15234-7-mkoutny@suse.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On 4/5/24 10:05 PM, Michal Koutný wrote:
> This commit adds (and wires in) new test program for checking basic pids
> controller functionality -- restricting tasks in a cgroup and correct
> event counting.
> 
> Signed-off-by: Michal Koutný <mkoutny@suse.com>
> ---
>  tools/testing/selftests/cgroup/Makefile    |   2 +
>  tools/testing/selftests/cgroup/test_pids.c | 187 +++++++++++++++++++++
Please create/add test_pid to .gitignore file.

>  2 files changed, 189 insertions(+)
>  create mode 100644 tools/testing/selftests/cgroup/test_pids.c
> 
> diff --git a/tools/testing/selftests/cgroup/Makefile b/tools/testing/selftests/cgroup/Makefile
> index f3e1ef69e88d..f5f0886a2c4a 100644
> --- a/tools/testing/selftests/cgroup/Makefile
> +++ b/tools/testing/selftests/cgroup/Makefile
> @@ -15,6 +15,7 @@ TEST_GEN_PROGS += test_hugetlb_memcg
>  TEST_GEN_PROGS += test_kill
>  TEST_GEN_PROGS += test_kmem
>  TEST_GEN_PROGS += test_memcontrol
> +TEST_GEN_PROGS += test_pids
>  TEST_GEN_PROGS += test_zswap
>  
>  LOCAL_HDRS += $(selfdir)/clone3/clone3_selftests.h $(selfdir)/pidfd/pidfd.h
> @@ -29,4 +30,5 @@ $(OUTPUT)/test_hugetlb_memcg: cgroup_util.c
>  $(OUTPUT)/test_kill: cgroup_util.c
>  $(OUTPUT)/test_kmem: cgroup_util.c
>  $(OUTPUT)/test_memcontrol: cgroup_util.c
> +$(OUTPUT)/test_pids: cgroup_util.c
>  $(OUTPUT)/test_zswap: cgroup_util.c
> diff --git a/tools/testing/selftests/cgroup/test_pids.c b/tools/testing/selftests/cgroup/test_pids.c
> new file mode 100644
> index 000000000000..c1c3a3965624
> --- /dev/null
> +++ b/tools/testing/selftests/cgroup/test_pids.c
> @@ -0,0 +1,187 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#define _GNU_SOURCE
> +
> +#include <errno.h>
> +#include <linux/limits.h>
> +#include <signal.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include "../kselftest.h"
> +#include "cgroup_util.h"
> +
> +static int run_success(const char *cgroup, void *arg)
> +{
> +	return 0;
> +}
> +
> +static int run_pause(const char *cgroup, void *arg)
> +{
> +	return pause();
> +}
> +
> +/*
> + * This test checks that pids.max prevents forking new children above the
> + * specified limit in the cgroup.
> + */
> +static int test_pids_max(const char *root)
> +{
> +	int ret = KSFT_FAIL;
> +	char *cg_pids;
> +	int pid;
> +
> +
Please remove extra line.

> +	cg_pids = cg_name(root, "pids_test");
> +	if (!cg_pids)
> +		goto cleanup;
> +
> +	if (cg_create(cg_pids))
> +		goto cleanup;
> +
> +	if (cg_read_strcmp(cg_pids, "pids.max", "max\n"))
> +		goto cleanup;
> +
> +	if (cg_write(cg_pids, "pids.max", "2"))
> +		goto cleanup;
> +
> +	if (cg_enter_current(cg_pids))
> +		goto cleanup;
> +
> +	pid = cg_run_nowait(cg_pids, run_pause, NULL);
> +	if (pid < 0)
> +		goto cleanup;
> +
> +	if (cg_run_nowait(cg_pids, run_success, NULL) != -1 || errno != EAGAIN)
> +		goto cleanup;
> +
> +	if (kill(pid, SIGINT))
> +		goto cleanup;
> +
> +	ret = KSFT_PASS;
> +
> +cleanup:
> +	cg_enter_current(root);
> +	cg_destroy(cg_pids);
> +	free(cg_pids);
> +
> +	return ret;
> +}
> +
> +/*
> + * This test checks that pids.max prevents forking new children above the
> + * specified limit in the cgroup.
> + */
> +static int test_pids_events(const char *root)
> +{
> +	int ret = KSFT_FAIL;
> +	char *cg_parent = NULL, *cg_child = NULL;
> +	int pid;
> +
> +
> +	cg_parent = cg_name(root, "pids_parent");
> +	cg_child = cg_name(cg_parent, "pids_child");
> +	if (!cg_parent || !cg_child)
> +		goto cleanup;
> +
> +	if (cg_create(cg_parent))
> +		goto cleanup;
> +	if (cg_write(cg_parent, "cgroup.subtree_control", "+pids"))
> +		goto cleanup;
> +	if (cg_create(cg_child))
> +		goto cleanup;
> +
> +	if (cg_write(cg_parent, "pids.max", "2"))
> +		goto cleanup;
> +
> +	if (cg_read_strcmp(cg_child, "pids.max", "max\n"))
> +		goto cleanup;
> +
> +	if (cg_enter_current(cg_child))
> +		goto cleanup;
> +
> +	pid = cg_run_nowait(cg_child, run_pause, NULL);
> +	if (pid < 0)
> +		goto cleanup;
> +
> +	if (cg_run_nowait(cg_child, run_success, NULL) != -1 || errno != EAGAIN)
> +		goto cleanup;
> +
> +	if (kill(pid, SIGINT))
> +		goto cleanup;
> +
> +
Remove extra line.

> +	if (cg_read_key_long(cg_child, "pids.events", "max ") != 0)
> +		goto cleanup;
> +	if (cg_read_key_long(cg_child, "pids.events", "max.imposed ") != 1)
> +		goto cleanup;
> +
> +	if (cg_read_key_long(cg_parent, "pids.events", "max ") != 1)
> +		goto cleanup;
> +	if (cg_read_key_long(cg_parent, "pids.events", "max.imposed ") != 1)
> +		goto cleanup;
> +
> +
> +	ret = KSFT_PASS;
> +
> +cleanup:
> +	cg_enter_current(root);
> +	if (cg_child)
> +		cg_destroy(cg_child);
> +	if (cg_parent)
> +		cg_destroy(cg_parent);
> +	free(cg_child);
> +	free(cg_parent);
> +
> +	return ret;
> +}
> +
> +
> +
> +#define T(x) { x, #x }
> +struct pids_test {
> +	int (*fn)(const char *root);
> +	const char *name;
> +} tests[] = {
> +	T(test_pids_max),
> +	T(test_pids_events),
> +};
> +#undef T
> +
> +int main(int argc, char **argv)
> +{
> +	char root[PATH_MAX];
> +	int i, ret = EXIT_SUCCESS;
The
	ksft_print_header();
	ksft_set_plan(total_number_of_tests);
are missing. Please use all of the ksft APIs to make the test TAP compliant.

> +
> +	if (cg_find_unified_root(root, sizeof(root)))
> +		ksft_exit_skip("cgroup v2 isn't mounted\n");
> +
> +	/*
> +	 * Check that pids controller is available:
> +	 * pids is listed in cgroup.controllers
> +	 */
> +	if (cg_read_strstr(root, "cgroup.controllers", "pids"))
> +		ksft_exit_skip("pids controller isn't available\n");
> +
> +	if (cg_read_strstr(root, "cgroup.subtree_control", "pids"))
> +		if (cg_write(root, "cgroup.subtree_control", "+pids"))
> +			ksft_exit_skip("Failed to set pids controller\n");
> +
> +	for (i = 0; i < ARRAY_SIZE(tests); i++) {
> +		switch (tests[i].fn(root)) {
> +		case KSFT_PASS:
> +			ksft_test_result_pass("%s\n", tests[i].name);
> +			break;
> +		case KSFT_SKIP:
> +			ksft_test_result_skip("%s\n", tests[i].name);
> +			break;
> +		default:
> +			ret = EXIT_FAILURE;
> +			ksft_test_result_fail("%s\n", tests[i].name);
> +			break;
Use ksft_test_result_report() instead of swith-case here.

> +		}
> +	}
> +
> +	return ret;
> +}

-- 
BR,
Muhammad Usama Anjum