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/linprocf
On Tue, 2 May 2017, Konstantin Belousov wrote:
On Wed, May 03, 2017 at 01:31:10AM +1000, Bruce Evans wrote:
On Tue, 2 May 2017, Konstantin Belousov wrote:
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)?
Things like various counters for pages freed due to a reason can. E.g.
freed due to the process exit is the counter which I saw changing fast.
4 billion page operations/second or 10 is impossible. It is difficult
to even increment a register to count events that fast.
Wire counts might fluctuate relatively quickly, but I think that wiring
is slower. Unwiring might be fast.
The need to zero pages before reuse limits the speed.
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?
I do not see why vmstat counters ever need to be reset. I do not think
that truncating the value to present small values to 32bit readers is
a reasonable cause.
It would be mostly for presenting a consistent set of values.
diff --git a/sys/vm/vm_meter.c b/sys/vm/vm_meter.c
index 5f4cd46ab1e..b4666a400b2 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 val;
+#ifdef COMPAT_FREEBSD11
+ uint32_t val32;
+#endif
+
+ val = counter_u64_fetch(*(counter_u64_t *)arg1);
+#ifdef COMPAT_FREEBSD11
+ if (req->oldlen == sizeof(val32)) {
+ val32 = val;/* truncate */
+ return (SYSCTL_OUT(req, &val32, sizeof(val32)));
+ }
+#endif
+ return (SYSCTL_OUT(req, &val, sizeof(val)));
+}
+
#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)
OK.
Bruce
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
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/linprocf
On Wed, May 03, 2017 at 01:31:10AM +1000, Bruce Evans wrote:
>
>
> On Tue, 2 May 2017, Konstantin Belousov wrote:
> 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)?
Things like various counters for pages freed due to a reason can. E.g.
freed due to the process exit is the counter which I saw changing fast.
Wire counts might fluctuate relatively quickly, but I think that wiring
is slower. Unwiring might be fast.
> 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*.
Ok.
> 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?
I do not see why vmstat counters ever need to be reset. I do not think
that truncating the value to present small values to 32bit readers is
a reasonable cause.
diff --git a/sys/vm/vm_meter.c b/sys/vm/vm_meter.c
index 5f4cd46ab1e..b4666a400b2 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 val;
+#ifdef COMPAT_FREEBSD11
+ uint32_t val32;
+#endif
+
+ val = counter_u64_fetch(*(counter_u64_t *)arg1);
+#ifdef COMPAT_FREEBSD11
+ if (req->oldlen == sizeof(val32)) {
+ val32 = val;/* truncate */
+ return (SYSCTL_OUT(req, &val32, sizeof(val32)));
+ }
+#endif
+ return (SYSCTL_OUT(req, &val, sizeof(val)));
+}
+
#defineVM_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);
#defineVM_STATS_VM(var, descr) VM_STATS(_vm_stats_vm, var,
descr)
#defineVM_STATS_SYS(var, descr)VM_STATS(_vm_stats_sys, var,
descr)
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
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/linprocf
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
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
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/linprocf
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.
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 */
+ return (SYSCTL_OUT(req, &out32, sizeof(out32)));
+ }
+#endif
+ return (SYSCTL_OUT(req, &out, sizeof(out)));
+}
+
#defineVM_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);
#defineVM_STATS_VM(var, descr) VM_STATS(_vm_stats_vm, var,
descr)
#defineVM_STATS_SYS(var, descr)VM_STATS(_vm_stats_sys, var,
descr)
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
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/linprocf
On Tue, 2 May 2017, Konstantin Belousov wrote:
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 doesn't work for that either. Truncating either just works using
differences like it has done for 30 years or so, or gives weirder values
that clamping.
Arguably, the API is that the counters may wrap, and applications must
handle this as well as possible by sampling them more often than they
wrap, so that only the initial values might be wrong. Applications should
also have attempt to guess when the initial values are wrong, and print
hints about this that are more visible than printing blindly wrapped
value.
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.
Then you have to return no error, but truncate the value instead of
clamping. Anything else is incompatible.
To find the buggy applications, it is better to return a new errno, not
ENOMEM. EOVERFLOW resulted in top aborting with a useful error message.
...
+ 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.
sizeof is really bloated when you handle all the sizes as in my larger
patch. The basic sizes are 1, 2, 4 and 8 bytes (and possibly a larger
intmax_t), and it is a detail that the sizes in the integer types are
in bits. However, once we have set up the variables to match the sizes
in bytes, it is not so bad to use sizeof(var) instead of sizeof(typename).
The size is then hard-coded in only 1 place. That works here too:
use sizeof(out32).
+ out1 = 0x;
+ if (out < out1)
+
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/linprocf
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 20
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/linprocf
On Tue, 2 May 2017, Konstantin Belousov wrote: On Tue, May 02, 2017 at 12:02:32PM +0200, Hans Petter Selasky wrote: On 05/02/17 11:55, Konstantin Belousov wrote: + out1 = 0x; Nitpicking: Should this be written: 0xU ?? No, 0xU should be written 0x except in very delicate code which needs it to have type precisely u_int (and u_int has at least 32 bits, else the constant would still not be u_int). Compiler must do it on its own. The constant is not representable as int so it is auto-promoted to unsigned int, if fitting. It doesn't really matter here, but we must make too many assumptions and do too many type analyses to see that it doesn't matter (or too see that either 0x or 0xU is correct or needed, but plain 0x usually works better since it lets the compiler decide). Here you assume <= 32-bit ints for simplicity (otherwise, 0x would be int). POSIX and practice also requires >= 32-bit ints. So ints are assumed to be precisely 32 bits, and then 0x is just a better spelling of 0xU. Here we only need that the constant is non-negative. Its type is irrelevant, so it would be a style bug to force the type to unsigned or larger (with < 32-bit ints) using a U suffix. All integer constants are non-negative. We could even spell the constant in decimal here. It would then have type int64_t. Spelling it in hex makes its value clearer and allows its type to be unsigned. Bruce ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
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/linprocf
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.
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.
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.)
+
+ 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.
+ out1 = 0x;
+ if (out < out1)
+ out1 = out;
This does the clamping. Clamping is probably better expressed as:
out1 = MIN(out, 0x)
+ 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
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/linprocf
On Tue, May 02, 2017 at 12:02:32PM +0200, Hans Petter Selasky wrote: > On 05/02/17 11:55, Konstantin Belousov wrote: > > + out1 = 0x; > > Nitpicking: > > Should this be written: 0xU ?? Compiler must do it on its own. The constant is not representable as int so it is auto-promoted to unsigned int, if fitting. ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
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/linprocf
On 05/02/17 11:55, Konstantin Belousov wrote: + out1 = 0x; Nitpicking: Should this be written: 0xU ?? --HPS ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
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/linprocf
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 {
> XXuint64_t out;
> XX + uint32_t out32;
> XXint error;
> XX
> XXout = 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
> XXif (error || !req->newptr)
> XXreturn (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.
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
+
+ out = counter_u64_fetch(*(counter_u64_t *)arg1);
+#ifdef COMPAT_FREEBSD11
+ if (req->oldlen == sizeof(uint32_t)) {
+ out1 = 0x;
+ if (out < out1)
+ out1 = out;
+ return (SYSCTL_OUT(req, &out1, sizeof(uint32_t)));
+ }
+#endif
+ return (SYSCTL_OUT(req, &out, sizeof(uint64_t)));
+}
+
#defineVM_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);
#defineVM_STATS_VM(var, descr) VM_STATS(_vm_stats_vm, var,
descr)
#defineVM_STATS_SYS(var, descr)VM_STATS(_vm_stats_sys, var,
descr)
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
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/linprocf
On Sun, 30 Apr 2017, Konstantin Belousov wrote:
On Wed, Apr 19, 2017 at 02:09:58PM +1000, Bruce Evans wrote:
On Tue, 18 Apr 2017, Alan Somers wrote:
On Mon, Apr 17, 2017 at 11:34 AM, Gleb Smirnoff wrote:
head/usr.bin/top/machine.c
head/usr.bin/vmstat/vmstat.c
The previous 2 lines turn out to be relevant. I missed the update to
vmstat and checked a wrong version in my bug report described below
I checked an updated top, but the change seemed too small to help.
systat was not updated.
This change broke backwards compatibility with old top binaries. When
I use a kernel at version 317094 but a top from 14-April, I get the
error "top: sysctl(vm.stats.vm.v_swappgsin...) failed: Cannot allocate
memory". I get the same error when running top from an 11.0-RELEASE
jail. Can you please add backward compatibility shims?
I sent a much longer (30 times longer) bug report to the author of the
bug.
Most or all statistics utilities that use vmmeter are broken. vmstat
is too broken to even notice the bug -- it silently ignores the error,
an has type errors which prevent other errors which it doesn't ignore.
The result is silently truncating to 32 bits, like a quick fix would
do intentionally. systat -v detects the errors but prints warning
messages to the status line where they overwrite each other so make
the problem look smaller than it is. The result is unsilently
truncating to 32 bits.
I've never seen any backwards compatibility shims added for sysctls.
None should be needed, but there are few or no utilities other than
sysctl(8) which use sysctl(3) in a portable way.
See vfs.bufspace handled by vfs_bio.c:sysctl_bufspace().
Ugh.
My fix for this continues to work fine. It handles about 162
combinations of integer sizes/signedness for all sysctls in not much
more space than sysctl_bufspace(). Unfortunately, the full version
breaks the API slightly (it even breaks sysctl(8)), and the dumbed
down version causes problems although it technically doesn't change
the API.
sysctl_bufspace() also changes the API slightly, much like my dumbed
down version. This tends to break probes.
In fact, not only top and vmstat are broken. Quite unexpected and
worrying, reboot(8) is broken as well. This is due to get_pageins()
failing and system stopping userspace much earlier than it is desirable.
I now believe that compat shims to handle int-sized vmstat queries are
required.
Here are my current versions and a test program (mainly for old design
and implementation bugs with short reads and writes).
XX Index: kern_sysctl.c
XX ===
XX --- kern_sysctl.c(revision 317604)
XX +++ kern_sysctl.c(working copy)
This version changes the API too much, and I now use the simpler patch
to fix the immediate problem with counters.
The full version of it does:
- let userland do reads and writes of integers with any size large enough
to hold the value
- new errno EOVERFLOW for userland sizes that are not large enough. This
replaces ENOMEM in most cases for integer sysctls. Only oldlen = 0
should mean to probe.
- new errno EOVERFLOW for kernel sizes that are not large enough.Be
more careful not to write anything if the kernel size is not large
enough. The previous errno for this is EINVAL.
- it is no longer very fragile to read with a type larger than the kernel
type. The kernel expands to the requested size. This used to be a
non-error with garbage in the bits after the end of the unexpanded
kernel size.
- it is now possible to write with a type smaller than the kernel type.
The kernel expands to its size. The case is documented to fail with
EINVAL, and works as documented IIRC.
- it is now possible to write correctly with a type larger than the
kernel type, provided the value fits. The case is documented to fail
with EINVAL, but IIRC it is the case that has been broken for almost
20 years. Usually, the bug is harmless since the value fits (due to
are 0's or 1's in the unwritten top bits).
The dumbed down version keeps ENOMEM instead of EOVERFLOW, and doesn't
allow all combinations of sizes. For the counter ABI change, the case
where the userland variable is smaller must be supported for reading,
and the opposite is not needed yet. I forget what happens for writing.
XX @@ -1344,6 +1344,302 @@
XX return (error);
XX }
XX
XX +int sysctl_uicvt(const void *src, size_t srclen, void *dst, size_t dstlen);
XX +int
XX +sysctl_uicvt(const void *src, size_t srclen, void *dst, size_t dstlen)
XX +{
XX +uintmax_t val, valtrunc;
XX +uint64_t val64;
XX +uint32_t val32;
XX +uint16_t val16;
XX +uint8_t val8;
XX +
XX +if (dstlen > sizeof(uintmax_t))
XX +dstlen = sizeof(uintmax_t);
XX +if (dstlen > srclen)
XX +dstlen = srclen;
The conversion functions are trivial, but not as small or as fast as
they could be. In most case, no conversion is needed.
These dstlen a
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/linprocf
On Wed, Apr 19, 2017 at 02:09:58PM +1000, Bruce Evans wrote: > On Tue, 18 Apr 2017, Alan Somers wrote: > > > On Mon, Apr 17, 2017 at 11:34 AM, Gleb Smirnoff wrote: > > >> head/usr.bin/top/machine.c > >> head/usr.bin/vmstat/vmstat.c > > The previous 2 lines turn out to be relevant. I missed the update to > vmstat and checked a wrong version in my bug report described below > I checked an updated top, but the change seemed too small to help. > systat was not updated. > > > This change broke backwards compatibility with old top binaries. When > > I use a kernel at version 317094 but a top from 14-April, I get the > > error "top: sysctl(vm.stats.vm.v_swappgsin...) failed: Cannot allocate > > memory". I get the same error when running top from an 11.0-RELEASE > > jail. Can you please add backward compatibility shims? > > I sent a much longer (30 times longer) bug report to the author of the > bug. > > Most or all statistics utilities that use vmmeter are broken. vmstat > is too broken to even notice the bug -- it silently ignores the error, > an has type errors which prevent other errors which it doesn't ignore. > The result is silently truncating to 32 bits, like a quick fix would > do intentionally. systat -v detects the errors but prints warning > messages to the status line where they overwrite each other so make > the problem look smaller than it is. The result is unsilently > truncating to 32 bits. > > I've never seen any backwards compatibility shims added for sysctls. > None should be needed, but there are few or no utilities other than > sysctl(8) which use sysctl(3) in a portable way. See vfs.bufspace handled by vfs_bio.c:sysctl_bufspace(). In fact, not only top and vmstat are broken. Quite unexpected and worrying, reboot(8) is broken as well. This is due to get_pageins() failing and system stopping userspace much earlier than it is desirable. I now believe that compat shims to handle int-sized vmstat queries are required. ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
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/linprocf
On Mon, Apr 17, 2017 at 7:34 PM, Gleb Smirnoff wrote: > Author: glebius > Date: Mon Apr 17 17:34:47 2017 > New Revision: 317061 > URL: https://svnweb.freebsd.org/changeset/base/317061 > > Log: > - Remove 'struct vmmeter' from 'struct pcpu', leaving only global vmmeter > in place. To do per-cpu stats, convert all fields that previously were > maintained in the vmmeters that sit in pcpus to counter(9). > - Since some vmmeter stats may be touched at very early stages of boot, > before we have set up UMA and we can do counter_u64_alloc(), provide an > early counter mechanism: > o Leave one spare uint64_t in struct pcpu, named pc_early_dummy_counter. > o Point counter(9) fields of vmmeter to pcpu[0].pc_early_dummy_counter, > so that at early stages of boot, before counters are allocated we > already > point to a counter that can be safely written to. > o For sparc64 that required a whole dummy pcpu[MAXCPU] array. Hi, This seems to break a few ports, the most important ones: http://gohan2.ysv.freebsd.org/data/head-amd64-default-baseline/p438755_s317076/logs/errors/net-snmp-5.7.3_15.log http://gohan2.ysv.freebsd.org/data/head-amd64-default-baseline/p438755_s317076/logs/errors/sigar-1.7.3_2.log Cheers, Antoine ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
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/linprocf
On Tue, 18 Apr 2017, Alan Somers wrote:
On Mon, Apr 17, 2017 at 11:34 AM, Gleb Smirnoff wrote:
Please trim quotes.
[... 5 lines of quotes trimmed]
Log:
- Remove 'struct vmmeter' from 'struct pcpu', leaving only global vmmeter
in place. To do per-cpu stats, convert all fields that previously were
maintained in the vmmeters that sit in pcpus to counter(9).
[... 21 lines of quotes trimmed]
[... more than 50 lines of quotes trimmed]
head/usr.bin/top/machine.c
head/usr.bin/vmstat/vmstat.c
The previous 2 lines turn out to be relevant. I missed the update to
vmstat and checked a wrong version in my bug report described below
I checked an updated top, but the change seemed too small to help.
systat was not updated.
This change broke backwards compatibility with old top binaries. When
I use a kernel at version 317094 but a top from 14-April, I get the
error "top: sysctl(vm.stats.vm.v_swappgsin...) failed: Cannot allocate
memory". I get the same error when running top from an 11.0-RELEASE
jail. Can you please add backward compatibility shims?
I sent a much longer (30 times longer) bug report to the author of the
bug.
Most or all statistics utilities that use vmmeter are broken. vmstat
is too broken to even notice the bug -- it silently ignores the error,
an has type errors which prevent other errors which it doesn't ignore.
The result is silently truncating to 32 bits, like a quick fix would
do intentionally. systat -v detects the errors but prints warning
messages to the status line where they overwrite each other so make
the problem look smaller than it is. The result is unsilently
truncating to 32 bits.
I've never seen any backwards compatibility shims added for sysctls.
None should be needed, but there are few or no utilities other than
sysctl(8) which use sysctl(3) in a portable way. Utilities tend to
hard-code types and thus break when the types are expanded or shrunk.
Especially broken ones like vmstat even ignore the error for their
size being too small. There is no error for the size being too small,
so callers must check if the size returned is the size supplied.
vmstat is too broken to check this too. systat -v does check.
Note that using the types in the system header is a form of hard-coding.
This gives binaries that only work when run on the same system as the
headers. Using the "any" type is best done using a smart wrapper or
perhaps a smarter sysctl(3), and many utilities already have dumb
wrappers. The one in vmstat() shows how not to do it:
X static int
X mysysctl(const char *name, void *oldp, size_t *oldlenp,
X void *newp, size_t newlen)
X {
X int error;
X
X error = sysctlbyname(name, oldp, oldlenp, newp, newlen);
X if (error != 0 && errno != ENOMEM)
X xo_err(1, "sysctl(%s)", name);
X return (error);
X }
ENOMEM here means that our variable is too small. It is not easy to expand
correctly, but it is easy to check if it is small enough to cause a problem.
Something like:
// Zero-extend in case our variable is larger than the
// system's. We only support unsigned variables.
bzero(oldp, *oldlenp);
// Don't use our garbage new* variables. All calls are
// read-only since we are a stats utility, and this wrapper
// doesn't support writing.
if (sysctlbyname(name, oldp, oldlenp, NULL, 0) == 0)
return (0)
u_char buf[640 * 1024]; // should be enough for anyint
size_t buflen;
size_t off;
buflen = sizeof(buf);
error = sysctlbyname(name, buf, buflen, NULL, 0);
if (error != 0)
xo_err(1, "sysctl(%s)", name);
for (off = *oldlenp; buf < bufsize; off++) {
// Check for zero-extension.
// This version only supports unsigned types.
if (buf[off] != 0) {
errno = EOVERFLOW;
#ifdef CALLERS_PROBLEM
// sysctl(3) should always have done this
// too. That is, return EOVERFLOW instead
// of ENOMEM when the result is too large
// to fit, and don't return an error if
// it does fit.
return (-1);
#else
// Now a useful error message.
xo_err(1, "sysctl(%s)", name);
#endif
}
}
Then expand sizes in the caller if necessary. u_int would work in most
cases, and so would u_char for small and boolean variables. CALLERS_PROBLEM
allows use of small types when only differences are needed, as for some
cases in vmstat. uin
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/linprocf
On 04/18/2017 16:05, Larry Rosenman wrote: > On 4/18/17, 2:58 PM, "Alan Somers" of asom...@freebsd.org> wrote: > > On Mon, Apr 17, 2017 at 11:34 AM, Gleb Smirnoff > wrote: > > Author: glebius > > Date: Mon Apr 17 17:34:47 2017 > > New Revision: 317061 > > URL: https://svnweb.freebsd.org/changeset/base/317061 > > > > Log: > > - Remove 'struct vmmeter' from 'struct pcpu', leaving only global > vmmeter > > in place. To do per-cpu stats, convert all fields that previously > were > > maintained in the vmmeters that sit in pcpus to counter(9). > > - Since some vmmeter stats may be touched at very early stages of > boot, > > before we have set up UMA and we can do counter_u64_alloc(), > provide an > > early counter mechanism: > > o Leave one spare uint64_t in struct pcpu, named > pc_early_dummy_counter. > > o Point counter(9) fields of vmmeter to > pcpu[0].pc_early_dummy_counter, > > so that at early stages of boot, before counters are allocated we > already > > point to a counter that can be safely written to. > > o For sparc64 that required a whole dummy pcpu[MAXCPU] array. > > > > Further related changes: > > - Don't include vmmeter.h into pcpu.h. > > - vm.stats.vm.v_swappgsout and vm.stats.vm.v_swappgsin changed to > 64-bit, > > to match kernel representation. > > - struct vmmeter hidden under _KERNEL, and only vmstat(1) is an > exclusion. > > > > This is based on benno@'s 4-year old patch: > > https://lists.freebsd.org/pipermail/freebsd-arch/2013-July/014471.html > > > > Reviewed by: kib, gallatin, marius, lidl > > Differential Revision:https://reviews.freebsd.org/D10156 > > > This change broke backwards compatibility with old top binaries. When > I use a kernel at version 317094 but a top from 14-April, I get the > error "top: sysctl(vm.stats.vm.v_swappgsin...) failed: Cannot allocate > memory". I get the same error when running top from an 11.0-RELEASE > jail. Can you please add backward compatibility shims? > > -Alan > It also broke emulators/virtualbox-ose-kmod True but it is not a big deal. A patch will be committed with VirtualBox 5.1.20 soon. Jung-uk Kim signature.asc Description: OpenPGP digital signature
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/linprocf
On 4/18/17, 2:58 PM, "Alan Somers" wrote: On Mon, Apr 17, 2017 at 11:34 AM, Gleb Smirnoff wrote: > Author: glebius > Date: Mon Apr 17 17:34:47 2017 > New Revision: 317061 > URL: https://svnweb.freebsd.org/changeset/base/317061 > > Log: > - Remove 'struct vmmeter' from 'struct pcpu', leaving only global vmmeter > in place. To do per-cpu stats, convert all fields that previously were > maintained in the vmmeters that sit in pcpus to counter(9). > - Since some vmmeter stats may be touched at very early stages of boot, > before we have set up UMA and we can do counter_u64_alloc(), provide an > early counter mechanism: > o Leave one spare uint64_t in struct pcpu, named pc_early_dummy_counter. > o Point counter(9) fields of vmmeter to pcpu[0].pc_early_dummy_counter, > so that at early stages of boot, before counters are allocated we already > point to a counter that can be safely written to. > o For sparc64 that required a whole dummy pcpu[MAXCPU] array. > > Further related changes: > - Don't include vmmeter.h into pcpu.h. > - vm.stats.vm.v_swappgsout and vm.stats.vm.v_swappgsin changed to 64-bit, > to match kernel representation. > - struct vmmeter hidden under _KERNEL, and only vmstat(1) is an exclusion. > > This is based on benno@'s 4-year old patch: > https://lists.freebsd.org/pipermail/freebsd-arch/2013-July/014471.html > > Reviewed by: kib, gallatin, marius, lidl > Differential Revision:https://reviews.freebsd.org/D10156 > This change broke backwards compatibility with old top binaries. When I use a kernel at version 317094 but a top from 14-April, I get the error "top: sysctl(vm.stats.vm.v_swappgsin...) failed: Cannot allocate memory". I get the same error when running top from an 11.0-RELEASE jail. Can you please add backward compatibility shims? -Alan It also broke emulators/virtualbox-ose-kmod ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
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/linprocf
On Mon, Apr 17, 2017 at 11:34 AM, Gleb Smirnoff wrote: > Author: glebius > Date: Mon Apr 17 17:34:47 2017 > New Revision: 317061 > URL: https://svnweb.freebsd.org/changeset/base/317061 > > Log: > - Remove 'struct vmmeter' from 'struct pcpu', leaving only global vmmeter > in place. To do per-cpu stats, convert all fields that previously were > maintained in the vmmeters that sit in pcpus to counter(9). > - Since some vmmeter stats may be touched at very early stages of boot, > before we have set up UMA and we can do counter_u64_alloc(), provide an > early counter mechanism: > o Leave one spare uint64_t in struct pcpu, named pc_early_dummy_counter. > o Point counter(9) fields of vmmeter to pcpu[0].pc_early_dummy_counter, > so that at early stages of boot, before counters are allocated we > already > point to a counter that can be safely written to. > o For sparc64 that required a whole dummy pcpu[MAXCPU] array. > > Further related changes: > - Don't include vmmeter.h into pcpu.h. > - vm.stats.vm.v_swappgsout and vm.stats.vm.v_swappgsin changed to 64-bit, > to match kernel representation. > - struct vmmeter hidden under _KERNEL, and only vmstat(1) is an exclusion. > > This is based on benno@'s 4-year old patch: > https://lists.freebsd.org/pipermail/freebsd-arch/2013-July/014471.html > > Reviewed by: kib, gallatin, marius, lidl > Differential Revision:https://reviews.freebsd.org/D10156 > > Modified: > head/libexec/rpc.rstatd/rstat_proc.c > head/sys/amd64/amd64/trap.c > head/sys/amd64/include/atomic.h > head/sys/amd64/include/counter.h > head/sys/amd64/include/pcpu.h > head/sys/arm/arm/intr.c > head/sys/arm/arm/trap-v4.c > head/sys/arm/arm/trap-v6.c > head/sys/arm/arm/undefined.c > head/sys/arm/include/counter.h > head/sys/arm/include/pcpu.h > head/sys/arm64/include/counter.h > head/sys/arm64/include/pcpu.h > head/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c > head/sys/compat/linprocfs/linprocfs.c > head/sys/fs/fuse/fuse_vnops.c > head/sys/fs/nfsclient/nfs_clbio.c > head/sys/fs/smbfs/smbfs_io.c > head/sys/i386/i386/trap.c > head/sys/i386/include/atomic.h > head/sys/i386/include/counter.h > head/sys/i386/include/pcpu.h > head/sys/kern/kern_fork.c > head/sys/kern/kern_intr.c > head/sys/kern/kern_synch.c > head/sys/kern/kern_thread.c > head/sys/kern/subr_intr.c > head/sys/kern/subr_syscall.c > head/sys/kern/subr_trap.c > head/sys/kern/vfs_bio.c > head/sys/mips/include/counter.h > head/sys/mips/include/intr_machdep.h > head/sys/mips/include/pcpu.h > head/sys/powerpc/include/counter.h > head/sys/powerpc/include/pcpu.h > head/sys/powerpc/powerpc/trap.c > head/sys/sparc64/include/counter.h > head/sys/sparc64/include/pcpu.h > head/sys/sparc64/sparc64/exception.S > head/sys/sparc64/sparc64/genassym.c > head/sys/sparc64/sparc64/intr_machdep.c > head/sys/sparc64/sparc64/machdep.c > head/sys/sparc64/sparc64/trap.c > head/sys/sys/pcpu.h > head/sys/sys/vmmeter.h > head/sys/vm/swap_pager.c > head/sys/vm/vm_fault.c > head/sys/vm/vm_meter.c > head/sys/vm/vm_object.c > head/sys/vm/vm_page.c > head/sys/vm/vm_pageout.c > head/sys/vm/vnode_pager.c > head/sys/x86/acpica/srat.c > head/sys/x86/x86/intr_machdep.c > head/usr.bin/top/machine.c > head/usr.bin/vmstat/vmstat.c This change broke backwards compatibility with old top binaries. When I use a kernel at version 317094 but a top from 14-April, I get the error "top: sysctl(vm.stats.vm.v_swappgsin...) failed: Cannot allocate memory". I get the same error when running top from an 11.0-RELEASE jail. Can you please add backward compatibility shims? -Alan ___ svn-src-all@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
