Page MenuHomeFreeBSD

systat: use and correctly display 64bit counters
ClosedPublic

Authored by kaktus on Nov 15 2017, 4:18 PM.

Details

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 <pawel.biernacki@gmail.com>
Sponsored by: Mysterious Code Ltd.
PR: 2137

Test Plan

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

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

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 ↗(On Diff #35283)

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

686 ↗(On Diff #35283)

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

687 ↗(On Diff #35283)

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

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

usr.bin/systat/vmstat.c
686 ↗(On Diff #35283)

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

  • Update with long diff.
  • Use correct formatting for uint64_t in snprintf.
usr.bin/systat/vmstat.c
687 ↗(On Diff #35283)

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

687 ↗(On Diff #35283)

s/code/string/

usr.bin/systat/vmstat.c
686 ↗(On Diff #35283)

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

684 ↗(On Diff #35293)

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.

718 ↗(On Diff #35293)

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 ?

750 ↗(On Diff #35293)

Same as above.

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 added inline comments.
usr.bin/systat/vmstat.c
718 ↗(On Diff #35293)

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%.

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.

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.

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().

Use correctly generated diff.

No need to use 64 bit arithmetics for namei cache.

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

usr.bin/systat/vmstat.c
671 ↗(On Diff #35519)

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

681 ↗(On Diff #35519)

Perhaps name this function do_putuint64().

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 edited the summary of this revision. (Show Details)

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

kaktus edited the summary of this revision. (Show Details)

s/divider/divisor/

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.

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.

This revision is now accepted and ready to land.Nov 21 2017, 7:55 PM
This revision was automatically updated to reflect the committed changes.