From owner-svn-src-head@freebsd.org Tue May 2 12:17:18 2017 Return-Path: Delivered-To: svn-src-head@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 6ECE6D5A959; Tue, 2 May 2017 12:17:18 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id EF33EBA3; Tue, 2 May 2017 12:17:17 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id v42CHBTf076523 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Tue, 2 May 2017 15:17:11 +0300 (EEST) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua v42CHBTf076523 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id v42CHB8o076522; Tue, 2 May 2017 15:17:11 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Tue, 2 May 2017 15:17:11 +0300 From: Konstantin Belousov To: Bruce Evans Cc: Alan Somers , Gleb Smirnoff , "src-committers@freebsd.org" , "svn-src-all@freebsd.org" , "svn-src-head@freebsd.org" Subject: Re: svn commit: r317061 - in head: libexec/rpc.rstatd sys/amd64/amd64 sys/amd64/include sys/arm/arm sys/arm/include sys/arm64/include sys/cddl/contrib/opensolaris/uts/common/fs/zfs sys/compat/linprocfs... Message-ID: <20170502121711.GE1622@kib.kiev.ua> References: <201704171734.v3HHYlf5022945@repo.freebsd.org> <20170419130428.J956@besplex.bde.org> <20170430201324.GV1622@kib.kiev.ua> <20170501163725.U972@besplex.bde.org> <20170502095527.GB1622@kib.kiev.ua> <20170502203703.I1176@besplex.bde.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170502203703.I1176@besplex.bde.org> User-Agent: Mutt/1.8.2 (2017-04-18) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 02 May 2017 12:17:18 -0000 On Tue, May 02, 2017 at 09:25:40PM +1000, Bruce Evans wrote: > On Tue, 2 May 2017, Konstantin Belousov wrote: > > > On Mon, May 01, 2017 at 06:25:11PM +1000, Bruce Evans wrote: > >> XX Index: subr_counter.c > >> XX =================================================================== > >> XX --- subr_counter.c (revision 317604) > >> XX +++ subr_counter.c (working copy) > >> XX @@ -78,11 +78,15 @@ > >> XX sysctl_handle_counter_u64(SYSCTL_HANDLER_ARGS) > >> XX { > >> XX uint64_t out; > >> XX + uint32_t out32; > >> XX int error; > >> XX > >> XX out = counter_u64_fetch(*(counter_u64_t *)arg1); > >> XX > >> XX - error = SYSCTL_OUT(req, &out, sizeof(uint64_t)); > >> XX + if (req->oldlen == 4 && (out32 = out) == out) > >> XX + error = SYSCTL_OUT(req, &out32, sizeof(out32)); > >> XX + else > >> XX + error = SYSCTL_OUT(req, &out, sizeof(out)); > >> XX > >> XX if (error || !req->newptr) > >> XX return (error); > >> > >> This does the minimum necessary to fix the current problem. > >> > >> This works until the counters become too large for a u_int. There > >> could be an option to get truncation with no error, but that would > >> require an API change, so applications should be required to do the > >> truncation for themself if that is what they want. > > > > Yes, this is approximately what I consider to be enough fix, but with > > some details which I do not like and which did not worked well on my > > test machine with enough memory to generate lot of increments. > > > > Below is what I consider to be good enough. I will proceed with this > > version unless very strong objections appear. > > I prefer my version. It is the same as for sysctl_bufspace(), except > for simplifications and bug fixes that are easier because the types > are unsigned. > > Your version is especially broken for the case of a lot of increments. You consider the 'differentiators' as the main users, but I only want systat and top to not abort, I have no intent of making them work for updated counters if overflown. Instead, I want something human-visible so that person looking at raw counters values using old tools detected the failure. > > Clamping breaks many cases in systat -v which just needs unsigned counters > to calculate small differences. Not many counters overflow faster than > the refresh interval in systat or top. These cases used to appear to > work perfectly for most counters when the kernel did blind truncations. > Clamping breaks the API. Not returing ENOMEM breaks the API. > > In my version, this is handled as specified by the API by truncating and > returning ENOMEM. Then: > - top sees the ENOMEM and mishandles it by aborting > - systat sees the ENOMEM (and also sees a short count, if it has the newer > breakage of hard-coding 64-bit counters instead of u_int ones, and is > run on a kernel with u_int counters) and mishandles it better by > printing an error message but not aborting. Thus it appears to work > just as well as when the kernel did blind truncation, except for the > error message. ENOMEM is, of course, the situation which I want to avoid. > > Clamping would be difficult for the application to handle even if an > error is returned. This would be like the situation for clamping for > strtoul(), but worse since it changes the API. Many applications don't > understand how to handle errno for strtoul(), but they do do a bogus > check of the clamped value. For sysctl(), clamping is not part of > the API now, so applications shouldn't check for the clamped values. > Ones that work with deltas like systat -v would see the counters stop > incrementing when the clamped value is reached, while truncated values > allow them to continue to appear to work perfectly. > > > diff --git a/sys/vm/vm_meter.c b/sys/vm/vm_meter.c > > index 5f4cd46ab1e..93d5161de89 100644 > > --- a/sys/vm/vm_meter.c > > +++ b/sys/vm/vm_meter.c > > @@ -266,8 +266,29 @@ static SYSCTL_NODE(_vm_stats, OID_AUTO, vm, CTLFLAG_RW, 0, > > "VM meter vm stats"); > > SYSCTL_NODE(_vm_stats, OID_AUTO, misc, CTLFLAG_RW, 0, "VM meter misc stats"); > > > > +static int > > +sysctl_handle_vmstat(SYSCTL_HANDLER_ARGS) > > +{ > > + uint64_t out; > > +#ifdef COMPAT_FREEBSD11 > > + uint32_t out1; > > +#endif > > The extra wrapper and the ifdef limit the damage from the API change. > Otherwise, I don't like them. > > I prefer my variable name out32 to the nondescript out1. ('out' should > also be renamed out64, but I wanted to keep the diffs small.) Changed to the out32 name. > > > + > > + out = counter_u64_fetch(*(counter_u64_t *)arg1); > > +#ifdef COMPAT_FREEBSD11 > > + if (req->oldlen == sizeof(uint32_t)) { > > sizeof(uint32_t) is an obfuscated spelling of '4'. The size is already > hard-coded in '32'. This depends on sizeof(char) being 1. It is, and > won't change. POSIX started specifying this in 2001, and the mere > existence of uint8_t probably requires 8-bit chars since u_char must > be the smallest type of an object. This depends on the size of a u_int > being 32 bits/4 bytes in the old ABI. This won't change, and is already > hard-coded as 32, as it must be because sizeof(u_int) may change. I prefer the sizeof over 4. > > > + out1 = 0xffffffff; > > + if (out < out1) > > + out1 = out; > > This does the clamping. Clamping is probably better expressed as: > > out1 = MIN(out, 0xffffffff) Changed as well. > > > + return (SYSCTL_OUT(req, &out1, sizeof(uint32_t))); > > Here sizeof(uint32_t) is an obfuscated spelling of sizeof(out1). I think > the sizeof() is justified in this case, to limit the hard-coding to of > sizes to 1 place. Truncation by assigment to the destination variable also > helps limit this > (sysctl_bufspace() could also be improved by truncation by assignment. > Its integers are signed, so the result is implementation-defined, but > we know that it is benign 2's complement and don't really care what it > is provided it is reasonable. This is better than laboriously checking > the lower limit or neglecting to check it). > Here the limit 0xffffffff also depends on the size. Yes, there is no UINT32_MAX, and never will. > > > + } > > +#endif > > + return (SYSCTL_OUT(req, &out, sizeof(uint64_t))); > > +} > > + > > #define VM_STATS(parent, var, descr) \ > > - SYSCTL_COUNTER_U64(parent, OID_AUTO, var, CTLFLAG_RD, &vm_cnt.var, descr) > > + SYSCTL_OID(parent, OID_AUTO, var, CTLTYPE_U64 | CTLFLAG_MPSAFE | \ > > + CTLFLAG_RD, &vm_cnt.var, 0, sysctl_handle_vmstat, "QU", descr); > > I see a new problem, shared with my version. The sysctl data says that this > sysctl is pure 64 bits, but it can return 32 bits. The sysctl data just > can't describe the variation. I guess this is not much a problem, since it > takes magic to read the sysctl data from the kernel and not many programs > except sysctl know how to do it. There programs will see the pure 64 > bits and only attempt to use 64 bit types. It can return 32bit only on improper use, which we allow for ABI compat. So I do not see this as a problem either. diff --git a/sys/vm/vm_meter.c b/sys/vm/vm_meter.c index 5f4cd46ab1e..67f24609b5a 100644 --- a/sys/vm/vm_meter.c +++ b/sys/vm/vm_meter.c @@ -266,8 +266,27 @@ static SYSCTL_NODE(_vm_stats, OID_AUTO, vm, CTLFLAG_RW, 0, "VM meter vm stats"); SYSCTL_NODE(_vm_stats, OID_AUTO, misc, CTLFLAG_RW, 0, "VM meter misc stats"); +static int +sysctl_handle_vmstat(SYSCTL_HANDLER_ARGS) +{ + uint64_t out; +#ifdef COMPAT_FREEBSD11 + uint32_t out32; +#endif + + out = counter_u64_fetch(*(counter_u64_t *)arg1); +#ifdef COMPAT_FREEBSD11 + if (req->oldlen == sizeof(uint32_t)) { + out32 = MIN(out, 0xffffffff); + return (SYSCTL_OUT(req, &out32, sizeof(uint32_t))); + } +#endif + return (SYSCTL_OUT(req, &out, sizeof(uint64_t))); +} + #define VM_STATS(parent, var, descr) \ - SYSCTL_COUNTER_U64(parent, OID_AUTO, var, CTLFLAG_RD, &vm_cnt.var, descr) + SYSCTL_OID(parent, OID_AUTO, var, CTLTYPE_U64 | CTLFLAG_MPSAFE | \ + CTLFLAG_RD, &vm_cnt.var, 0, sysctl_handle_vmstat, "QU", descr); #define VM_STATS_VM(var, descr) VM_STATS(_vm_stats_vm, var, descr) #define VM_STATS_SYS(var, descr) VM_STATS(_vm_stats_sys, var, descr)