On Mon, May 08, 2017 at 13:38 +0200, Mark Kettenis wrote:
> So the reason mikeb@'s mbuf changes caused issues is that the way we
> define struct mbuf is inherently fragile because it doesn't take
> structure padding into account. Adding an int64_t member to struct
> pkthdr changed the alignment from 4 bytes to 8 bytes on most 32-bit
> architectures (but not i386). Since struct m_hdr is only 20 bytes
> this means the compiler has to add padding between struct m_hdr and
> union that includes struct pkthdr in struct mbuf. But this padding is
> not included in the calculation of MLEN so struct mbuf gets too big.
>
> FreeBSD solved the issue by adding explicit padding when !__LP64__.
> But that's rather fragile. The padding would have to be reconsidered
> whenever the defenition of the mbuf header changes.
>
> NetBSD solved this by using offsetoff() instead of sizeof() in its
> definition of MLEN. But that requires the definition of a dummy
> structure through a macro that isn't exactly pretty. But it should be
> fairly robust.
>
> The diff below takes a different approach. It marks struct m_hdr as
> 8-byte aligned. That has the effect of padding out the struct to be a
> multiple of 8 bytes (because otherways the elements of arrays of
> struct m_hdr wouldn't be properly aligned). This makes us rely on
> compiler extensions (C99 doesn't include the possibility to specify
> alignment), but all compilers we care of provide such an extension and
> it is nicely abstracted in <sys/cdefs.h>. That somewhat matters since
> <sys/mbuf.h> exposes the struct mbuf definition to userland. This
> approach should be robus (until somebody decides they absolutely need
> a 128-bit type in their packet header).
>
The userland exposure is basically limited to libkvm. I don't see
it as a problem.
> Fun detail; gcc on armv7 crashes if I use __aligned(sizeof(int64_t))
> instead of __aligned(8). The armv7 kernel boots fine with this diff,
> but I'm defenitely doing a full build before I even consider
> committing this.
>
> Thoughts?
>
If this solves the issue for these archictures and there are no
objections from the crowd, I'm OK with the diff. I don't have
an issue with __aligned on structs.
We can also add a CTASSERT directly into the mbuf.h.
>
> Index: sys/mbuf.h
> ===================================================================
> RCS file: /cvs/src/sys/sys/mbuf.h,v
> retrieving revision 1.226
> diff -u -p -r1.226 mbuf.h
> --- sys/mbuf.h 7 May 2017 17:53:30 -0000 1.226
> +++ sys/mbuf.h 8 May 2017 11:11:09 -0000
> @@ -86,7 +86,7 @@ struct m_hdr {
> u_int mh_len; /* amount of data in this mbuf */
> short mh_type; /* type of data in this mbuf */
> u_short mh_flags; /* flags; see below */
> -};
> +} __aligned(8);
>
> /* pf stuff */
> struct pf_state_key;
> @@ -122,6 +122,7 @@ struct pkthdr_pf {
> struct pkthdr {
> void *ph_cookie; /* additional data */
> SLIST_HEAD(, m_tag) ph_tags; /* list of packet tags */
> + int64_t ph_timestamp; /* packet timestamp */
> int len; /* total packet length */
> u_int16_t ph_tagsset; /* mtags attached */
> u_int16_t ph_flowid; /* pseudo unique flow id */
