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...

[Bruce Evans]

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