Fix new users of MAXPHYS and hide it from the kernel namespace
ClosedPublic
Actions

Authored by gallatin on Sun, Apr 28, 1:54 AM.

Details

Reviewers

kib
markj
imp
jhb
jkim
james_mclgh.net

Commits

rG13a5a46c49d0: Fix new users of MAXPHYS and hide it from the kernel namespace

Summary

In cd8537910406, @kib made maxphys a load-time tunable. The #define MAXPHYS in sys/param.h became almost entirely obsolete, as it could now be overridden by kern.maxphys at boot time, or by opt_maxphys.h. After this change, MAXPHYS really only had meaning in subr_param.c, where it is used to initialize maxphys.

However, decades of tradition have led to several new, incorrect, uses of MAXPHYS in other parts of the kernel, mostly by seasoned developers. I've corrected those uses here in a mechanical fashion, and verified that it fixes a bug in the md driver that I was experiencing.

Since using MAXPHYS is such an easy mistake to make, it is best to hide it from the kernel namespace. So I've moved its definition to subr_param.c. Note that lots of userspace programs use it for different reasons, most of them probably wrong. But that's outside the scope of this change.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

gallatin requested review of this revision.Sun, Apr 28, 1:54 AM

gallatin created this revision.

kib accepted this revision.Sun, Apr 28, 2:16 AM

kib added inline comments.

sys/kern/subr_param.c
85	To avoid copy/paste, you could move this fragment into a new header sys/_maxphys.h, then include it from subr_param.h and sys/param.h for !_KERNEL.

This revision is now accepted and ready to land.Sun, Apr 28, 2:16 AM

Please add Fixes: 30038a8b4efc ("md: Get rid of the pbuf zone") to the commit log message.

Update diff to avoid cutting/pasting MAXPHYS definition as per @kib's suggestion

This revision now requires review to proceed.Sun, Apr 28, 4:41 PM

imp accepted this revision.Sun, Apr 28, 6:04 PM

This revision is now accepted and ready to land.Sun, Apr 28, 6:04 PM

kib accepted this revision.Mon, Apr 29, 5:45 PM

kib added inline comments.

sys/sys/_maxphys.h
2	I suspect we do need a copyright boilerplate for the file, even if a single SPDX-line kind.

I consulted with @imp, and after a trip down the rabbit hole, we concluded that a header file consisting only of the definition of MAXPHYS is not creative (as this is the only way to express this in C) so it can't have copyright protection, and should simply be public domain.

This revision now requires review to proceed.Mon, Apr 29, 11:24 PM

kib accepted this revision.Mon, Apr 29, 11:54 PM

This revision is now accepted and ready to land.Mon, Apr 29, 11:54 PM

imp added inline comments.Tue, Apr 30, 1:49 AM

sys/sys/_maxphys.h
8	The one i propsed had shifts and no comments

gallatin added inline comments.Tue, Apr 30, 2:11 PM

sys/sys/_maxphys.h
8	Whoops, sorry. I missed that in our convo.. I'm happy to remove the comment, but I'd prefer to keep the #defines the same because: to me at least, the multiplications are more clear than the shifts I don't want to do another tinderbox run