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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kaktus created this revision.Nov 15 2017, 4:18 PM
kib added a comment.Nov 15 2017, 4:54 PM

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.

kaktus updated this revision to Diff 35293.Nov 15 2017, 7:16 PM
  • Update with long diff.
  • Use correct formatting for uint64_t in snprintf.
kaktus added inline comments.Nov 15 2017, 7:18 PM
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/

kaktus marked an inline comment as done.Nov 15 2017, 7:23 PM
kib added inline comments.Nov 16 2017, 4:50 PM
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.

kaktus updated this revision to Diff 35354.Nov 16 2017, 10:16 PM

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 marked an inline comment as done.Nov 16 2017, 10:17 PM
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%.

kaktus marked an inline comment as done.Nov 16 2017, 10:18 PM
kib added a comment.Nov 17 2017, 4:20 PM
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.

kaktus updated this revision to Diff 35514.Nov 21 2017, 12:33 AM

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 updated this revision to Diff 35515.Nov 21 2017, 12:36 AM

Use correctly generated diff.

kaktus updated this revision to Diff 35519.Nov 21 2017, 2:36 AM

No need to use 64 bit arithmetics for namei cache.

kib added a comment.Nov 21 2017, 9:46 AM

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 updated this revision to Diff 35537.Nov 21 2017, 10:33 AM
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)Nov 21 2017, 10:33 AM
kaktus updated this revision to Diff 35538.Nov 21 2017, 10:38 AM
kaktus edited the summary of this revision. (Show Details)

s/divider/divisor/

kaktus edited the summary of this revision. (Show Details)Nov 21 2017, 11:43 AM
kib added a comment.Nov 21 2017, 1:16 PM

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 added a comment.Nov 21 2017, 2:02 PM

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.

robak resigned from this revision.Nov 21 2017, 2:04 PM
robak removed a reviewer: robak.Nov 21 2017, 3:12 PM
kib accepted this revision.Nov 21 2017, 7:55 PM
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.