Date: Sun, 11 Jan 2015 13:31:32 +0000 (GMT) From: Robert Watson <rwatson@FreeBSD.org> To: net@FreeBSD.org Subject: Inlining struct m_hdr with struct mbuf; assertion changes Message-ID: <alpine.BSF.2.11.1501111324170.36182@fledge.watson.org>
next in thread | raw e-mail | index | archive | help
Dear all: As you may have spotted from recent commits to 11-CURRENT, I've been working through the mbuf allocator and mbuf consumers (protocols, device drivers) trying to reduce consumer dependence on the specifics of the mbuf layout -- i.e., using M_SIZE() instead of caller assumptions about MLEN, MHLEN, and MCLBYTES, etc. This is part of a larger project to allow more flexibility in how we implement the mbuf allocator, with a particular interest in supporting 'variable-size mbufs' -- i.e., mbufs with sizes other than MSIZE. I've been running these patches past #network on reviews.FreeBSD.org, but wanted to post this one to the mailing list explicitly due to the change affecting the layout of struct mbuf itself. You can find the review instance here to see current reviews/discussion: https://reviews.freebsd.org/D1483 The attached patch implements the following change: 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. Update calculation of these constants to no longer depend on 'struct mbuf' being of size MSIZE. - 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. Robert Index: sys/kern/uipc_mbuf.c =================================================================== --- sys/kern/uipc_mbuf.c (revision 276911) +++ sys/kern/uipc_mbuf.c (working copy) @@ -88,11 +88,38 @@ * Ensure the correct size of various mbuf parameters. It could be off due * to compiler-induced padding and alignment artifacts. */ -CTASSERT(sizeof(struct mbuf) == MSIZE); CTASSERT(MSIZE - offsetof(struct mbuf, m_dat) == MLEN); CTASSERT(MSIZE - offsetof(struct mbuf, m_pktdat) == MHLEN); /* + * mbuf data storage should be 64-bit aligned regardless of architectural + * pointer size; check this is the case with and without a packet header. + */ +CTASSERT(offsetof(struct mbuf, m_dat) % 8 == 0); +CTASSERT(offsetof(struct mbuf, m_pktdat) % 8 == 0); + +/* + * While the specific values here don't matter too much (i.e., +/- a few + * words), we do want to ensure that changes to these values are carefully + * reasoned about and properly documented. This is especially the case as + * network-protocol and device-driver modules encode these layouts, and must + * be recompiled if the structures change. Check these values at compile time + * against the ones documented in comments in mbuf.h. + * + * NB: Possibly they should be documented there via #define's and not just + * comments. + */ +#if defined(__LP64__) +CTASSERT(offsetof(struct mbuf, m_dat) == 32); +CTASSERT(sizeof(struct pkthdr) == 56); +CTASSERT(sizeof(struct struct_m_ext) == 48); +#else +CTASSERT(offsetof(struct mbuf, m_dat) == 24); +CTASSERT(sizeof(struct pkthdr) == 48); +CTASSERT(sizeof(struct struct_m_ext) == 28); +#endif + +/* * m_get2() allocates minimum mbuf that would fit "size" argument. */ struct mbuf * Index: sys/sys/mbuf.h =================================================================== --- sys/sys/mbuf.h (revision 276911) +++ sys/sys/mbuf.h (working copy) @@ -60,9 +60,15 @@ * MLEN is data length in a normal mbuf. * MHLEN is data length in an mbuf with pktheader. * MINCLSIZE is a smallest amount of data that should be put into cluster. + * + * Compile-time assertions in uipc_mbuf.c test these values to ensure that + * they are sensible. */ -#define MLEN ((int)(MSIZE - sizeof(struct m_hdr))) -#define MHLEN ((int)(MLEN - sizeof(struct pkthdr))) +struct mbuf; +#define MHSIZE offsetof(struct mbuf, M_dat.M_databuf) +#define MPKTHSIZE offsetof(struct mbuf, M_dat.MH.MH_dat.MH_databuf) +#define MLEN ((int)(MSIZE - MHSIZE)) /* normal data len */ +#define MHLEN ((int)(MSIZE - MPKTHSIZE)) /* data len w/pkthdr */ #define MINCLSIZE (MHLEN + 1) #ifdef _KERNEL @@ -87,23 +93,6 @@ #endif /* _KERNEL */ /* - * Header present at the beginning of every mbuf. - * Size ILP32: 24 - * LP64: 32 - */ -struct m_hdr { - struct mbuf *mh_next; /* next buffer in chain */ - struct mbuf *mh_nextpkt; /* next chain in queue/record */ - caddr_t mh_data; /* location of data */ - int32_t mh_len; /* amount of data in this mbuf */ - uint32_t mh_type:8, /* type of data in this mbuf */ - mh_flags:24; /* flags; see below */ -#if !defined(__LP64__) - uint32_t mh_pad; /* pad for 64bit alignment */ -#endif -}; - -/* * Packet tag structure (see below for details). */ struct m_tag { @@ -118,6 +107,9 @@ * Record/packet header in first mbuf of chain; valid only if M_PKTHDR is set. * Size ILP32: 48 * LP64: 56 + * + * Compile-time assertions in uipc_mbuf.c test these values to ensure that + * they are correct. */ struct pkthdr { struct ifnet *rcvif; /* rcv interface */ @@ -166,8 +158,11 @@ * set. * Size ILP32: 28 * LP64: 48 + * + * Compile-time assertions in uipc_mbuf.c test these values to ensure that + * they are correct. */ -struct m_ext { +struct struct_m_ext { volatile u_int *ext_cnt; /* pointer to ref count info */ caddr_t ext_buf; /* start of buffer */ uint32_t ext_size; /* size of buffer, for ext_free */ @@ -184,24 +179,43 @@ * purposes. */ struct mbuf { - struct m_hdr m_hdr; + /* + * Header present at the beginning of every mbuf. + * + * Size ILP32: 24 + * LP64: 32 + * + * Compile-time assertions in uipc_mbuf.c test these values to ensure + * that they are correct. + */ + struct mbuf *m_next; /* next buffer in chain */ + struct mbuf *m_nextpkt; /* next chain in queue/record */ + caddr_t m_data; /* location of data */ + int32_t m_len; /* amount of data in this mbuf */ + uint32_t m_type:8, /* type of data in this mbuf */ + m_flags:24; /* flags; see below */ +#if !defined(__LP64__) + uint32_t m_pad; /* pad for 64bit alignment */ +#endif + + /* + * A set of optional headers (packet header, external storage header) + * and internal data storage. Historically, these arrays were sized + * to MHLEN (space left after a packet header) and MLEN (space left + * after only a regular mbuf header); they are now variable size in + * order to support future work on variable-size mbufs. + */ union { struct { struct pkthdr MH_pkthdr; /* M_PKTHDR set */ union { - struct m_ext MH_ext; /* M_EXT set */ - char MH_databuf[MHLEN]; + struct struct_m_ext MH_ext; /* M_EXT set */ + char MH_databuf[0]; } MH_dat; } MH; - char M_databuf[MLEN]; /* !M_PKTHDR, !M_EXT */ + char M_databuf[0]; /* !M_PKTHDR, !M_EXT */ } M_dat; }; -#define m_next m_hdr.mh_next -#define m_len m_hdr.mh_len -#define m_data m_hdr.mh_data -#define m_type m_hdr.mh_type -#define m_flags m_hdr.mh_flags -#define m_nextpkt m_hdr.mh_nextpkt #define m_pkthdr M_dat.MH.MH_pkthdr #define m_ext M_dat.MH.MH_dat.MH_ext #define m_pktdat M_dat.MH.MH_dat.MH_databuf
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?alpine.BSF.2.11.1501111324170.36182>