Unify mbuf alignment functions to reduce exposure of mbuf internals outside of core mbuf code

[rwatson]

Summary

To ease changes to underlying mbuf structure and the mbuf allocator, reduce the knowledge of mbuf layout, and in particular constants such as M_EXT, MLEN, MHLEN, and so on, in mbuf consumers by unifying various alignment utility functions (M_ALIGN(), MH_ALIGN(), MEXT_ALIGN() in a single M_ALIGN() macro, implemented by a now-inlined m_align() function:

• Move m_align() from uipc_mbuf.c to mbuf.h; mark as __inline.
• Reimplement M_ALIGN(), MH_ALIGN(), and MEXT_ALIGN() using m_align().
• Update consumers around the tree to simply use M_ALIGN().

This change eliminates a number of cases where mbuf consumers must be aware of whether or not mbufs returned by the allocator use external storage, but also assumptions about the size of the returned mbuf. This will make it easier to introduce changes in how we use external storage, as well as features such as variable-size mbufs.

[glebius]

The change is definitely positive and brings more order to mbuf KPI. I have two suggestions for you to consider. In either case I accept the revision.

1. The overall direction I try to follow with mbuf KPI is to eliminate macros in favor of functions, no matter are they inlined or not. I already swept the tree for the allocation macros. Since this change already touches all users of alignment macros, I'd suggest to eliminate the macros at all and use m_align() everywhere.

2. I am not sure that inlining small functions is still a performance win on modern hadware. Jumps to static addresses are much cheaper than they used to be 20 years ago. At the same time all hot code fitting the instruction cache becomes critical, and inlining blows the code volume. Also it makes debugging more difficult. Of course this note is speculative, and I didn't do any measurements.

[rwatson]

Yes, this is one aspect of the change that I felt a bit conflicted on. I ended up shifting m_align() to being inlined to maintain the status quo that previous callers of M_ALIGN() and friends experienced -- but do agree with the general observation that making it a true function might be preferable (although slightly disturbing the status quo for a few pieces of code, such as IPSEC, which would now experience inlining but hadn't before). Another argument for making it a function would be to limit exposure of MHLEN and friends via the KBI, not just the KPI, as is currently the case.

Thanks for the review!

[trasz]

I like the idea of removing those macros, from both the code touched here and from mbuf.9. It might be a good idea to do it in a separate step, so that the first diff (this one here) can be MFC-ed, and the one removing the macros - not, to preserve API in stable.

[bz]

Minor suggestions but otherwise fine.

sys/netinet/sctp_os_bsd.h

307 ↗

(On Diff #2989)

Make sure Michael knows about this and syncs it to his sources.
Not sure how he's tracking these things, but probably fine for head.

sys/sys/mbuf.h

848 ↗

(On Diff #2989)

How much sense does it make in this context to still keep the const char *msg around with only 1 KASSERT. I'd rather fold that into two lines and get rid of the extra #ifdef.

858 ↗

(On Diff #2989)

I kept wondering if that sizeof(long) has been the right thing to do in the last 15 years (apart from by accident) ;-)

[rwatson]

I've maintained the current INVARIANTS code in m_align() for the copy -- I suspect the original code in m_align() was from the M_ALIGN() macro, meaning that func was from the caller function rather than naming m_align() itself. When it was made into a function, presumably that behaviour was broken. I'll commit it as is and then do a tidy in a followup commit.

[rwatson]

Closed by commit rS276692 (authored by @rwatson).