systat: use and correctly display 64bit counters

[kaktus]

Summary

Following struct vmtotal changes, make systat use and correctly display 64-bit counters. Switch to humanize_number(3) to overcome homegrown arithmetics limits in pretty printing large numbers. Use 1024 as a divisor for memory fields to make it consistent with other tools and users expectations.

Submitted by: Pawel Biernacki <[e-mail]>
Sponsored by: Mysterious Code Ltd.
PR: 2137

Test Plan

Allocate large amount of virtual memory, run systat -vmstat, observe VIRTUAL Tot and Share fields.

[kib]

Make the diff with -U999999 to preserve full context, otherwise use of the review system lose its only advantage.

usr.bin/systat/vmstat.c

683

The format specifier is wrong, type of n is not uintmax_t.

686

Why use DIV_1000 ? It is completely unexpected to have decimal suffix for the memory size. We do not sell harddrives.

687

Why do you ever need this snprintf() ? To copy the string, use strlcpy().

[kaktus]

Sorry, uploaded the short patch. Will upload long one with fixes.

usr.bin/systat/vmstat.c

686

I kept what the old version did, which was dividing bo 1000.

[kaktus]

• Update with long diff.
• Use correct formatting for uint64_t in snprintf.

[kaktus]

usr.bin/systat/vmstat.c

687

Because it pads the code to the length of w, returning the whole length that is used later.

687

s/code/string/

[kib]

usr.bin/systat/vmstat.c

686

I think it is reasonable to use natural suffixes, instead of decimals. Especially because we do not stop at k.

698–703

The cast of n to uintmax_t is what lot of places in the FreeBSD tree use. I believe there is almost no uses of PRIi64 in the non-contributed code, and some people dislike this feature loudly.

736

Why the explicit cast for f is needed ?

That said, use of humanize_number for floating-point value is weird. Why do you change this code, for which field it matters ?

768

Same as above.

[kaktus]

Address @kib comments:

• Don't use PRIi64, just cast to unitmax_t.
• Don't use humanise_number in floating point.
• Drop 1000 as a divider and stick to expected 1024.

[kaktus]

usr.bin/systat/vmstat.c

736

I did the change for consistency with the putint() function. As you see in case of number larger enough the decimal part is dropped. Anyway, I think that the code is dead anyway because I wasn't able to cause any of the % field to grow beyond 100%.

[kib]

In D13105#272897, @pawel.biernacki-gmail.com wrote:

Address @kib comments:

• Drop 1000 as a divider and stick to expected 1024.

So we have two uses of putint(). One is for (possibly very large) VM counters, and others for (usually not too large) event counts e.g. syscall/intr and so on.

Should the new function used only for 'large' counters, leaving old display algorithm around for slow counters ? This question jibes with the note that changed function should be called putuint64.

[kaktus]

In D13105#273075, @kib wrote:

In D13105#272897, @pawel.biernacki-gmail.com wrote:

Address @kib comments:

• Drop 1000 as a divider and stick to expected 1024.

So we have two uses of putint(). One is for (possibly very large) VM counters, and others for (usually not too large) event counts e.g. syscall/intr and so on.

Should the new function used only for 'large' counters, leaving old display algorithm around for slow counters ? This question jibes with the note that changed function should be called putuint64.

As in to have a one function with 1000 divider for non memory related fields and second one that uses 1024 and potentially larger numbers? I don't know if putuint64 would be descriptive enough to mark the distinction. I'll prepare some patch and we'll see if it fits.

[kaktus]

Split printing integer values to putint() and putuint64() functions, calling the actual implementation with parameter selecting using SI or IEC divisor. Update all memory related values to use putuint64().

[kaktus]

Use correctly generated diff.

[kaktus]

No need to use 64 bit arithmetics for namei cache.

[kib]

Add complete 'Submitted by:' line into the revision summary.

usr.bin/systat/vmstat.c

671

Blank line is required by style(9) after '{' if no local vars are defined.

681

Perhaps name this function do_putuint64().

[robak]

In D13105#274420, @kib wrote:

Add complete 'Submitted by:' line into the revision summary.

I'm committing these changes once they have approval from src comitter with relevant experience, so I'm always working out commit messages with Paweł in the end, so that they'd follow the requirements.

[kaktus]

Rename real_putuint64() to do_putuint64(), style(9) fixes.

[kaktus]

s/divider/divisor/

[kib]

In D13105#274447, @robak wrote:

I'm committing these changes once they have approval from src comitter with relevant experience, so I'm always working out commit messages with Paweł in the end, so that they'd follow the requirements.

I fail to see why do I need an intermediary to commit the changes I review and test.

[robak]

I fail to see why do I need an intermediary to commit the changes I review and test.

You don't of course. Simply, it was easier for both Pawel and myself to find a committer willing to just review and approve the code, and then I would handle rest (testing, proper commit message, commit itself) than to find a committer willing to do all of it. This way we've been able to move faster and push more changes. Feel free to act on it in any way and length you find appropriate, in the end it's you who owns src commit bit.