sys/kern/subr_vmem.c
612 ↗	(On Diff #79556)	Commit separately?
824 ↗	(On Diff #79556)	I would delete the `rehash` comment and merge the two blocks. All of the functions below can arguably be called "vmem internal functions".
831 ↗	(On Diff #79556)	This can be a separate commit.
861 ↗	(On Diff #79556)	This can be a separate commit.
1375 ↗	(On Diff #79556)	Why?
1471 ↗	(On Diff #79556)	I really dislike conditionally defined top-level local variables, so I'd prefer to move the definition into the `if` block below. I don't insist on it though.
1604 ↗	(On Diff #79556)	You could eliminate some inline ifdefs with `#define panic(...) assert(0)`.
1618 ↗	(On Diff #79556)	Same comment as above wrt local variables.
1633 ↗	(On Diff #79556)	This can be a separate commit.
1732 ↗	(On Diff #79556)	This block should probably be formally protected with ifdef _KERNEL as well.

This revision is now accepted and ready to land.Nov 16 2020, 4:41 PM

sys/kern/subr_vmem.c
1471 ↗	(On Diff #79556)	I agree in general, and if this were some special case (e.g. INVARIANTS) instead of the usual case I'd argue strongly for markj's take.

kib marked 7 inline comments as done.Nov 17 2020, 1:32 AM

kib added inline comments.

sys/kern/subr_vmem.c
1375 ↗	(On Diff #79556)	To not add __unused declaration, I think it is logically wrong.
1604 ↗	(On Diff #79556)	There were only two of them so I did not bothered.

This revision was automatically updated to reflect the committed changes.

kib marked 2 inline comments as done.

Rebase after commit of warning fixes.
Handle Mark notes.

markj added inline comments.

sys/contrib/openzfs/include/sys/zfs_context.h
53 ↗	(On Diff #79634)	Is this just a reminder to upstream the change? Or is there another problem?
sys/kern/subr_vmem.c
1375 ↗	(On Diff #79556)	I think `(void)qcache_max;` is more common for this scenario.