From owner-svn-src-all@freebsd.org Wed Nov 22 17:24:24 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 1E254DF08CB; Wed, 22 Nov 2017 17:24:24 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail107.syd.optusnet.com.au (mail107.syd.optusnet.com.au [211.29.132.53]) by mx1.freebsd.org (Postfix) with ESMTP id B5D2A2639; Wed, 22 Nov 2017 17:24:23 +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 mail107.syd.optusnet.com.au (Postfix) with ESMTPS id 6D8DAD4E12D; Thu, 23 Nov 2017 04:24:14 +1100 (AEDT) Date: Thu, 23 Nov 2017 04:24:13 +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: <20171122103917.GS2272@kib.kiev.ua> Message-ID: <20171123021646.M1933@besplex.bde.org> References: <201711211955.vALJtWhg047906@repo.freebsd.org> <20171122071838.R1172@besplex.bde.org> <20171122103917.GS2272@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=bc8baKHB c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=kcmJ2rNy8ptLwktde7kA:9 a=QHhIYrB9zNK-slCL:21 a=tSUsDk6LL9iiXYT8:21 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: Wed, 22 Nov 2017 17:24:24 -0000 On Wed, 22 Nov 2017, Konstantin Belousov wrote: > On Wed, Nov 22, 2017 at 08:59:21AM +1100, Bruce Evans wrote: >> On Tue, 21 Nov 2017, Konstantin Belousov wrote: >> >>> Log: >>> systat: use and correctly display 64bit counters. >> ... >> Using unsigned types gives sign extension bugs as usual. In old versions >> ... >>> @@ -491,15 +495,15 @@ showkre(void) >>> putfloat(100.0 * s.v_kmem_map_size / kmem_size, >>> STATROW + 1, STATCOL + 22, 2, 0, 1); >>> >>> - putint(pgtokb(total.t_arm), MEMROW + 2, MEMCOL + 4, 7); >>> - putint(pgtokb(total.t_armshr), MEMROW + 2, MEMCOL + 12, 7); >>> - putint(pgtokb(total.t_avm), MEMROW + 2, MEMCOL + 20, 8); >>> - putint(pgtokb(total.t_avmshr), MEMROW + 2, MEMCOL + 29, 8); >>> - putint(pgtokb(total.t_rm), MEMROW + 3, MEMCOL + 4, 7); >>> - putint(pgtokb(total.t_rmshr), MEMROW + 3, MEMCOL + 12, 7); >>> - putint(pgtokb(total.t_vm), MEMROW + 3, MEMCOL + 20, 8); >>> - putint(pgtokb(total.t_vmshr), MEMROW + 3, MEMCOL + 29, 8); >>> - putint(pgtokb(total.t_free), MEMROW + 2, MEMCOL + 38, 7); >>> + putuint64(pgtokb(total.t_arm), MEMROW + 2, MEMCOL + 4, 7); >>> + putuint64(pgtokb(total.t_armshr), MEMROW + 2, MEMCOL + 12, 7); >>> + putuint64(pgtokb(total.t_avm), MEMROW + 2, MEMCOL + 20, 8); >>> + putuint64(pgtokb(total.t_avmshr), MEMROW + 2, MEMCOL + 29, 8); >>> + putuint64(pgtokb(total.t_rm), MEMROW + 3, MEMCOL + 4, 7); >>> + putuint64(pgtokb(total.t_rmshr), MEMROW + 3, MEMCOL + 12, 7); >>> + putuint64(pgtokb(total.t_vm), MEMROW + 3, MEMCOL + 20, 8); >>> + putuint64(pgtokb(total.t_vmshr), MEMROW + 3, MEMCOL + 29, 8); >>> + putuint64(pgtokb(total.t_free), MEMROW + 2, MEMCOL + 38, 7); >> >> I see that a very recent expansion from int32_t to uint64_t didn't work >> here. > Can you explain, please ? Just that systat -v didn't automatically benefit from the expansion (since it is not very well written, with hard-coded types. Hard-coded ints were at least simple. Now it has lots of hard-coded uint64_t's, partly to work around dehumanize_number() being broken as designed (this function doesn't support numbers, but only supports uint64_t. It would be a bit much for it to support surreal numbers, but it doesn't even support signed integers)). I checked all programs in /usr/src that use t_avm (t_vm is too hard to grep for). (There aren't many. Statistics utilities in ports probably have more variations and more bugs altogether.) None automatically benefited from the change, and all still have sign type errors: - systat/vmstat.c as above - vmstat/vmstat.c: This is much uglier, since it has flags to control the dehumanized printing and uses libxo so it us ugly even without all the lexical style bugs added with libxo. vmstat does most things worse: - with hflag (this gives the dehumanized printing), it tries to print memory sizes in bytes. Even humans would have problems reading the large decimal numbers resulting from sizes in bytes, but limited field widths usually result in dehumanization to K or M (rarely G or T). systat -v tries to print sizes in K (without the usually-redundant K suffix) more consistently, but this has problems when the values in K don't fit: vmstat does one thing better: if the size is too large to express in K, say just 4M, then vmstat will print 4M starting with the size in bytes, but vmstat starts with the size in K (4096) and will now print this as 4K, while the old version printed it as 4k. 4K of a size in K is almost as confusing as 4k of a size in K. This is one of the few places where the expansion worked automatically, because prthuman() already uses uint64_t (misspelled u_int64_t). The casts of sum.v_page_size to u_int64_t is now redundant as well as misspelled. It was needed even with int32_t fields because of the bad units -- 2G in bytes is quite small. This casts also gave sign extension/overflow. Now it has no effect. Such casts are safer, but uint64_t hard-coded in so many places the code might as well be simplified by hard-coding here too. - the !hflag case is broken by blind expansion to use more 4 more columns than are available even when all the fields fit (large numbers mess this up more). Also, the headers are broken with or without hflag (the first header line no longer lines up with the second header line. - without hflag, the code is similar to systat, but much more broken. It has the same fairly hard to reach overflows in pgtokb(). Now it has undefined behaviour in all cases in xo_emit(), since the arg type changed to uint64_t but the format is still %7d. Previously this only had undefined behaviour in overflowing cases (t_avm was int32_t, but v_page_size is u_int, so pgtokb() had type u_int; in overflow cases its value exceeds INT_MAX so the behaviour is undefined even though the overflow didn't give undefined behaviour. xo_emit() is not declared as __printflike(), so the undefined behaviour cannot be detected by the compiler. In systat, these bugs were reduced by forcing pgtokb() back to int by passing them to putnum(). The behaviour was implementation-defined (truncate). release/picobsd/tinyware/vm/vm.c: This is a tiny program, though not so tiny with 64-bit integers. It does a small part of what vmstat does, and has the same undefined behaviour from using %7d format to print pgtok() which now has type uint64_t. Since it doesn't have libxo, printf format checking might detect the error. But the Makefile doesn't set WARNS. sysctl/sysctl.c: sysctl(8) has bogus support for prettyprinting struct vmtotal (sysctl shouldn't have any prettyprinting, especially not for structs that have specialized programs to print them and much more). This uses intmax_t for all calculations and printing except for the int16_t fields, so it automatically benefited from the expansion. However since it uses a correct type (signed, and not restricted to 64 bits), it now has minor type errors -- it dowcasts the uint64_t to intmax_t wheen the latter is 64 bits. Its Makefile uses a fairly high WARNS, so if its type errors were fatal then printf format checking would have detected them (but not non-fatal errors involving downcasting). I still haven't checked if the old ABI still works. The compatibilty code looks OK. >>> putint(total.t_rq - 1, PROCSROW + 2, PROCSCOL, 3); >>> putint(total.t_pw, PROCSROW + 2, PROCSCOL + 4, 3); >>> putint(total.t_dw, PROCSROW + 2, PROCSCOL + 8, 3); >> >> This has larger sign extension bugs than before. All the fields here are >> still int16_t. putint() still takes int args, and so the int16_t's int >> converted to int. The used to be printed as int, but now they are converted >> to uint64_t. They shouldn't be negative, but if they are then the were printed >> as negative. Now the conversion to uint64_t has sign-extension bugs/overflows >> for negative values. Negative values still be printed as negative via further >> overflows, but if the values are passed to dehumanize_number(), they are >> printed as enormous unsigned values. > I do not see a point in expanding the process counters to uint16_t. I > might do it if somebody has a realistic load with 30K processes in a > system. Using unsigned types should be a last resort to get 1 more bit, but since space is not a problem (but the ABI is), if 32K is not enough for someone then they should be 32-bits signed not 16 bits unsigned. > Having 100K threads created simultaneously is quite affordable, so the > change could be useful one day. Another change of the ABI is not so good. > ... >>> @@ -518,13 +522,13 @@ showkre(void) >>> PUTRATE(v_pdwakeups, VMSTATROW + 9, VMSTATCOL, 8); >>> PUTRATE(v_pdpages, VMSTATROW + 10, VMSTATCOL, 8); >>> PUTRATE(v_intrans, VMSTATROW + 11, VMSTATCOL, 8); >>> - putint(pgtokb(s.v_wire_count), VMSTATROW + 12, VMSTATCOL, 8); >>> - putint(pgtokb(s.v_active_count), VMSTATROW + 13, VMSTATCOL, 8); >>> - putint(pgtokb(s.v_inactive_count), VMSTATROW + 14, VMSTATCOL, 8); >>> - putint(pgtokb(s.v_laundry_count), VMSTATROW + 15, VMSTATCOL, 8); >>> - putint(pgtokb(s.v_free_count), VMSTATROW + 16, VMSTATCOL, 8); >>> + putuint64(pgtokb(s.v_wire_count), VMSTATROW + 12, VMSTATCOL, 8); >>> + putuint64(pgtokb(s.v_active_count), VMSTATROW + 13, VMSTATCOL, 8); >>> + putuint64(pgtokb(s.v_inactive_count), VMSTATROW + 14, VMSTATCOL, 8); >>> + putuint64(pgtokb(s.v_laundry_count), VMSTATROW + 15, VMSTATCOL, 8); >>> + putuint64(pgtokb(s.v_free_count), VMSTATROW + 16, VMSTATCOL, 8); >> >> This is bogus. The fields still have type u_int. pgtokb() expands by a >> factor of 4 or 8, and putuint64() can handle the expansion, but pgtokb() >> still overflows before the value can be passed. > These fields come from different structure (vmtotal vs. vmmeter), and > surprisingly vmmeter is not part of the ABI. Expanding the type of > v_free_count is easy. Having the changes in vmstat for vmmeter fields > done together with vmtotal expansion is reasonable. We targeted all > memory reporting. BTW, I once thought that the VM_METER sysctl should actually return all vmmeter statistics (at actually returns only struct vmtotal and the API was unimproved by renaming it to VM_TOTAL). But then the ABI would have been harder to change. Portable code for the about should use intmax_t (or better double). > ... >>> if (LINES - 1 > VMSTATROW + 17) >>> - putint(s.bufspace / 1024, VMSTATROW + 17, VMSTATCOL, 8); >>> + putuint64(s.bufspace / 1024, VMSTATROW + 17, VMSTATCOL, 8); >> >> s.bufspace has the bogus type long, so in 32-bit systems overflow occurs >> with only 2T of bufspace, and on 64-bit systems the expansion can overflow >> even uint64_t, but the world doesn't have that much memory (2T * 2**32). > So practically overflow cannot occur. I do not quite see the point of the > comment. There is a silly mixture of types that gives more overflow cases to avoid. This long is basically intmax_t written before intmax_t existed. Both long and intmax_t automatically expand with the register size. Except, ABI problems inhibit expanding either. > ... >>> + snr = snprintf(b, sizeof(b), "%*jd", w, (uintmax_t)n); >> >> The signed format no longer matches the unsigned arg. It still gives the >> early warning hack via overflow earlier in most cases. The behaviour here >> is undefined iff (uintmax_t)n exceeds INTMAX_MAX. > Thanks, see the patch at the end. Too short to fix all that I want :-). Bruce