Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped - Build Status
Buildable 68098 Build 64981: arc lint + arc unit
Event Timeline
| share/man/man4/gzero.4 | ||
|---|---|---|
| 1–6 ↗ | (On Diff #162724) | (per style(9), although, I don't know how the "all rights reserved" line works) |
| 15–33 ↗ | (On Diff #162724) | Loading modules from loader.conf is discouraged unless necessary. mdoc(7) says we list the configuration in SYNOPSIS. If we list the configuration, then the kldload is implied because its exactly the same. Also implied are myriad other ways we don't list here, like the sysrc. We should stop putting prose in SYNOPSIS. Even the maintainer of the language says it does not belong there. |
| share/man/man4/gzero.4 | ||
|---|---|---|
| 15–33 ↗ | (On Diff #162724) | Where is this advice? It's wrong. Loading from loader.conf is preferred, unless there's a reason not to (drm, sadly, is one that is broken). The loader used to be slow, but is no longer. If it's in the config, then you can't load or unload it. It's in the kernel. You would EITHER load it or you'd have it in the kernel. |
| share/man/man4/gzero.4 | ||
|---|---|---|
| 15–33 ↗ | (On Diff #162724) |
I don't know that it's written anywhere, it's what I was told. Thats good to know, thanks. Do you have a preference for loading modules with rc.conf vs loader.conf?
Should we say "Or in ___.conf(5)" instead of "In ___.conf(5)"? |
| share/man/man4/gzero.4 | ||
|---|---|---|
| 1–6 ↗ | (On Diff #162724) | I'll take care of it, thanks! The All rights reserved was added there by the author and I'm not sure I'm legally allowed to remove it based on what I've seen so far in other instances. |
- Fix SPDX location
- Reference bio(9)
- Explain the clear sysctl now that I have a better understanding of it
- Add some notes from the original commit introducing ZERO
| share/man/man4/geom_zero.4 | ||
|---|---|---|
| 16–34 | ||
| share/man/man4/geom_zero.4 | ||
|---|---|---|
| 7 | ||
| share/man/man4/geom_zero.4 | ||
|---|---|---|
| 16–34 | Hmm, actually, how about the following: .Sh SYNOPSIS
.Ss Kernel Configuration
.Cd "options GEOM_ZERO"
.Ss Loader
geom_zero="YES"
.Ss rc.conf
.Cd kld_list="${kld_list} geom_zero"
.Ss Sysctl MIBs
.Bl -tag -compact
.It Va kern.geom.zero.byte
.It Va kern.geom.zero.clear
.El | |
- Add BUGS section
- Fix style of the license comment
- Wordsmith the description section
- Condense SYNOPSIS
| share/man/man4/geom_zero.4 | ||
|---|---|---|
| 16–34 | That's much further in the right direction, but Ingo (mdoc/mandoc maintainer) said subsections do not belong in synopsis when we proposed doing this. I tried to link my talk about this at BSDcan on YouTube but phabricator doesn't allow it. | |
| share/man/man4/geom_zero.4 | ||
|---|---|---|
| 16–34 | But then the mdoc manual even mentions that Nm is causing indentation in SYNOPSIS until Nm, Ss, and Sh. Which suggests that Ss acutally makes sense. TBH, the first version I uploaded was actually consistent with many other manuals in man4/. I think the Ss way could be our future style because it is both expressive and compact. | |
| share/man/man4/geom_zero.4 | ||
|---|---|---|
| 16–34 | I think the Ss style wastes empty lines, disallows cross-references, and was already declared wrong by upstream.
Jeez bro, respectfully, you could look at the 5 minute presentation I already gave on the topic after thinking deeply about this and talking to a great many people before making declarations about our future style. The style I proposed to you was in use in the project before you or I came here. Above all I want our team to just do something consistent for the quality of our thing we are doing together. That is the most important thing. If you don't like what I have come up with then please help come up with something better in good faith, i.e. with consideration what has been said already on the topic. It does not serve the project to try to improve consistency by doing something upstream has already publicly declared as wrong. | |
I don't think the addition of a man page for an obscure component warrants a relnote entry? gzero is really only of interest to developers, and a small minority of them at that.
| share/man/man4/geom_zero.4 | ||
|---|---|---|
| 53 | It's really only useful for benchmarking GEOM and GEOM classes (which may include cryptographic transformations, yes). When you write to gzero, the I/O requests don't reach any disks. "subsystem" is too vague. | |
| 86 | ||
| 164 | ... only if kern.geom.zero.clear==0. | |
| 165 | Not quite, it'll cause the kernel to fall back to mapped I/O, and throughput will be significantly lower than it would be otherwise. Said another way, this module is basically useless for benchmarking unless kern.geom.zero.clear==0. | |
Address markj's feebback:
- Write about GEOM and GEOM classes instead of disks and subsystems
- s/copying/memory initialization/
- Remove the BUGS section. I've finished implementing unmapped IO for gzero(4), so there is no need to mention the bugs here as they will be gone soon (wip: https://reviews.freebsd.org/D52998). Also, the recommendation to set clear to "0" is already mentioned in the description of the sysctl.
| share/man/man4/geom_zero.4 | ||
|---|---|---|
| 164 | Hmmm, I'm not so sure about that. Currently, here's what we have: if (g_zero_clear) gpp->flags &= ~G_PF_ACCEPT_UNMAPPED; else gpp->flags |= G_PF_ACCEPT_UNMAPPED; gzero(4) will accept unmapped I/O if kern.geom.zero.clear is 0, which means that GEOM will not map the bio data buffer. If kern.geom.zero.clear is 1, then G_PF_ACCEPT_UNMAPPED will be removed from gpp->flags and as a result GEOM will map the bio data. Anyhow, I'm removing the BUGS section altogether as it is a bit confusing and will need to be removed anyway when https://reviews.freebsd.org/D52998 lands. | |
| 165 | Thanks! I've decided to remove this section as https://reviews.freebsd.org/D52998 will land soon. Also, the clear==0 recommendation si already mentioned in the sysctl's description. | |