BB: mpstat / iostat applets

[Denys Vlasenko]

On Monday 19 July 2010 21:40, Marek Polacek wrote:
>   Thank you for your review, it was very valuable.

Hey Marek, let's be less formal. :D
My "very valuable" input was also very late :(
Sorry.


> >          memcpy(buf, "Average:", 16);
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > After ":" you are copying garbage into buf[]
> >
> >          buf[15] = '\0';
> >          write_stats_core(2, current, buf, buf);
> > }
> What I've done was very wrong. I should have used memccpy() or maybe 
> strncpy() (I'm not a big fan of str*() functions though).

str*() functions are not bad at all. Really.
Some are even inlined by compiler.

> But I've  
> solved it differently: I've added const qualifiers to char * arguments 
> in functions write_irqcpu_stats() and write_stats_core(). And into 
> write_stats_avg() I've added line (memcpy is gone):
> 
>     static const char buf[] = "Average:";
> 
> it's static, so we have exactly one copy and it is read-only. _If_ I'm 
> not wrong, this way it'll go into .rodata.

Yes, it will go to .rodata

> If we would omit keyword  
> static, it would be allocated (generally) on stack or TLS. Where do we 
> want it to go to? I am very curious about this.

static const char buf[] = "Average:"

is ok.


> >                          /*
> >                           * Compute uptime in jiffies (1/HZ), multiply with
> >                           * number of CPUs.
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > Where is the" multiply" part? I don't see it.
> "Multiply" is maybe inappropriate word. In fact it means, that it is the 
> sum of individual CPU's uptimes.

Ok, where is the sum operator then? The comment says one thing and the code
does something a bit different. This is wrong. Comment seems to be unclear.


> I am sending version with (hopefully) corrected write_stats_core().

Let's see. It's getting better, but still a lot to do.
ll_sp_value() is such a "great" function name it makes
the reader want to strangle the author. :D
Variable names like "itv" are also bad.

So, with non-informative names like that, you end up with
code like this:

                        /* Compute interval again for current proc */
                        pc_itv = get_per_cpu_interval(scc, scp);
                        if (!pc_itv)
                                printf(" %9.2f\n", 0.0);
                        else
                                printf(" %9.2f\n",
                                        ll_s_value(sip->irq_nr, sic->irq_nr, itv));

See anything strange?
Why do we pass *itv* to ll_s_value? Shall we pass *pc_itv* instead?


        /* Compute time interval */
        itv = g_itv = jiffies_diff(G.uptime[prev], G.uptime[current]);

        /* Reduce interval to one CPU */
        if (G.cpu_nr > 1)
                itv = jiffies_diff(G.uptime0[prev], G.uptime0[current]);

What is the difference between G.uptime and G.uptime0? Can we give them
better names (ones which forestall questions like that)?


                                        q0 = per_cpu_stats[prev] + offset;
                                        if (strcmp(p0->irq_name, q0->irq_name) != 0
                                         && (j + 1 < total_irqs)
                                        ) {
                                                offset = j + 1;
                                        }
                                        q0 = per_cpu_stats[prev] + offset;

Why do we recompute q0 _unconditionally_?


Please find updated applet attached.

function                                             old     new   delta
write_stats_core                                     844    1290    +446
percent_value                                          -      88     +88
starts_with_cpu                                        -      31     +31
write_irqcpu_stats                                   597     588      -9
get_cpu_statistics                                   350     333     -17
mpstat_main                                         1620    1596     -24
write_stats                                          128      99     -29
get_irqs_from_interrupts                             576     519     -57
ll_sp_value                                          115       -    -115
ll_s_value                                           127       -    -127
print_stats_cpu_struct                               363       -    -363
------------------------------------------------------------------------------
(add/remove: 2/3 grow/shrink: 1/5 up/down: 565/-741)         Total: -176 bytes

Please look over the changes - I might break something...
Please rename [foo_]itv variables so that they are understandable.



> You've changed
> 
>      #define debug(fmt, ...)     /* Nothing */
> to
>      #define debug(fmt, ...) ((void)0)
> 
> -- is this just a cosmetics change, or there are situations, where would 
> it actually make a difference?

Well, in most real-world cases it does not matter, but here is a contrived example
with "comma expression" thing:

	return (debug("returning 1\n"), 1);

gcc will not like this:

	return (/* Nothing */, 1);



> BTW: "if ((buf[0] - 'c') | (buf[1] - 'p') | (buf[2] - 'u'))" -- this is 
> excellent :-))

Not sure about it being so great... I just hope it's the smallest code,
need to check it.

-- 
vda
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mpstat.c
Type: text/x-csrc
Size: 24056 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20100720/f157b683/attachment-0001.c>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: mpstat.c.diff
Type: text/x-diff
Size: 19439 bytes
Desc: not available
URL: <http://lists.busybox.net/pipermail/busybox/attachments/20100720/f157b683/attachment-0001.bin>