Date: Fri, 30 Nov 2012 09:53:41 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Robert Watson <rwatson@FreeBSD.org> Cc: freebsd-arch@FreeBSD.org Subject: Re: Print a (rate-limited) warning when UMA zone is full. Message-ID: <20121130084954.Q1018@besplex.bde.org> In-Reply-To: <alpine.BSF.2.00.1211291027430.59662@fledge.watson.org> References: <20121129090147.GB1370@garage.freebsd.pl> <alpine.BSF.2.00.1211291027430.59662@fledge.watson.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 29 Nov 2012, Robert Watson wrote: > On Thu, 29 Nov 2012, Pawel Jakub Dawidek wrote: > >> I'd like to propose the following patch: >> >> http://people.freebsd.org/~pjd/patches/uma_warning.patch Please include small (< 10000 line) patches in mail. They would have to be included in replies to review them. >> When UMA zone is created, one can add configure a warning that should be >> printed when UMA zone is full by calling: >> >> uma_zone_set_warning(socket_zone, >> "kern.ipc.maxsockets limit exceeded, please see tuning(7)."); Please don't clone messages of this form. It has bad grammar, bad style and a garbage pointer. Many of these were cloned from previous messages of this form. Bad grammar: 1. Redundant "limit". kern.ipc.maxsockets is already a limit. 2. Wrong object. The thing not being exceeded is the number of sockets, not the limit on the number of sockets. 3. Wrong verb. It is impossible to exceed an enforcible, enforced limit This seems hard to fix without making the message too verbose. A full description would say something to the effect that that the limit would be exeeded if exceeding it were possible and permitted. Or some conventional wording for this situation could be used. The above has conventional wording, but has too many errors for me. 4. comma splice Bad style: 5. pleading 6. termination with a ".'. Error messages are conventionally not terminated 7. general verboseness, with the help of (1), (5) and (7). > Just to follow up on some out-of-band communication -- this is a good idea, > but there was some concern about printf() under mutexes. I'm not actually > that concerned about that case (we do it quite a lot for warnings and kernel > debugging), but it might be useful to consider using log() instead, so that > it ends up in the system log as well as on the console. printf() works in the "any" context so it is less restricted than log(). But it is less controllable, and spams the console. > Finally, we should make sure that in all instances where we point at > tuning(7), it has something useful to say about the topic :-). This is the garbage pointer: 8. Pointer to nonexistent or too-generic info in tuning(7). Since you didn't include the patch, I couldn't see if it provided the info, but other replies in this thread complained about this too and said that it didn't. When messages of this form were originally implemented, many of them pointed to garbage. Now some of them are documented there. But I think this is a wrong place, except for generic info about how to change complicated interacting sysctls. Each sysctl should be described in a more specialized man page, and documenting sysctls in a general place like tuning gives duplication or inhibits the specialized documentation. Current messages of this form in freefall's kernel: % Consider tuning vm.kmem_size and vm.kmem_size_max Apparently part of an essay written in an error message. % kern.maxfiles limit exceeded by uid %i, please see tuning(7). Has all the above errors, plus %i instead of %d in the printf format (not just a style bug, but a type error. uid_t is not only a typedefed type that might differ from int; it does differ from int since it is unsigned. %i misprints valid uids that exceed INT_MAX. POSIX requires uids to be nonnegative integers, though IIRC it doesn't require uid_t to be unsigned or even integral in old versions. But in FreeBSD, uids can be anything representable by uid_t, so the printf format must match uid_t. tuning(7) actuall mentions this. % maxproc limit exceeded by uid %i, please see tuning(7) and login.conf(5). This describes the limit as "maxproc limit" instead of as kern.maxproc. This avoids bugs (1) and (2), but makes finding the info a little harder. It references another man page that provides very little relevant info. login.conf(5) has 1 line about maxproc and that line does nother except un-abbreviate it. This is better than tuning(7), which has no lines about maxproc or limits on it. % kern.ipc.maxpipekva exceeded; see tuning(7) This is missing bugs (1), (2), (4), (5) (6) and (7), because I complained strongly enough about it when it was committed. The original version was also missing ".ipc" in the sysctl name, so the pointer to tuning(7) was garbage in a different way. % Configure virtual memory overcommit behavior. See tuning(7) for details. This is the description for the vm.overcommit sysctl. It avoids errors (1)-(6) but not (7). Its fix for the comma splice is to use multiple sentences with a misformatted sentence break. With multiple sentences, the terminating '.' is arguably not a style bug. But verbose descriptions that need multiple sentences are. This is actually mentioned in tuning(7). References to man pages are especially unnecessary in sysctl descriptions and are mercifully absent for all the sysctls mentioned in rate-limited messages that mention tuning(7). It should go without saying that every sysctl description is expanded in the man page(s) about the sysctl, if the sysctl actually has a man page. Relevant man pages are most easily found using 'zgrep -irl /usr/share/man'. For that, it is useful to give the full sysctl name in all places and not paraphrase it like "maxproc limit". Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20121130084954.Q1018>