Re: svn commit: r236380 - head/sys/vm
On 1 June 2012 08:42, Eitan Adler ead...@freebsd.org wrote:
Author: eadler
Date: Fri Jun 1 04:42:52 2012
New Revision: 236380
URL: http://svn.freebsd.org/changeset/base/236380
Log:
Add sysctl to query amount of swap space free
PR: kern/166780
Submitted by: Radim Kolar h...@sendmail.cz
Approved by: cperciva
MFC after: 1 week
Well, we already have more powerful vm.swap_info, so
I see no reason to add yet another one to do the same thing
(but now with a human interface).
Probably sysctl(8) should be enhanced to parse it instead.
Modified:
head/sys/vm/swap_pager.c
Modified: head/sys/vm/swap_pager.c
==
--- head/sys/vm/swap_pager.c Fri Jun 1 04:34:49 2012 (r236379)
+++ head/sys/vm/swap_pager.c Fri Jun 1 04:42:52 2012 (r236380)
@@ -2692,3 +2692,18 @@ swaponvp(struct thread *td, struct vnode
NODEV);
return (0);
}
+
+static int
+sysctl_vm_swap_free(SYSCTL_HANDLER_ARGS) {
+ int swap_free, used;
+ int total;
+
+ swap_pager_status(total, used);
+
+ swap_free = (total - used) * PAGE_SIZE;
+ return SYSCTL_OUT(req, swap_free, sizeof(swap_free));
+}
+
+SYSCTL_OID(_vm, OID_AUTO, swap_free, CTLTYPE_INT|CTLFLAG_RD|CTLFLAG_MPSAFE,
+ NULL, 0, sysctl_vm_swap_free, Q,
+ Blocks of free swap storage.);
--
wbr,
pluknet
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r236380 - head/sys/vm
On 01/06/2012, at 15:38, Sergey Kandaurov wrote: Well, we already have more powerful vm.swap_info, so I see no reason to add yet another one to do the same thing (but now with a human interface). Probably sysctl(8) should be enhanced to parse it instead. There are already sysctls which have duplicate information, eg kern.geom.conf* (text, XML dot versions of the same data) -- Daniel O'Connor software and network engineer for Genesis Software - http://www.gsoft.com.au The nice thing about standards is that there are so many of them to choose from. -- Andrew Tanenbaum GPG Fingerprint - 5596 B766 97C0 0E94 4347 295E E593 DC20 7B3F CE8C ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r236380 - head/sys/vm
On Fri, Jun 01, 2012 at 04:42:52AM +, Eitan Adler wrote:
Author: eadler
Date: Fri Jun 1 04:42:52 2012
New Revision: 236380
URL: http://svn.freebsd.org/changeset/base/236380
Log:
Add sysctl to query amount of swap space free
PR: kern/166780
Submitted by: Radim Kolar h...@sendmail.cz
Approved by:cperciva
MFC after: 1 week
Modified:
head/sys/vm/swap_pager.c
The commit messages lack any rationale for the change.
The rationale specified in the PR is wrong, there _is_ the sysctl
interface to read the swap use, vm.swap_info.N. It is used by e.g.
swapinfo(8)/libkvm(3) on live system.
Modified: head/sys/vm/swap_pager.c
==
--- head/sys/vm/swap_pager.c Fri Jun 1 04:34:49 2012(r236379)
+++ head/sys/vm/swap_pager.c Fri Jun 1 04:42:52 2012(r236380)
@@ -2692,3 +2692,18 @@ swaponvp(struct thread *td, struct vnode
NODEV);
return (0);
}
+
+static int
+sysctl_vm_swap_free(SYSCTL_HANDLER_ARGS) {
+ int swap_free, used;
+ int total;
+
+ swap_pager_status(total, used);
+
+ swap_free = (total - used) * PAGE_SIZE;
+ return SYSCTL_OUT(req, swap_free, sizeof(swap_free));
+}
This just overflows at swap sizes greater then 2GB.
+
+SYSCTL_OID(_vm, OID_AUTO, swap_free, CTLTYPE_INT|CTLFLAG_RD|CTLFLAG_MPSAFE,
+ NULL, 0, sysctl_vm_swap_free, Q,
+ Blocks of free swap storage.);
Please revert the commit.
pgptg3nKDADR4.pgp
Description: PGP signature
Re: svn commit: r236380 - head/sys/vm
On Fri, 1 Jun 2012, Sergey Kandaurov wrote:
On 1 June 2012 08:42, Eitan Adler ead...@freebsd.org wrote:
...
Log:
?Add sysctl to query amount of swap space free
?PR: ? ? ? ? ? kern/166780
?Submitted by: Radim Kolar h...@sendmail.cz
?Approved by: ?cperciva
?MFC after: ? ?1 week
Well, we already have more powerful vm.swap_info, so
I see no reason to add yet another one to do the same thing
(but now with a human interface).
The new interface provides many more bugs. Mostly style bugs, but
also type mismatches and potential overflow.
Probably sysctl(8) should be enhanced to parse it instead.
That would be another bug. sysctl(8) already does too much parsing.
Parsing belongs in specialized utilities, and I think there are already
some that do it for swap. sysctl(8) largest existing exessive parsing
and presenting is for related vmtotal things.
Modified:
?head/sys/vm/swap_pager.c
Modified: head/sys/vm/swap_pager.c
==
--- head/sys/vm/swap_pager.c ? ?Fri Jun ?1 04:34:49 2012 ? ? ? ?(r236379)
+++ head/sys/vm/swap_pager.c ? ?Fri Jun ?1 04:42:52 2012 ? ? ? ?(r236380)
@@ -2692,3 +2692,18 @@ swaponvp(struct thread *td, struct vnode
? ? ? ? ? ?NODEV);
? ? ? ?return (0);
?}
Please don't put binary characters in mail.
+
+static int
+sysctl_vm_swap_free(SYSCTL_HANDLER_ARGS) {
First style bug: misplaced brace.
+ ? ? ? int swap_free, used;
+ ? ? ? int total;
Second and third style bugs: int variables not altogther, and not sorted.
But these are probably actually type and overflow errors (bugs 4 and 5)...
+
+ ? ? ? swap_pager_status(total, used);
+
Bug 6 is a style bug (extra blank line).
+ ? ? ? swap_free = (total - used) * PAGE_SIZE;
We multiply by PAGE_SIZE. This can probably overflow at 2G. Then
assigning to the int variable overflows at the same point. This gives
bugs 4 and 5. Related sysctls for memory sizes (see kern_mib.c) avoid
this problem by using u_long instead of int. But for disk sizes, using
u_long only reduces the problem to overflow at 4G, since systems with
32-bit longs can have larger disks than memory, and large disks can
have large swap.
I'm not sure if old restrictions on swap size have been fixed so that
more than 2G can actually be allocated, but most places that muliply
by PAGE_SIZE aee now careful to use expressions like
'(vm_ooffset_t)nblks * PAGE_SIZE' and to assign the result to a variable
of type vm_ooffset_t. swap_total is one such variable. But its sysctl
has type errors too. vm_ooffset_t is just not a supported type in sysctl.
SYSCTL_QUAD() is used for it. Quads shouldn't exist, and SYSCTL_QUAD()
should never be used, especially for non-quads. There are now some
support for 64-bit types in sysctl. Using these would be less bogus.
Bug 7 is a style bug: the related sysctls for memory sizes use ctob()
instead of hard-coding PAGE_SIZE. Avoiding this style bug also avoids
multiplication overflow (else you need a cast to go above 2G starting
with an int page count). ctob() is bogus too (seen any clicks lately?).
+ ? ? ? return SYSCTL_OUT(req, swap_free, sizeof(swap_free));
Bug 8 is a style bug (no spaces around return value).
+}
+
+SYSCTL_OID(_vm, OID_AUTO, swap_free, CTLTYPE_INT|CTLFLAG_RD|CTLFLAG_MPSAFE,
+ ? ? ? ? ? ? ? NULL, 0, sysctl_vm_swap_free, Q,
+ ? ? ? ? ? ? ? Blocks of free swap storage.);
Bug 9 is a style bug. I didn't even know that the raw SYSCTL_OID() could
be misused like this. The normal SYSCTL_PROC() is identical with
SYSCTL_OID() except it checks that the access flags are not 0. Few or no
SYSCTL_FOO()s have no access flags, and this is not one. It has rather
excessive access flags (I think CTLFLAG_MPSAFE is unnecessary. It is not
used for the related memory sysctls). vm has 4 existing SYSCTL_OID()s;
kern has 3; ia64/ia64 has 1; i386/i386 has 1; netipsec has 1. These are
the only matches for ^SYSCTL_OID in /sys, and they all seem to be just
style bugs.
Bug 10 is a collection of style bugs (missing spaces around binary operator
'|').
Bug 11 is a collection of style bugs (weird 2-tab continuation indentation
instead of the normal 4 spaces. 5 out of 7 existing SYSCTL_*()s in this
file including the vm_swap ones use normal continuation indentation.
Bug 12 is the most serious type error. The format is Q, but only
an int is returned. I don't see how this can result in anything except
garbage printing in sysctl(8). The access flag gives the type correctly
as int, but sysctl(8) mostly uses the format string for output. Oops,
that was in an old version. sysctl(8) now mostly uses the access flag,
and has no literal Q's in it any more. So this error might not be
serious, depending on whether the bad format string is actually used.
The sysctl data doesn't give the size of type type, but leaves it as
0. This works because the size is given as sizeof(swap_free) in the
call to SYSCTL_OUT(). This can be confusing, and use of the raw
Re: svn commit: r236380 - head/sys/vm
On 1 June 2012 05:14, Bruce Evans b...@optusnet.com.au wrote: On 1 June 2012 08:42, Eitan Adler ead...@freebsd.org wrote: ... I want to ack the replies to this commit. I'll either try to fix the bugs mentioned here, or more likely revert the commit. Note that it may take a few days for me to get to this though. -- Eitan Adler Source Ports committer X11, Bugbusting teams ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r236380 - head/sys/vm
On Fri, Jun 1, 2012 at 2:14 AM, Bruce Evans b...@optusnet.com.au wrote: +SYSCTL_OID(_vm, OID_AUTO, swap_free, CTLTYPE_INT|CTLFLAG_RD|CTLFLAG_MPSAFE, + NULL, 0, sysctl_vm_swap_free, Q, + Blocks of free swap storage.); Bug 9 is a style bug. I didn't even know that the raw SYSCTL_OID() could be misused like this. The normal SYSCTL_PROC() is identical with SYSCTL_OID() except it checks that the access flags are not 0. Few or no SYSCTL_FOO()s have no access flags, and this is not one. It has rather excessive access flags (I think CTLFLAG_MPSAFE is unnecessary. I wanted to correct this one point. CTLFLAG_MPSAFE is helpful, because its use prevents kern_sysctl from taking Giant before calling the sysctl handler. It's probably nearing the case, now, that any sysctl *without* CTLFLAG_MPSAFE is incorrect, except perhaps for a few that set/get text strings that don't want to roll their own serialization. Cheers, matthew ___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
Re: svn commit: r236380 - head/sys/vm
On Fri, 1 Jun 2012 m...@freebsd.org wrote: On Fri, Jun 1, 2012 at 2:14 AM, Bruce Evans b...@optusnet.com.au wrote: +SYSCTL_OID(_vm, OID_AUTO, swap_free, CTLTYPE_INT|CTLFLAG_RD|CTLFLAG_MPSAFE, + ? ? ? ? ? ? ? NULL, 0, sysctl_vm_swap_free, Q, + ? ? ? ? ? ? ? Blocks of free swap storage.); Bug 9 is a style bug. ?I didn't even know that the raw SYSCTL_OID() could be misused like this. ?The normal SYSCTL_PROC() is identical with SYSCTL_OID() except it checks that the access flags are not 0. ?Few or no SYSCTL_FOO()s have no access flags, and this is not one. ?It has rather excessive access flags (I think CTLFLAG_MPSAFE is unnecessary. I wanted to correct this one point. CTLFLAG_MPSAFE is helpful, because its use prevents kern_sysctl from taking Giant before calling the sysctl handler. It's probably nearing the case, now, that any sysctl *without* CTLFLAG_MPSAFE is incorrect, except perhaps for a few that set/get text strings that don't want to roll their own serialization. The magic is that SYSCTL_FOO() adds CTFLAG_MPSAFE for most or all simple integer SYSCTL_FOO()s like SYSCTL_INT(), but it doesn't do this for any non-integer SYSCTL_FOO(). Not for SYSCTL_PROC(), and especially not for the raw SYSCTL_OID(). There must be a lot of SYSCTL_PROC()s that don't bother with this, although many return an integer after calculating it. Perhaps the calculation isn't properly locked, but Giant will rarely help and no locking helps much for read-only sysctls of dynamic data (the value may change before it is returned). In kern, there are 113 lines matching SYSCTL_PROC, and only 4 of these match CTLFLAG_MPSAFE. Bruce___ svn-src-head@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
svn commit: r236380 - head/sys/vm
Author: eadler
Date: Fri Jun 1 04:42:52 2012
New Revision: 236380
URL: http://svn.freebsd.org/changeset/base/236380
Log:
Add sysctl to query amount of swap space free
PR: kern/166780
Submitted by: Radim Kolar h...@sendmail.cz
Approved by: cperciva
MFC after:1 week
Modified:
head/sys/vm/swap_pager.c
Modified: head/sys/vm/swap_pager.c
==
--- head/sys/vm/swap_pager.cFri Jun 1 04:34:49 2012(r236379)
+++ head/sys/vm/swap_pager.cFri Jun 1 04:42:52 2012(r236380)
@@ -2692,3 +2692,18 @@ swaponvp(struct thread *td, struct vnode
NODEV);
return (0);
}
+
+static int
+sysctl_vm_swap_free(SYSCTL_HANDLER_ARGS) {
+ int swap_free, used;
+ int total;
+
+ swap_pager_status(total, used);
+
+ swap_free = (total - used) * PAGE_SIZE;
+ return SYSCTL_OUT(req, swap_free, sizeof(swap_free));
+}
+
+SYSCTL_OID(_vm, OID_AUTO, swap_free, CTLTYPE_INT|CTLFLAG_RD|CTLFLAG_MPSAFE,
+ NULL, 0, sysctl_vm_swap_free, Q,
+ Blocks of free swap storage.);
___
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to svn-src-head-unsubscr...@freebsd.org
