Should we mark those as __read_mostly?

The maxphys var is mostly unused after the patch, its consumers are driver initialization code.

Also I am not a fan of jealous application of micro-optimizations without proof of importance.

The + 1 looks like it's in the wrong place, but I do not really understand why it's there to begin with.

Indeed misplaced, while needed to represent misaligned maxphys buffers.

This condition can be relaxed with extra page in pbuf.

If MAXPHYS is left here for some time for some driver compatibility, I think it would be right to include opt_maxphys.h here too. I was surprised how sbp(4) driver builds while still having MAXPHYS, remember that you've moved the option to separate header.

nitems(job->pages) is wrong as well.

It cannot be done this way because sys/param.h is also used by userspace.
In fact I wanted to remove MAXPHYS define at all, at least from common headers, but this is not going to work due to userspace.

For sbp(4), can you please point to the full file path and line where MAXPHYS is left ?

According to my sources I see two:
sys/dev/firewire/sbp.c:74
sys/dev/isci/scil/sci_controller_constants.h:160

Could we use #define MAXPHYS __get_maxphys() for userland? Or does this need to be a compile time constant for userland?

I see how I missed them, thanks.

No we cannot do a trick with __get_maxphys(), it should be constant. I do not see an issue with having it this way. If filesystems or drivers want to inform userspace about proper transfer sizes, there are already channels to communicate that, e.g. statfs(2).

OK. It might make sense, then, to see what we're using it for in userland. Most places a compile-time thing would be fine, but there's at least two places that use MAXPHYS to size an array... There's a number of places that use it to scale buffers for performance reasons, but others that use it to limit kernel requests.... Clearly beyond the scope of this work, though....

Would you also remove the above block, since now vmapbuf() should do the right thing?

I would prefer you to do it. Perhaps you have a test setup to exercise this code path, neither I nor pho do not.

Do you mean also in separate commit? OK.

Is there a minimum that needs to be enforced?

I am not sure, perhaps one page is absolutely required, I am not sure how far can we proceed with it.

I somewhat dislike even the checks I added, I do not see why users should be not allowed to experiment. The cost of mistake there is low, at worst machine won't boot. It is somewhat costly for a user not having access to console, but such user probably ought to not make experiments like that.

I think a basic check of PAGE_SIZE is a reasonable seat-belt. Otherwise some might think that 512b is a reasonable MAXPHYS, or even 0, both of which is think would result in a system unable to do any disk I/O. It's not like this would be checked on every I/O, it's just a boot-time seatbelt. I'm not going to object to it's absence, and I might add it later if it doesn't happen.

Are you should that reordering vm_page_unhold_pages() and pmap_qremove() is universally safe? Consider powerpc{,64}. The powerpc{,64} pmaps create PVO entries for the unmanaged mappings created by pmap_qenter(). Moreover, pmap_page_is_mapped() doesn't distinguish between PVO entries representing managed versus unmanaged mappings. So, if the page is freed by vm_page_unhold_pages(), can't the KASSERT(!pmap_page_is_mapped()) fail?

This is, in fact, a surprise for me. I thought that qenter() mappings are never accounted for as mappings for pmap_page_is_mapped().

The reordering was needed because job->pages is really pbuf->b_pages, and code freed pbuf before doing unwire().

D27409