From owner-svn-src-all@freebsd.org Fri Nov 24 09:15:09 2017 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 8389ADC1BBA; Fri, 24 Nov 2017 09:15:09 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail106.syd.optusnet.com.au (mail106.syd.optusnet.com.au [211.29.132.42]) by mx1.freebsd.org (Postfix) with ESMTP id 32BF86C2C5; Fri, 24 Nov 2017 09:15:08 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail106.syd.optusnet.com.au (Postfix) with ESMTPS id B4BBF3C5900; Fri, 24 Nov 2017 20:15:06 +1100 (AEDT) Date: Fri, 24 Nov 2017 20:15:06 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Konstantin Belousov cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r326073 - head/usr.bin/systat In-Reply-To: <20171123151849.GU2272@kib.kiev.ua> Message-ID: <20171124184535.E980@besplex.bde.org> 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> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=KeqiiUQD c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=TKKoXLfbl_5D-oQeqmUA:9 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 24 Nov 2017 09:15:09 -0000 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