[systemd-devel] [PATCH] cgtop: Add cpu time opt and become stdout sensitive
Umut Tezduyar
umut at tezduyar.com
Mon Apr 1 01:01:40 PDT 2013
Hi Zbigniew,
Thank you very much for reviewing. Please find my comments below.
On Mon, Apr 1, 2013 at 6:19 AM, Zbigniew Jędrzejewski-Szmek <
zbyszek at in.waw.pl> wrote:
> 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?
>
Umut: Sounds reasonable.
>
> > + }
> > +
> > + 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?
>
Umut: I agree.
>
> >
> > 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.
>
Umut: Didn't quite understand this. Please explain why you think it should
be optional?
>
> > { 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.
>
Umut: I will add the type limitation.
>
> > 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:
>
Umut: I thought not to put a 1 loop limitation when not on tty. Someone
might want to record data over a period of time. Thats why I kept the
limit. I have also noticed the repeating column headers but the reason why
I kept them was, to provide a splitter between consecutive data output. If
the consensus is only printing them once, I will fix it.
>
> $ 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:
>
Umut: I am indeed on 32 bit. I will use the format modifiers from stdint.
>
> 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20130401/17b3315e/attachment-0001.html>
More information about the systemd-devel
mailing list