svn commit: r326073 - head/usr.bin/systat

[Bruce Evans]

On Wed, 22 Nov 2017, Konstantin Belousov wrote:

> On Wed, Nov 22, 2017 at 08:59:21AM +1100, Bruce Evans wrote:
>> On Tue, 21 Nov 2017, Konstantin Belousov wrote:
>>
>>> Log:
>>>  systat: use and correctly display 64bit counters.
>> ...
>> Using unsigned types gives sign extension bugs as usual.  In old versions
>> ...
>>> @@ -491,15 +495,15 @@ showkre(void)
>>> 	putfloat(100.0 * s.v_kmem_map_size / kmem_size,
>>> 	   STATROW + 1, STATCOL + 22, 2, 0, 1);
>>>
>>> -	putint(pgtokb(total.t_arm), MEMROW + 2, MEMCOL + 4, 7);
>>> -	putint(pgtokb(total.t_armshr), MEMROW + 2, MEMCOL + 12, 7);
>>> -	putint(pgtokb(total.t_avm), MEMROW + 2, MEMCOL + 20, 8);
>>> -	putint(pgtokb(total.t_avmshr), MEMROW + 2, MEMCOL + 29, 8);
>>> -	putint(pgtokb(total.t_rm), MEMROW + 3, MEMCOL + 4, 7);
>>> -	putint(pgtokb(total.t_rmshr), MEMROW + 3, MEMCOL + 12, 7);
>>> -	putint(pgtokb(total.t_vm), MEMROW + 3, MEMCOL + 20, 8);
>>> -	putint(pgtokb(total.t_vmshr), MEMROW + 3, MEMCOL + 29, 8);
>>> -	putint(pgtokb(total.t_free), MEMROW + 2, MEMCOL + 38, 7);
>>> +	putuint64(pgtokb(total.t_arm), MEMROW + 2, MEMCOL + 4, 7);
>>> +	putuint64(pgtokb(total.t_armshr), MEMROW + 2, MEMCOL + 12, 7);
>>> +	putuint64(pgtokb(total.t_avm), MEMROW + 2, MEMCOL + 20, 8);
>>> +	putuint64(pgtokb(total.t_avmshr), MEMROW + 2, MEMCOL + 29, 8);
>>> +	putuint64(pgtokb(total.t_rm), MEMROW + 3, MEMCOL + 4, 7);
>>> +	putuint64(pgtokb(total.t_rmshr), MEMROW + 3, MEMCOL + 12, 7);
>>> +	putuint64(pgtokb(total.t_vm), MEMROW + 3, MEMCOL + 20, 8);
>>> +	putuint64(pgtokb(total.t_vmshr), MEMROW + 3, MEMCOL + 29, 8);
>>> +	putuint64(pgtokb(total.t_free), MEMROW + 2, MEMCOL + 38, 7);
>>
>> I see that a very recent expansion from int32_t to uint64_t didn't work
>> here.
> Can you explain, please ?

Just that systat -v didn't automatically benefit from the expansion (since
it is not very well written, with hard-coded types.  Hard-coded ints were
at least simple.  Now it has lots of hard-coded uint64_t's, partly to
work around dehumanize_number() being broken as designed (this function
doesn't support numbers, but only supports uint64_t.  It would be a bit
much for it to support surreal numbers, but it doesn't even support signed
integers)).

