From 726afd5d9a6fc2c048375a692d1659578e9ba968 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 14 Mar 2023 17:24:07 +0100 Subject: [PATCH 1/4] cgtop: split out the main loop into a separate function This way the initial setup is nicely separated from the main loop logic. --- src/cgtop/cgtop.c | 81 ++++++++++++++++++++++++----------------------- 1 file changed, 41 insertions(+), 40 deletions(-) diff --git a/src/cgtop/cgtop.c b/src/cgtop/cgtop.c index 5d82d656bc..65d9941f7a 100644 --- a/src/cgtop/cgtop.c +++ b/src/cgtop/cgtop.c @@ -96,6 +96,7 @@ static Group *group_free(Group *g) { return mfree(g); } +DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR(group_hash_ops, char, path_hash_func, path_compare, Group, group_free); static const char *maybe_format_timespan(char *buf, size_t l, usec_t t, usec_t accuracy) { if (arg_raw) { @@ -911,52 +912,19 @@ static const char* counting_what(void) { return "userspace processes (excl. kernel)"; } -DEFINE_PRIVATE_HASH_OPS_WITH_VALUE_DESTRUCTOR(group_hash_ops, char, path_hash_func, path_compare, Group, group_free); - -static int run(int argc, char *argv[]) { +static int loop(const char *root) { _cleanup_hashmap_free_ Hashmap *a = NULL, *b = NULL; unsigned iteration = 0; usec_t last_refresh = 0; - bool quit = false, immediate_refresh = false; - _cleanup_free_ char *root = NULL; - PidsCount possible_count; - CGroupMask mask; + bool immediate_refresh = false; int r; - log_setup(); - - r = parse_argv(argc, argv); - if (r <= 0) - return r; - - r = cg_mask_supported(&mask); - if (r < 0) - return log_error_errno(r, "Failed to determine supported controllers: %m"); - - /* honor user selection unless pids controller is unavailable */ - possible_count = (mask & CGROUP_MASK_PIDS) ? COUNT_PIDS : COUNT_ALL_PROCESSES; - arg_count = MIN(possible_count, arg_count); - - if (arg_recursive_unset && arg_count == COUNT_PIDS) - return log_error_errno(SYNTHETIC_ERRNO(EINVAL), - "Non-recursive counting is only supported when counting processes, not tasks. Use -P or -k."); - - r = show_cgroup_get_path_and_warn(arg_machine, arg_root, &root); - if (r < 0) - return log_error_errno(r, "Failed to get root control group path: %m"); - log_debug("CGroup path: %s", root); - a = hashmap_new(&group_hash_ops); b = hashmap_new(&group_hash_ops); if (!a || !b) return log_oom(); - signal(SIGWINCH, columns_lines_cache_reset); - - if (arg_iterations == UINT_MAX) - arg_iterations = on_tty() ? 0 : 1; - - while (!quit) { + for (;;) { usec_t t; char key; @@ -978,7 +946,7 @@ static int run(int argc, char *argv[]) { display(b); if (arg_iterations && iteration >= arg_iterations) - break; + return 0; if (!on_tty()) /* non-TTY: Empty newline as delimiter between polls */ fputs("\n", stdout); @@ -1009,8 +977,7 @@ static int run(int argc, char *argv[]) { break; case 'q': - quit = true; - break; + return 0; case 'p': arg_order = ORDER_PATH; @@ -1103,8 +1070,42 @@ static int run(int argc, char *argv[]) { break; } } +} - return 0; +static int run(int argc, char *argv[]) { + _cleanup_free_ char *root = NULL; + CGroupMask mask; + int r; + + log_setup(); + + r = parse_argv(argc, argv); + if (r <= 0) + return r; + + r = cg_mask_supported(&mask); + if (r < 0) + return log_error_errno(r, "Failed to determine supported controllers: %m"); + + /* honor user selection unless pids controller is unavailable */ + PidsCount possible_count = (mask & CGROUP_MASK_PIDS) ? COUNT_PIDS : COUNT_ALL_PROCESSES; + arg_count = MIN(possible_count, arg_count); + + if (arg_recursive_unset && arg_count == COUNT_PIDS) + return log_error_errno(SYNTHETIC_ERRNO(EINVAL), + "Non-recursive counting is only supported when counting processes, not tasks. Use -P or -k."); + + r = show_cgroup_get_path_and_warn(arg_machine, arg_root, &root); + if (r < 0) + return log_error_errno(r, "Failed to get root control group path: %m"); + log_debug("CGroup path: %s", root); + + signal(SIGWINCH, columns_lines_cache_reset); + + if (arg_iterations == UINT_MAX) + arg_iterations = on_tty() ? 0 : 1; + + return loop(root); } DEFINE_MAIN_FUNCTION(run); From 882c514d50dce5c05154d6881cf1e555c8523b32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 14 Mar 2023 17:24:43 +0100 Subject: [PATCH 2/4] cgtop: drop counterproductive bitfields The variable after the bitfields has 8 byte alignment, so we weren't saving any memory, but the code to serve the bitfields was more complicated. --- src/cgtop/cgtop.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cgtop/cgtop.c b/src/cgtop/cgtop.c index 65d9941f7a..bb4f7a5ef0 100644 --- a/src/cgtop/cgtop.c +++ b/src/cgtop/cgtop.c @@ -36,10 +36,10 @@ typedef struct Group { char *path; - bool n_tasks_valid:1; - bool cpu_valid:1; - bool memory_valid:1; - bool io_valid:1; + bool n_tasks_valid; + bool cpu_valid; + bool memory_valid; + bool io_valid; uint64_t n_tasks; From 5346bb499f77650679f9d44885ea5551b4bcce81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 14 Mar 2023 17:51:24 +0100 Subject: [PATCH 3/4] cgls: stop showing cgroup ids and xattrs by default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Those are rather specialized bits of information, useful mostly for debugging. I use cgls quite often but never had the need to use either of those… But they take up a significant amount of screen real estate, esp. when executed as root: -.slice ├─user.slice (#1683) │ → user.invocation_id: 74b0bd1258c5485eb969016384e0d06a │ → trusted.invocation_id: 74b0bd1258c5485eb969016384e0d06a │ └─user-1000.slice (#6488) │ → user.invocation_id: b0261a14fe74490d9a9d5266c52cceb6 │ → trusted.invocation_id: b0261a14fe74490d9a9d5266c52cceb6 │ ├─user@1000.service … (#6590) │ │ → user.invocation_id: 9e1fb54ad07940d8b92c33c81d169f11 │ │ → user.delegate: 1 │ │ → trusted.invocation_id: 9e1fb54ad07940d8b92c33c81d169f11 │ │ → trusted.delegate: 1 │ │ ├─session.slice (#6874) ... Let's not show them by default, so we can show more cgroups. (Also, on a terminal, we already highlight delegate units via underlining and an ellipsis, so 'user.delegate:1' is redundant.) --- man/systemd-cgls.xml | 4 ++-- src/cgls/cgls.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/man/systemd-cgls.xml b/man/systemd-cgls.xml index 3fa0a35770..4b5449e71c 100644 --- a/man/systemd-cgls.xml +++ b/man/systemd-cgls.xml @@ -109,14 +109,14 @@ Controls whether to include information about extended attributes of the listed - control groups in the output. Expects a boolean value, defaults to yes. + control groups in the output. Expects a boolean value. Defaults to no. Controls whether to include the numeric ID of the listed control groups in the - output. Expects a boolean value, defaults to yes. + output. Expects a boolean value. Defaults to no. diff --git a/src/cgls/cgls.c b/src/cgls/cgls.c index d6703634e1..a0c0d29116 100644 --- a/src/cgls/cgls.c +++ b/src/cgls/cgls.c @@ -24,7 +24,7 @@ #include "unit-name.h" static PagerFlags arg_pager_flags = 0; -static OutputFlags arg_output_flags = OUTPUT_CGROUP_XATTRS | OUTPUT_CGROUP_ID; +static OutputFlags arg_output_flags = 0; static enum { SHOW_UNIT_NONE, From ec38ec55ee138c80e56d448de110c47791040540 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Zbigniew=20J=C4=99drzejewski-Szmek?= Date: Tue, 14 Mar 2023 17:58:08 +0100 Subject: [PATCH 4/4] cgls: add -x and -c options -x is short for --xattrs=yes and -c is short for --cgroup-id=yes. --- man/systemd-cgls.xml | 7 +++++-- src/cgls/cgls.c | 36 ++++++++++++++++++++---------------- 2 files changed, 25 insertions(+), 18 deletions(-) diff --git a/man/systemd-cgls.xml b/man/systemd-cgls.xml index 4b5449e71c..794ff868f9 100644 --- a/man/systemd-cgls.xml +++ b/man/systemd-cgls.xml @@ -106,17 +106,20 @@ + Controls whether to include information about extended attributes of the listed - control groups in the output. Expects a boolean value. Defaults to no. + control groups in the output. With the long option, expects a boolean value. Defaults to no. + + Controls whether to include the numeric ID of the listed control groups in the - output. Expects a boolean value. Defaults to no. + output. With the long option, expects a boolean value. Defaults to no. diff --git a/src/cgls/cgls.c b/src/cgls/cgls.c index a0c0d29116..32780c606a 100644 --- a/src/cgls/cgls.c +++ b/src/cgls/cgls.c @@ -54,11 +54,11 @@ static int help(void) { " -a --all Show all groups, including empty\n" " -u --unit Show the subtrees of specified system units\n" " --user-unit Show the subtrees of specified user units\n" - " --xattr=BOOL Show cgroup extended attributes\n" - " --cgroup-id=BOOL Show cgroup ID\n" + " -x --xattr=BOOL Show cgroup extended attributes\n" + " -c --cgroup-id=BOOL Show cgroup ID\n" " -l --full Do not ellipsize output\n" " -k Include kernel threads in output\n" - " -M --machine= Show container\n" + " -M --machine=NAME Show container NAME\n" "\nSee the %s for details.\n", program_invocation_short_name, link); @@ -72,8 +72,6 @@ static int parse_argv(int argc, char *argv[]) { ARG_NO_PAGER = 0x100, ARG_VERSION, ARG_USER_UNIT, - ARG_XATTR, - ARG_CGROUP_ID, }; static const struct option options[] = { @@ -85,8 +83,8 @@ static int parse_argv(int argc, char *argv[]) { { "machine", required_argument, NULL, 'M' }, { "unit", optional_argument, NULL, 'u' }, { "user-unit", optional_argument, NULL, ARG_USER_UNIT }, - { "xattr", required_argument, NULL, ARG_XATTR }, - { "cgroup-id", required_argument, NULL, ARG_CGROUP_ID }, + { "xattr", required_argument, NULL, 'x' }, + { "cgroup-id", required_argument, NULL, 'c' }, {} }; @@ -95,7 +93,7 @@ static int parse_argv(int argc, char *argv[]) { assert(argc >= 1); assert(argv); - while ((c = getopt_long(argc, argv, "-hkalM:u::", options, NULL)) >= 0) + while ((c = getopt_long(argc, argv, "-hkalM:u::xc", options, NULL)) >= 0) switch (c) { @@ -143,18 +141,24 @@ static int parse_argv(int argc, char *argv[]) { arg_machine = optarg; break; - case ARG_XATTR: - r = parse_boolean(optarg); - if (r < 0) - return log_error_errno(r, "Failed to parse --xattr= value: %s", optarg); + case 'x': + if (optarg) { + r = parse_boolean(optarg); + if (r < 0) + return log_error_errno(r, "Failed to parse --xattr= value: %s", optarg); + } else + r = true; SET_FLAG(arg_output_flags, OUTPUT_CGROUP_XATTRS, r); break; - case ARG_CGROUP_ID: - r = parse_boolean(optarg); - if (r < 0) - return log_error_errno(r, "Failed to parse --cgroup-id= value: %s", optarg); + case 'c': + if (optarg) { + r = parse_boolean(optarg); + if (r < 0) + return log_error_errno(r, "Failed to parse --cgroup-id= value: %s", optarg); + } else + r = true; SET_FLAG(arg_output_flags, OUTPUT_CGROUP_ID, r); break;