[systemd-devel] [PATCH] cgtop: Add cpu time opt and become stdout sensitive
Zbigniew Jędrzejewski-Szmek
zbyszek at in.waw.pl
Sun Mar 31 21:19:43 PDT 2013
On Sun, Mar 31, 2013 at 08:22:05PM +0200, Umut Tezduyar wrote:
> ---
> src/cgtop/cgtop.c | 108 ++++++++++++++++++++++++++++++++++++-----------------
> 1 files changed, 74 insertions(+), 34 deletions(-)
>
> diff --git a/src/cgtop/cgtop.c b/src/cgtop/cgtop.c
> index eebebf0..0ef4fcc 100644
> --- a/src/cgtop/cgtop.c
> +++ b/src/cgtop/cgtop.c
> @@ -69,6 +69,11 @@ static enum {
> ORDER_IO
> } arg_order = ORDER_CPU;
>
> +static enum {
> + CPU_PERCENT,
> + CPU_TIME,
> +} arg_cpu_type = CPU_PERCENT;
> +
> static void group_free(Group *g) {
> assert(g);
>
> @@ -372,16 +377,22 @@ static int group_compare(const void*a, const void *b) {
> return 1;
>
> if (arg_order == ORDER_CPU) {
> - if (x->cpu_valid && y->cpu_valid) {
> -
> - if (x->cpu_fraction > y->cpu_fraction)
> + if (arg_cpu_type == CPU_PERCENT) {
> + if (x->cpu_valid && y->cpu_valid) {
> + if (x->cpu_fraction > y->cpu_fraction)
> + return -1;
> + else if (x->cpu_fraction < y->cpu_fraction)
> + return 1;
> + } else if (x->cpu_valid)
> return -1;
> - else if (x->cpu_fraction < y->cpu_fraction)
> + else if (y->cpu_valid)
> return 1;
> - } else if (x->cpu_valid)
> - return -1;
> - else if (y->cpu_valid)
> - return 1;
> + } else {
> + if (x->cpu_usage > y->cpu_usage)
> + return -1;
> + else if (x->cpu_usage < y->cpu_usage)
> + return 1;
> + }
> }
>
> if (arg_order == ORDER_TASKS) {
> @@ -428,13 +439,16 @@ static int display(Hashmap *a) {
> Iterator i;
> Group *g;
> Group **array;
> - unsigned rows, path_columns, n = 0, j;
> + signed path_columns;
> + unsigned rows, n = 0, j, maxtcpu = 0, maxtpath = 0;
> + char cpu_title[21];
>
> assert(a);
>
> /* Set cursor to top left corner and clear screen */
> - fputs("\033[H"
> - "\033[2J", stdout);
> + if (on_tty())
> + fputs("\033[H"
> + "\033[2J", stdout);
>
> array = alloca(sizeof(Group*) * hashmap_size(a));
>
> @@ -444,33 +458,50 @@ static int display(Hashmap *a) {
>
> qsort(array, n, sizeof(Group*), group_compare);
>
> + /* Find longest names in one run */
> + for (j = 0; j < n; j++) {
> + snprintf(cpu_title, sizeof(cpu_title), "%llu", array[j]->cpu_usage);
> + if (strlen(cpu_title) > maxtcpu)
> + maxtcpu = strlen(cpu_title);
> + if (strlen(array[j]->path) > maxtpath)
> + maxtpath = strlen(array[j]->path);
The compiler *might* be smart enough, but let's not depend on that.
strlen() result should be cached. Maybe just use MAX?
> + }
> +
> + if (arg_cpu_type == CPU_PERCENT)
> + snprintf(cpu_title, sizeof(cpu_title), "%6s", "%CPU");
> + else
> + snprintf(cpu_title, sizeof(cpu_title), "%*s", maxtcpu, "CPU Time");
> +
> rows = lines();
> if (rows <= 10)
> rows = 10;
>
> - path_columns = columns() - 42;
> - if (path_columns < 10)
> - path_columns = 10;
> -
> - printf("%s%-*s%s %s%7s%s %s%6s%s %s%8s%s %s%8s%s %s%8s%s\n\n",
> - arg_order == ORDER_PATH ? ANSI_HIGHLIGHT_ON : "", path_columns, "Path",
> - arg_order == ORDER_PATH ? ANSI_HIGHLIGHT_OFF : "",
> - arg_order == ORDER_TASKS ? ANSI_HIGHLIGHT_ON : "", "Tasks",
> - arg_order == ORDER_TASKS ? ANSI_HIGHLIGHT_OFF : "",
> - arg_order == ORDER_CPU ? ANSI_HIGHLIGHT_ON : "", "%CPU",
> - arg_order == ORDER_CPU ? ANSI_HIGHLIGHT_OFF : "",
> - arg_order == ORDER_MEMORY ? ANSI_HIGHLIGHT_ON : "", "Memory",
> - arg_order == ORDER_MEMORY ? ANSI_HIGHLIGHT_OFF : "",
> - arg_order == ORDER_IO ? ANSI_HIGHLIGHT_ON : "", "Input/s",
> - arg_order == ORDER_IO ? ANSI_HIGHLIGHT_OFF : "",
> - arg_order == ORDER_IO ? ANSI_HIGHLIGHT_ON : "", "Output/s",
> - arg_order == ORDER_IO ? ANSI_HIGHLIGHT_OFF : "");
> + if (on_tty()) {
> + path_columns = columns() - 36 - strlen(cpu_title);
> + if (path_columns < 10)
> + path_columns = 10;
> + } else
> + path_columns = maxtpath;
> +
> + printf("%s%-*s%s %s%7s%s %s%s%s %s%8s%s %s%8s%s %s%8s%s\n\n",
> + on_tty() && arg_order == ORDER_PATH ? ANSI_HIGHLIGHT_ON : "", path_columns, "Path",
> + on_tty() && arg_order == ORDER_PATH ? ANSI_HIGHLIGHT_OFF : "",
> + on_tty() && arg_order == ORDER_TASKS ? ANSI_HIGHLIGHT_ON : "", "Tasks",
> + on_tty() && arg_order == ORDER_TASKS ? ANSI_HIGHLIGHT_OFF : "",
> + on_tty() && arg_order == ORDER_CPU ? ANSI_HIGHLIGHT_ON : "", cpu_title,
> + on_tty() && arg_order == ORDER_CPU ? ANSI_HIGHLIGHT_OFF : "",
> + on_tty() && arg_order == ORDER_MEMORY ? ANSI_HIGHLIGHT_ON : "", "Memory",
> + on_tty() && arg_order == ORDER_MEMORY ? ANSI_HIGHLIGHT_OFF : "",
> + on_tty() && arg_order == ORDER_IO ? ANSI_HIGHLIGHT_ON : "", "Input/s",
> + on_tty() && arg_order == ORDER_IO ? ANSI_HIGHLIGHT_OFF : "",
> + on_tty() && arg_order == ORDER_IO ? ANSI_HIGHLIGHT_ON : "", "Output/s",
> + on_tty() && arg_order == ORDER_IO ? ANSI_HIGHLIGHT_OFF : "");
What about defining
const char *on = on_tty() ? ANSI_HIGHLIGHT_ON : "", *off = on_tty() ? ANSI_HIGHLIGHT_OFF : "",
to make this easier to read?
>
> for (j = 0; j < n; j++) {
> char *p;
> char m[FORMAT_BYTES_MAX];
>
> - if (j + 5 > rows)
> + if (on_tty() && j + 5 > rows)
> break;
>
> g = array[j];
> @@ -484,10 +515,13 @@ static int display(Hashmap *a) {
> else
> fputs(" -", stdout);
>
> - if (g->cpu_valid)
> - printf(" %6.1f", g->cpu_fraction*100);
> + if (arg_cpu_type == CPU_PERCENT)
> + if (g->cpu_valid)
> + printf(" %6.1f", g->cpu_fraction*100);
> + else
> + fputs(" -", stdout);
> else
> - fputs(" -", stdout);
> + printf(" %*llu", maxtcpu, g->cpu_usage);
>
> if (g->memory_valid)
> printf(" %8s", format_bytes(m, sizeof(m), g->memory));
> @@ -519,6 +553,7 @@ static void help(void) {
> " -c Order by CPU load\n"
> " -m Order by memory load\n"
> " -i Order by IO load\n"
> + " --cpu=TYPE time or percentage (default) as CPU type\n"
> " -d --delay=DELAY Specify delay\n"
> " -n --iterations=N Run for N iterations before exiting\n"
> " -b --batch Run in batch mode, accepting no input\n"
> @@ -535,6 +570,7 @@ static int parse_argv(int argc, char *argv[]) {
> enum {
> ARG_VERSION = 0x100,
> ARG_DEPTH,
> + ARG_CPU_TYPE
> };
>
> static const struct option options[] = {
> @@ -544,6 +580,7 @@ static int parse_argv(int argc, char *argv[]) {
> { "iterations", required_argument, NULL, 'n' },
> { "batch", no_argument, NULL, 'b' },
> { "depth", required_argument, NULL, ARG_DEPTH },
> + { "cpu", required_argument, NULL, ARG_CPU_TYPE},
I think this should be optional_argument.
> { NULL, 0, NULL, 0 }
> };
>
> @@ -565,6 +602,11 @@ static int parse_argv(int argc, char *argv[]) {
> version();
> return 0;
>
> + case ARG_CPU_TYPE:
> + if (strcmp(optarg, "time") == 0)
> + arg_cpu_type = CPU_TIME;
> + break;
This should check for errors, especially that we want cgtop to be
scriptable. So an unsupported type should result in an error.
> case ARG_DEPTH:
> r = safe_atou(optarg, &arg_depth);
> if (r < 0) {
> @@ -777,8 +819,6 @@ int main(int argc, char *argv[]) {
> }
> }
>
> - log_info("Exiting.");
> -
> r = 0;
I thought that it was supposed to break off after one loop, if !on_tty().
It seems to loop. Also, for me is still prints too much headers:
$ systemd-cgtop|cat
Path Tasks %CPU Memory Input/s Output/s
/ 395 18.8 2.7G - -
...
I guess that you might be developing on 32 bits. Please put something like
this on top:
diff --git src/cgtop/cgtop.c src/cgtop/cgtop.c
index 0ef4fcc..9f7ffd6 100644
--- src/cgtop/cgtop.c
+++ src/cgtop/cgtop.c
@@ -19,9 +19,11 @@
along with systemd; If not, see <http://www.gnu.org/licenses/>.
***/
+#define __STDC_FORMAT_MACROS
#include <errno.h>
#include <string.h>
#include <stdlib.h>
+#include <stdint.h>
#include <unistd.h>
#include <alloca.h>
#include <getopt.h>
@@ -460,7 +462,7 @@ static int display(Hashmap *a) {
/* Find longest names in one run */
for (j = 0; j < n; j++) {
- snprintf(cpu_title, sizeof(cpu_title), "%llu", array[j]->cpu_usage);
+ snprintf(cpu_title, sizeof(cpu_title), "%"PRIu64, array[j]->cpu_usage);
if (strlen(cpu_title) > maxtcpu)
maxtcpu = strlen(cpu_title);
if (strlen(array[j]->path) > maxtpath)
@@ -521,7 +523,7 @@ static int display(Hashmap *a) {
else
fputs(" -", stdout);
else
- printf(" %*llu", maxtcpu, g->cpu_usage);
+ printf(" %*"PRIu64, maxtcpu, g->cpu_usage);
if (g->memory_valid)
printf(" %8s", format_bytes(m, sizeof(m), g->memory));
HTH,
Zbyszek
More information about the systemd-devel
mailing list