I checked all programs in /usr/src that use t_avm (t_vm is too hard to grep
for).  (There aren't many.  Statistics utilities in ports probably have more
variations and more bugs altogether.)  None automatically benefited from the
change, and all still have sign type errors:
- systat/vmstat.c as above
- vmstat/vmstat.c:
   This is much uglier, since it has flags to control the dehumanized printing
   and uses libxo so it us ugly even without all the lexical style bugs added
   with libxo.

   vmstat does most things worse:
   - with hflag (this gives the dehumanized printing), it tries to print
     memory sizes in bytes.  Even humans would have problems reading the
     large decimal numbers resulting from sizes in bytes, but limited field
     widths usually result in dehumanization to K or M (rarely G or T).
     systat -v tries to print sizes in K (without the usually-redundant K
     suffix) more consistently, but this has problems when the values in K
     don't fit: vmstat does one thing better:
       if the size is too large to express in K, say just 4M, then vmstat
       will print 4M starting with the size in bytes, but vmstat starts with
       the size in K (4096) and will now print this as 4K, while the old
       version printed it as 4k.  4K of a size in K is almost as confusing
       as 4k of a size in K.

     This is one of the few places where the expansion worked automatically,
     because prthuman() already uses uint64_t (misspelled u_int64_t).  The
     casts of sum.v_page_size to u_int64_t is now redundant as well as
     misspelled.  It was needed even with int32_t fields because of the
     bad units -- 2G in bytes is quite small.  This casts also gave sign
     extension/overflow.  Now it has no effect.  Such casts are safer, but
     uint64_t hard-coded in so many places the code might as well be
     simplified by hard-coding here too.

   - the !hflag case is broken by blind expansion to use more 4 more columns
     than are available even when all the fields fit (large numbers
     mess this up more).  Also, the headers are broken with or without
     hflag (the first header line no longer lines up with the second
     header line.

   - without hflag, the code is similar to systat, but much more broken.  It
     has the same fairly hard to reach overflows in pgtokb().  Now it has
     undefined behaviour in all cases in xo_emit(), since the arg type
     changed to uint64_t but the format is still %7d.  Previously this
     only had undefined behaviour in overflowing cases (t_avm was int32_t,
     but v_page_size is u_int, so pgtokb() had type u_int; in overflow
     cases its value exceeds INT_MAX so the behaviour is undefined even
     though the overflow didn't give undefined behaviour.

     xo_emit() is not declared as __printflike(), so the undefined behaviour
     cannot be detected by the compiler.  In systat, these bugs were reduced
     by forcing pgtokb() back to int by passing them to putnum().  The
     behaviour was implementation-defined (truncate).

release/picobsd/tinyware/vm/vm.c:
   This is a tiny program, though not so tiny with 64-bit integers.  It
   does a small part of what vmstat does, and has the same undefined behaviour
   from using %7d format to print pgtok() which now has type uint64_t.
   Since it doesn't have libxo, printf format checking might detect the
   error.  But the Makefile doesn't set WARNS.

sysctl/sysctl.c:
   sysctl(8) has bogus support for prettyprinting struct vmtotal (sysctl
   shouldn't have any prettyprinting, especially not for structs that have
   specialized programs to print them and much more).  This uses intmax_t
   for all calculations and printing except for the int16_t fields, so it
   automatically benefited from the expansion.  However since it uses a
   correct type (signed, and not restricted to 64 bits), it now has minor
   type errors -- it dowcasts the uint64_t to intmax_t wheen the latter is
   64 bits.  Its Makefile uses a fairly high WARNS, so if its type errors
   were fatal then printf format checking would have detected them (but
   not non-fatal errors involving downcasting).

I still haven't checked if the old ABI still works.  The compatibilty code
looks OK.

>>> 	putint(total.t_rq - 1, PROCSROW + 2, PROCSCOL, 3);
>>> 	putint(total.t_pw, PROCSROW + 2, PROCSCOL + 4, 3);
>>> 	putint(total.t_dw, PROCSROW + 2, PROCSCOL + 8, 3);
>>
>> This has larger sign extension bugs than before.  All the fields here are
>> still int16_t.  putint() still takes int args, and so the int16_t's int
>> converted to int.  The used to be printed as int, but now they are converted
>> to uint64_t.  They shouldn't be negative, but if they are then the were printed
>> as negative.  Now the conversion to uint64_t has sign-extension bugs/overflows
>> for negative values.  Negative values still be printed as negative via further
>> overflows, but if the values are passed to dehumanize_number(), they are
>> printed as enormous unsigned values.
> I do not see a point in expanding the process counters to uint16_t. I
> might do it if somebody has a realistic load with 30K processes in a
> system.

Using unsigned types should be a last resort to get 1 more bit, but since
space is not a problem (but the ABI is), if 32K is not enough for someone
then they should be 32-bits signed not 16 bits unsigned.

> Having 100K threads created simultaneously is quite affordable, so the
> change could be useful one day.

Another change of the ABI is not so good.

> ...
>>> @@ -518,13 +522,13 @@ showkre(void)
>>> 	PUTRATE(v_pdwakeups, VMSTATROW + 9, VMSTATCOL, 8);
>>> 	PUTRATE(v_pdpages, VMSTATROW + 10, VMSTATCOL, 8);
>>> 	PUTRATE(v_intrans, VMSTATROW + 11, VMSTATCOL, 8);
>>> -	putint(pgtokb(s.v_wire_count), VMSTATROW + 12, VMSTATCOL, 8);
>>> -	putint(pgtokb(s.v_active_count), VMSTATROW + 13, VMSTATCOL, 8);
>>> -	putint(pgtokb(s.v_inactive_count), VMSTATROW + 14, VMSTATCOL, 8);
>>> -	putint(pgtokb(s.v_laundry_count), VMSTATROW + 15, VMSTATCOL, 8);
>>> -	putint(pgtokb(s.v_free_count), VMSTATROW + 16, VMSTATCOL, 8);
>>> +	putuint64(pgtokb(s.v_wire_count), VMSTATROW + 12, VMSTATCOL, 8);
>>> +	putuint64(pgtokb(s.v_active_count), VMSTATROW + 13, VMSTATCOL, 8);
>>> +	putuint64(pgtokb(s.v_inactive_count), VMSTATROW + 14, VMSTATCOL, 8);
>>> +	putuint64(pgtokb(s.v_laundry_count), VMSTATROW + 15, VMSTATCOL, 8);
>>> +	putuint64(pgtokb(s.v_free_count), VMSTATROW + 16, VMSTATCOL, 8);
>>
>> This is bogus.  The fields still have type u_int.  pgtokb() expands by a
>> factor of 4 or 8, and putuint64() can handle the expansion, but pgtokb()
>> still overflows before the value can be passed.
> These fields come from different structure (vmtotal vs. vmmeter), and
> surprisingly vmmeter is not part of the ABI.  Expanding the type of
> v_free_count is easy.  Having the changes in vmstat for vmmeter fields
> done together with vmtotal expansion is reasonable.  We targeted all
> memory reporting.

BTW, I once thought that the VM_METER sysctl should actually return all
vmmeter statistics (at actually returns only struct vmtotal and the API
was unimproved by renaming it to VM_TOTAL).  But then the ABI would have
been harder to change.

Portable code for the about should use intmax_t (or better double).

> ...
>>> 	if (LINES - 1 > VMSTATROW + 17)
>>> -		putint(s.bufspace / 1024, VMSTATROW + 17, VMSTATCOL, 8);
>>> +		putuint64(s.bufspace / 1024, VMSTATROW + 17, VMSTATCOL, 8);
>>
>> s.bufspace has the bogus type long, so in 32-bit systems overflow occurs
>> with only 2T of bufspace, and on 64-bit systems the expansion can overflow
>> even uint64_t, but the world doesn't have that much memory (2T * 2**32).
> So practically overflow cannot occur.  I do not quite see the point of the
> comment.

There is a silly mixture of types that gives more overflow cases to
avoid.  This long is basically intmax_t written before intmax_t existed.
Both long and intmax_t automatically expand with the register size.
Except, ABI problems inhibit expanding either.

> ...
>>> +	snr = snprintf(b, sizeof(b), "%*jd", w, (uintmax_t)n);
>>
>> The signed format no longer matches the unsigned arg.  It still gives the
>> early warning hack via overflow earlier in most cases.  The behaviour here
>> is undefined iff (uintmax_t)n exceeds INTMAX_MAX.
> Thanks, see the patch at the end.

Too short to fix all that I want :-).

Bruce