From owner-svn-src-all@freebsd.org Tue May 2 15:31:15 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 145BDD5A2EC; Tue, 2 May 2017 15:31:15 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id 9ECA6E22; Tue, 2 May 2017 15:31:14 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c122-106-153-191.carlnfd1.nsw.optusnet.com.au [122.106.153.191]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id 045131048B19; Wed, 3 May 2017 01:31:11 +1000 (AEST) Date: Wed, 3 May 2017 01:31:10 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Konstantin Belousov cc: Bruce Evans , 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... In-Reply-To: <20170502144823.GH1622@kib.kiev.ua> Message-ID: <20170503005322.K1968@besplex.bde.org> 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> <20170502121711.GE1622@kib.kiev.ua> <20170502223324.P1508@besplex.bde.org> <20170502144823.GH1622@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=VbSHBBh9 c=1 sm=1 tr=0 a=Tj3pCpwHnMupdyZSltBt7Q==:117 a=Tj3pCpwHnMupdyZSltBt7Q==:17 a=kj9zAlcOel0A:10 a=t0vv-wrxefp5Oj2dI0IA:9 a=LDhBBNh50KkugDdz:21 a=PFlbG7WVNzEJfU_R:21 a=CjuIK1q_8ugA:10 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.23 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: Tue, 02 May 2017 15:31:15 -0000 On Tue, 2 May 2017, Konstantin Belousov wrote: > On Tue, May 02, 2017 at 11:31:21PM +1000, Bruce Evans wrote: >> On Tue, 2 May 2017, Konstantin Belousov wrote: >>> ENOMEM is, of course, the situation which I want to avoid. >> >> Then you have to return no error, but truncate the value instead of >> clamping. Anything else is incompatible. > I do not quite agree with the truncation part, bit I do not think that > it is too important. As I noted before, IMO the absolute numbers for > the counters have more values than per-interval diffs displayed by e.g. > systat. But if truncating causes less disagreement than clamping, lets > do truncation. This is better. I also thought of changing the scale when the values get high. The values would increase slower above about 2G instead of stabilizing at 4G-1. This is basically floating point and too complicated since nothing would understand it. Which counters wrap faster than a reasonable refresh interval of 1-10 seconds (which should be shorter if wrapping is a problem)? > diff --git a/sys/vm/vm_meter.c b/sys/vm/vm_meter.c > index 5f4cd46ab1e..6266ef670a6 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(out32)) { > + out32 = (uint32_t)out; /* truncate */ Style: - redundant cast. Especially not needed with the commit. Compilers might warn about this since they don't trust programmers, but don't because implicit downwards conversions are used often. - comment not indented I would also omit the ifdefs, and rename 'out' to out64, and may rename 'out*' to val*. > + return (SYSCTL_OUT(req, &out32, sizeof(out32))); > + } > +#endif > + return (SYSCTL_OUT(req, &out, sizeof(out))); > +} > + > #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) I just noticed that this sysctl is r/o (I thought I was preserving support for resetting 64-bit counters using a 32-bit size in my fix in sysctl_handle_counter_64(). That function has the dubious feature of not checking the size, so it allows writes of any length (0 to SIZE_MAX, possibly larger than the user data) to reset the counter to zero.) The r/o misfeature goes back to at least FreeBSD-3. 64-bit counters need resetting less than 32-bit ones, and it is more useful to ever reset them since they can hold the full counts since boot time, but there is no reason to limit resetting them now that the low-level code supports it. Is there already a better atomic reset of all vm stats? Bruce