[prev in list] [next in list] [prev in thread] [next in thread] 

List:       busybox
Subject:    Re: [PATCH] pmap: new applet
From:       Denys Vlasenko <vda.linux () googlemail ! com>
Date:       2010-08-28 21:21:31
Message-ID: 201008282321.31085.vda.linux () googlemail ! com
[Download RAW message or body]

On Tuesday 24 August 2010 09:47, Denys Vlasenko wrote:
> On Tuesday 17 August 2010 12:05, Alexander Shishkin wrote:
> > On Sun, Jul 31, 2010 at 01:23:54 +0300, Alexander Shishkin wrote:
> > > pmap is a tool used to look at processes' memory maps, normally found
> > > in procps package. It provides more readable and easily sortable output
> > > (one line per mapping) from  maps/smaps files in /proc/PID/.  This would
> > > help in debugging memory usage issues, especially on devices where lots
> > > of typing is not a viable option.
> > 
> > Ping.
> 
> +struct smaprec {
> +       unsigned long mapped_rw;
> +       unsigned long mapped_ro;
> +       unsigned long shared_clean;
> +       unsigned long shared_dirty;
> +       unsigned long private_clean;
> +       unsigned long private_dirty;
> +       unsigned long stack;
> +       unsigned long pss, swap;
> +       unsigned long size;
> +       unsigned long start;
> +       char mode[5];
> +       char *name;
> +};
> 
> Can you prefix new field names with smap_ ?
> Grepping fails for field named simply "name" - too many
> such names already.
> 
> 
>                 while (*str++ != ' ')
> -                       continue;
> -               /* we found a space char, str points after it */
> +                       ;
> 
> Why this change?
> 
> 
> +#ifdef ENABLE_FEATURE_TOPMEM
> 
> This should be #if, not #ifdef. Also, you need to add "|| ENABLE_PMAP"
> 
> 
> +       RESERVE_CONFIG_BUFFER(filename, FILENAME_MAX);
> +       RESERVE_CONFIG_BUFFER(buf, PROCPS_BUFSIZE);
> +
> +       snprintf(filename, FILENAME_MAX, "/proc/%d/smaps", pid);
> 
> It's better to use smaller buffer for filename.
> 
> 
> +#define SCAN(X) \
> +               if (strncasecmp(tp, #X ":", sizeof #X) == 0) {       \
> +                       tp = skip_whitespace(tp + sizeof(#X));       \
> +                       total->X += currec.X = fast_strtoul_10(&tp); \
> +                       continue;                                    \
> +               }
> +
> +               SCAN(pss);
> +               SCAN(swap);
> 
> strncasecmp is slower than strncmp. You trade a small convenience
> in calling the macro for accumulated slowness in top for _every_
> _line_ of smaps!
> 
> 
> +                       if (p == tp)
> +                               currec.name = xstrdup("  [ anon ]");
> +                       else {
> +                               *p = '\0';
> +                               currec.name = xstrdup(tp);
> +                       }
> 
> This allocating/freeing of name field will slow down top,
> without any reason to do so.
> 
> 
> +       opt = getopt32(argv, "xq");
> +
> +       for (argv += optind; argv && *argv; argv++) {
> 
> argv is never NULL, no need to check for that.
> 
> 
> +       RESERVE_CONFIG_BUFFER(buf, BUFSIZ);
> a
> +       read_cmdline(buf, BUFSIZ, pid, "no such process");
> +       printf("%d:   %s\n", pid, buf);
> 
> standard pmap does not show the whole command line, only process name.
> The output format is different too.
> 
> 
> +       if (!(opt & OPT_Q) && (opt & OPT_X))
> +               printf("--------" DASHES "  ------  ------  ------  ------\n"
> +                      "total" TABS " %7lu %7lu %7lu %7lu\n",
> +                      total.size, total.pss, total.private_dirty, total.swap);
> +       else if (!(opt & OPT_Q))
> +               printf(" total" TABS "%7luK\n", total.size);
> 
> !(opt & OPT_Q) is tested twice.
> 
> 
> And now, run testing. I ran top, pressed 's' and then pressed
> and kept space down. Top leaks memory:
> 
> Mem total:2054936 anon:191364 map:50604 free:583520
>  slab:118472 buf:133744 cache:1006420 dirty:20 write:0
> Swap total:131068 free:131068
>   PID   VSZ*VSZRW   RSS (SHR) DIRTY (SHR) STACK COMMAND
>  3616  202m  165m 67904  2824 47876    64   132 /usr/app/firefox-3.6/firefox-bin
> 24722 45092 44236 44292     0 44116     0   132 ./busybox top      <=======================
>  3075  104m 36972 56988 11944 38176     0  2368 kmail -caption KMail -icon kmail -miniicon kmail
>  2053 17080 12368 14784  1460 11232     0   132 X :0
> ...
> 
> I'm not impressed.
> Try attached.

Applied. Still have ~200 byte growth even with no pmap enabled...

-- 
vda
_______________________________________________
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox
[prev in list] [next in list] [prev in thread] [next in thread]