Page MenuHomeFreeBSD

Inline struct m_hdr in struct mbuf, change mbuf array lengths to [0], add assertions
ClosedPublic

Authored by rwatson on Jan 10 2015, 4:06 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 27, 12:19 PM
Unknown Object (File)
Oct 23 2024, 7:36 AM
Unknown Object (File)
Oct 3 2024, 2:58 PM
Unknown Object (File)
Sep 14 2024, 11:43 PM
Unknown Object (File)
Sep 8 2024, 11:59 AM
Unknown Object (File)
Sep 8 2024, 7:47 AM
Unknown Object (File)
Sep 8 2024, 1:10 AM
Unknown Object (File)
Sep 7 2024, 10:37 AM
Subscribers

Details

Summary

In order to reduce namespace collisions and make it easier to reason about
and extend mbuf data-structure layout:

  • Change the definitions of byte arrays embedded in mbufs to be of size [0] rather than [MLEN] or [MHLEN], as we anticipate embedding mbuf headers within variable-size regions of memory in the future. In fact, we already do so within the cxgbe driver, but would now like the global mbuf allocator to support this as well.
  • Fold 'struct m_hdr' into 'struct mbuf' itself, eliminating a set of macros that aliased mh_foo field names to m_foo names such as 'm_next'. These present a particular problem as we would like to add new mbuf-header fields -- e.g., m_size -- that, if similarly named via macros, would introduce collisions with many other variable names in the kernel.
  • Rename 'struct m_ext' to 'struct struct_m_ext' so that we can introduce compile-time assertions without bumping into the still-extant 'm_ext' macro.
  • Remove the MSIZE compile-time assertion for 'struct mbuf', but add new assertions for alignment of embedded data arrays (64-bit alignment even on 32-bit platforms), and for the sizes the mbuf header, packet header, and m_ext structure. Document that these assertions exist in comments in mbuf.h.

This change should not cause actual behavioural differences, but is a
precursour to other mbuf-allocator work such as the introduction of
variable-size mbufs.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

rwatson retitled this revision from to Inline struct m_hdr in struct mbuf, change mbuf array lengths to [0], add assertions.
rwatson updated this object.
rwatson edited the test plan for this revision. (Show Details)
rwatson added a reviewer: network.
rwatson set the repository for this revision to rS FreeBSD src repository - subversion.
rpaulo added inline comments.
sys/sys/mbuf.h
165 ↗(On Diff #3091)

Hopefully this is not used outside our tree.

213 ↗(On Diff #3091)

Do you expect this to cause no regressions with the current drivers/network stack? I'm asking because, to find all the users of these structure members, one has to account for the special macros as well.

sys/sys/mbuf.h
165 ↗(On Diff #3091)

It's quite hard to use 'struct m_ext' in any way at all outside of struct mbuf, as 'm_ext' is defined as a macro a little further down the header. This is actually why I had to rename the structure: I couldn't do a CTASSERT() on sizeof(struct m_ext) due to macro expansion. I think we're safe in this regard -- but if not, we'll limit this change to FreeBSD 11 and simply declare it to be part of the revised KPI.

213 ↗(On Diff #3091)

The main thing I'd like to be able to grep for is sizeof(struct mbuf), but unfortunately there aren't really any good tools (that I'm aware of) to do this. It turns out there are half a dozen or more typedefs for mbuf around the tree (e.g., in ipfilter), for example. The main concern is device drivers that do 'special' things with mbufs -- in particular, cxgbe, which allocates its own -- so I've specifically asked np@ if he could review and test these changes. In principle, the changes actually make it easier to do what he is doing -- but confirming we haven't broken the current cxgbe implementation is another matter. In the end, the changes I'm making are changes to both the KPI and KBI -- I'm just hopefully that we can disrupt existing driver code as little as possible, as doing that avoids work.

bz added a reviewer: bz.

Given it's only comment or style changes or suggestions, I am generally good with this, would like to see some of the changes made however;-)

I have not worried about this could break consumers left and right given all the magic all these device drivers (ab)use.

sys/kern/uipc_mbuf.c
100 ↗(On Diff #3091)

I kept wondering if % 8 or % sizeof(uint64_t) or even another type might be better?

121 ↗(On Diff #3091)

Whenever I see these things I almost want
https://svnweb.freebsd.org/base?limit_changes=0&view=revision&revision=189221
back as a more general solution; is there anything in a modern C/compiler world that would simplify this?

sys/sys/mbuf.h
71 ↗(On Diff #3091)

I find the two extra comments confusing; the ones on top seem adequate.

110 ↗(On Diff #3091)

Remove the blank line given you refer to "these" values.

161 ↗(On Diff #3091)

dit(t)o

187 ↗(On Diff #3091)

And again (given the c&p).

213 ↗(On Diff #3091)

So the final decision was to use [] and not [0] above and below?

This revision is now accepted and ready to land.Jan 13 2015, 2:58 PM
gnn added a reviewer: gnn.
gnn added a subscriber: gnn.

Let's do this BEFORE we integrate the idea of using the list macros for the mbufs.

np added a reviewer: np.

I will produce a minor patch update and upload it shortly.

sys/kern/uipc_mbuf.c
100 ↗(On Diff #3091)

I ponder that as well, but concluded that for the other CTASSERT()s, literal numbers were preferred, and did that here for consistency. If you feel strongly (or someone else does), I can change to uint64_t; I'm not particularly moved one way or the other.

sys/sys/mbuf.h
71 ↗(On Diff #3091)

Will remove in the next patch rev.

213 ↗(On Diff #3091)

Navdeep has correctly pointed out that it should be [] rather than [0], and will be in the next patch rev.

sys/sys/mbuf.h
213 ↗(On Diff #3091)

It turns out that, on closer inspection of C11, using [] for the last element of a union is not permitted, so we are stuck with [0].

rwatson edited edge metadata.

Cosmetic whitespace changes as requested by bz. In the end I did not change '[0]' to '[]', as it turns out that the former cannot be used in combination with unions.

This revision now requires review to proceed.Jan 14 2015, 10:23 PM
bz edited edge metadata.
This revision is now accepted and ready to land.Jan 14 2015, 10:27 PM
rwatson updated this revision to Diff 3185.

Closed by commit rS277203 (authored by @rwatson).