Date: Fri, 24 Nov 2017 20:15:06 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Konstantin Belousov <kostikbel@gmail.com> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r326073 - head/usr.bin/systat Message-ID: <20171124184535.E980@besplex.bde.org> In-Reply-To: <20171123151849.GU2272@kib.kiev.ua> References: <201711211955.vALJtWhg047906@repo.freebsd.org> <20171122071838.R1172@besplex.bde.org> <20171122103917.GS2272@kib.kiev.ua> <20171123021646.M1933@besplex.bde.org> <20171122220538.GT2272@kib.kiev.ua> <20171123224032.A992@besplex.bde.org> <20171123151849.GU2272@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 23 Nov 2017, Konstantin Belousov wrote:
> On Fri, Nov 24, 2017 at 12:10:09AM +1100, Bruce Evans wrote:
>> On Thu, 23 Nov 2017, Konstantin Belousov wrote:
>* ...
>>> Below is the cast to uintmax_t and unsigned format for sysctl(8).
>>
>> This adds style bugs by expanding lines from length 79 to 81.
>>
>>> diff --git a/sbin/sysctl/sysctl.c b/sbin/sysctl/sysctl.c
>>> index e1bf4e31914..92685a8171b 100644
>>> --- a/sbin/sysctl/sysctl.c
>>> +++ b/sbin/sysctl/sysctl.c
>* ...
>> All of the casts to uintmax_t can be avoided be avoided by changing the type
>> of pageKilo from int to uintmax_t.
>>
>> Better, do the conversion in a function-like macro pgtokb() as is done in
>> all (?) other utilities, but without so many overflow bugs as in most other
>> utiities:
>>
>> #define pgtok(p) ((uintmax_t)(p) * pageKilo)
> Amusingly there is already MD macro in machine/param.h with the same name
> and same intent, but as you formulate it, sloppy implementation. It uses
> unsigned long cast on almost all 64bit arches, except powerpc. For 32bit
> arches, the cast is not done, unfortunately.
I already pointed out the system pgtok().
It was almost correct to cast to u_int on all arches because the
implementation can know the size of all its page counters. That was int
or u_int, and the expansion factor is small. In practice, 32-bit systems
never have enough memory to overflow in K (that happens at 4TB), and
64-bit systems that overflow in K are close to overflowing the page
counters (that happens at 16TB with 4K-pages).
The pgtok() is just unusable because the page size can vary.
Perhaps it is a design error to allow the page size to vary or be
anything except 512 in APIs, just like for disk sector sizes. Most
disk APIs uses units of bytes or 512-blocks. The physical size may
be different. Applications need to know the physical memory page size
even less than they need to know the physical sector size.
>> (pageKilo is back to int). This is still sloppy:
>> - pageKilo = getpagesize() / 1024 assumes that the page size is a multiple
>> of 1024 and fails very badly when the page size is 512
>> - the multiplication can still overflow
>> - when the size in K is too large to fit, the size in M or G will normally
>> fit and converting directly to would avoid the overflow assuming that
>> the page size is <= 1M.
>>
>> Using floating point (even float precision) avoids all overflow and rounding
>> problems up to almost 128-bit sizes in bytes:
> No, I do not want to use floating point calculation there.
Why not? I don't want to use it here, but that is because I don't want
the whole S_vmtotal() function here. I want to use it in most places that
print large values. Using it in systat is most natural since floating
point is already used a lot there. OTOH, I don't like libdevstat using
it, sepecially long double. It doesn't simplify much except representation
of rates in libdevstat. The high precision of long double is especially
not needed for device statistics, and the precision cannot be depended on
to be more than double since many arches only have fake long double. Using
it for libdevstat just pessimizes for arches that have non-fake long double.
> diff --git a/sbin/sysctl/sysctl.c b/sbin/sysctl/sysctl.c
> index e1bf4e31914..36851f302a0 100644
> --- a/sbin/sysctl/sysctl.c
> +++ b/sbin/sysctl/sysctl.c
> @@ -608,14 +608,18 @@ S_timeval(size_t l2, void *p)
> static int
> S_vmtotal(size_t l2, void *p)
> {
> - struct vmtotal *v = (struct vmtotal *)p;
> - int pageKilo = getpagesize() / 1024;
> + struct vmtotal *v;
> + int pageKilo;
>
> if (l2 != sizeof(*v)) {
> warnx("S_vmtotal %zu != %zu", l2, sizeof(*v));
> return (1);
> }
>
> + v = p;
> + pageKilo = getpagesize() / 1024;
OK.
> +
> +#define pg2k(a) ((uintmax_t)(a) * pageKilo)
"2" is a bad abbreviation for "to". It isn't even shorter, and requires
cetain language skils to understand, and is especially unsuitable for
conversion functions since the digit 2 might mean a conversion factor.
> + printf("Free Memory:\t%juK", pg2k(v->t_free));
> +#undef pg2k
No need to undef it. It is in the application namespace.
Here are my old fixes for this function (to clean it up before removing it):
X Index: sysctl.c
X ===================================================================
X RCS file: /home/ncvs/src/sbin/sysctl/sysctl.c,v
X retrieving revision 1.86
X diff -u -2 -r1.86 sysctl.c
X --- sysctl.c 11 Jun 2007 13:02:15 -0000 1.86
X +++ sysctl.c 25 Sep 2017 07:04:54 -0000
X @@ -379,5 +381,5 @@
X {
X struct vmtotal *v = (struct vmtotal *)p;
X - int pageKilo = getpagesize() / 1024;
X + int pageKilo;
X
X if (l2 != sizeof(*v)) {
I fixed 1 of the initializatons in declarations. The other one is not
so bad (except for its redundant cast). This is a common style for void *
args and would be needed with const void * args.
X @@ -385,24 +387,19 @@
X return (1);
X }
X -
X - printf(
X - "\nSystem wide totals computed every five seconds:"
X - " (values in kilobytes)\n");
X + pageKilo = getpagesize() / 1024;
X + printf("\nSystem wide totals computed every five seconds:\n");
Fix style bugs in outout (verboseness).
Fix style bugs in source code (obfuscation of the string by splitting it,
and unnecessary splitting after 'printf('.
There is an unfixed problem with the newline at the start. For sysctl -n,
it is just wrong. It is to start the header on a new line for sysctl
without -n. This breaks grepability of sysctl -a output, but the multi-
line output breaks that anyway.
X printf("===============================================\n");
X printf(
X - "Processes:\t\t(RUNQ: %hd Disk Wait: %hd Page Wait: "
X - "%hd Sleep: %hd)\n",
X - v->t_rq, v->t_dw, v->t_pw, v->t_sl);
X - printf(
X - "Virtual Memory:\t\t(Total: %dK, Active %dK)\n",
X +"Processes:\t\t(RUNQ %d, Disk Wait %d, Page Wait %d, Sleep %d, Swap %d)\n",
X + v->t_rq, v->t_dw, v->t_pw, v->t_sl, v->t_sw);
X + printf("Virtual Memory:\t\t(Total %luK, Active %dK)\n",
X v->t_vm * pageKilo, v->t_avm * pageKilo);
Outdent the string to unobfuscate it.
Don't us fancy %hd format. I think the fields still have type int16_t,
so using %hd has no effect now, but would break expansion to int. AFAIK,
the only use for %hd is to convert int args that aren't actually representable
as shorts back to shorts before printing them.
Use commas after 'keyword: value' pairs and don't use semicolons after
keywords here and everywhere.
X - printf("Real Memory:\t\t(Total: %dK Active %dK)\n",
X + printf("Real Memory:\t\t(Total %dK, Active %dK)\n",
X v->t_rm * pageKilo, v->t_arm * pageKilo);
X - printf("Shared Virtual Memory:\t(Total: %dK Active: %dK)\n",
X + printf("Shared Virtual Memory:\t(Total %dK, Active %dK)\n",
X v->t_vmshr * pageKilo, v->t_avmshr * pageKilo);
X - printf("Shared Real Memory:\t(Total: %dK Active: %dK)\n",
X + printf("Shared Real Memory:\t(Total %dK, Active %dK)\n",
X v->t_rmshr * pageKilo, v->t_armshr * pageKilo);
This is for an old version, so it still prints plain ints.
X - printf("Free Memory Pages:\t%dK\n", v->t_free * pageKilo);
X -
X + printf("Free Memory:\t\t%dK", v->t_free * pageKilo);
Fix units (remove Pages). Already done in -current.
Remove another extra blank line.
X return (0);
X }
Grepping for 'Pages' shows it in further bloat for memory size printing
(efi and/or smap for amd64 and/or i386 only). 1 place for efi seems
to print a page count, while most places print lengths or offsets in
bytes, and for that it is essential to use hex format which is done
for smap (0x%16jx format). I think efi should support larger sizes,
but it only uses %012lx %12p %08lx formats (here %12p is because md_virt
has the non-vm type void *; this has various problems -- IIRC it gives
an 0x prefix which the other fields don't have, and the width 12 is
ignored in userland. The other widths are too small. The other field
types uint64_t (even for md_pages). The field widths of 12 and 8 are
too small for uint64_t, and the lx only work for uint64_t because this
code is only used by amd64.
I don't use efi, and normally see smap sizes only after booting with -v.
This is one of the things broken by late initialization of consoles in
-current (lots of printfs are done before consoles or the msgbuf are
initialized).
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20171124184535.E980>