[systemd-devel] [PATCH] cgtop: Add cpu time opt and become stdout sensitive
Umut Tezduyar
umut at tezduyar.com
Tue Apr 2 09:55:41 PDT 2013
Hi,
I have resubmitted the patch with agreed changes.
My usecase is pretty simple actually. I would like to store process time of
services right after default.target and store the values in a database.
That involves running cgtop 1 time and parsing the output.
Thanks.
On Mon, Apr 1, 2013 at 2:23 PM, Zbigniew Jędrzejewski-Szmek <
zbyszek at in.waw.pl> wrote:
> On Mon, Apr 01, 2013 at 10:01:40AM +0200, Umut Tezduyar wrote:
> > 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?
> Because --cpu=percentange is the default. So the part after = (arg),
> 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.
> Oh, OK. Would have nice to have this in the message description.
> The headers are probably not useful, if anything the empty line
> between consecutive outputs is good enough.
>
> I'm not convinced about the loop, though. It would not be very easy
> to use in scripting, because one would have to split the output. Actually
> invoking cgtop multiple times. Do we actually have a concrete example
> how this is to be used?
>
> > > $ 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),
>
> Zbyszek
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/systemd-devel/attachments/20130402/9c5c75fe/attachment-0001.html>
More information about the systemd-devel
mailing list