Date: Tue, 27 Aug 2013 13:05:35 +0200 From: Andre Oppermann <andre@freebsd.org> To: Michael Tuexen <tuexen@freebsd.org> Cc: freebsd-arm <freebsd-arm@FreeBSD.org> Subject: Re: ARM network trouble after recent mbuf changes Message-ID: <521C87FF.8010100@freebsd.org> In-Reply-To: <0E0536B2-2B7F-4EED-9EFD-4B9E2C2D729A@freebsd.org> References: <1377550636.1111.156.camel@revolution.hippie.lan> <521BC472.7040804@freebsd.org> <521BD531.4090104@sbcglobal.net> <521C4CD9.4050308@freebsd.org> <0E0536B2-2B7F-4EED-9EFD-4B9E2C2D729A@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a multi-part message in MIME format. --------------070207030006050305010600 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit On 27.08.2013 11:38, Michael Tuexen wrote: > On Aug 27, 2013, at 8:53 AM, Andre Oppermann <andre@FreeBSD.org> wrote: > >> On 27.08.2013 00:22, Thomas Skibo wrote: >>> On 8/26/13 2:11 PM, Andre Oppermann wrote: >>>> >>>> Can you try this patch see check if it makes a difference on the bitfield? >>> >>> Actually, this works for me. But, I'm worried that somewhere else something is going to trip over a >>> struct pkthdr not being 64-bit aligned. There are several 64-bit fields in there. >> >> The problem is the disconnect between the definition of MLEN and MHLEN and >> the effective size/padding of struct mbuf. That's the true bug. >> >> On LP64 all is fine. On i386 it turns out to be fine too because doesn't >> care. >> >> MLEN and MHLEN are incorrectly derived. In fact they should be derived from >> stuct mbuf where this padding would be taking into account. However the way >> it is structured right now it that would create a circular dependency. >> >> Please try the patch below to confirm. If it fixes your problem for now >> I'm going to commit as an immediate fix while searching for a better long >> term stable solution. >> >> -- >> Andre >> >> Index: sys/mbuf.h >> =================================================================== >> --- sys/mbuf.h (revision 254953) >> +++ sys/mbuf.h (working copy) >> @@ -94,6 +94,9 @@ >> 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(__ILP32__) >> + uint32_t mh_pad; /* pad to 64 bit alignment */ >> +#endif >> }; > > OK. It doesn't work. The reason is, that __ILP32__ is not defined... At > lease I don't see it anywhere in the BSD stack. So I'm currently rebuilding > with #if !defined(__LP64__) instead. I'll let you know... Thanks. I've changed the test accordingly. While doing the CTASSERTs to prevent such an incident in the future I stumbled across a bit of evil name space pollution in mbuf.h. It is impossible to take sizeof(struct m_ext) because "m_ext" is redefined to point into struct mbuf. In addition to the alignment fix I've solved the namespace issues with m_ext and the stupidly named struct pkthdr as well and properly prefixed them. The fallout from LINT was zero (as it should be). http://people.freebsd.org/~andre/m_hdr-alignment-20130827.diff Please test. -- Andre --------------070207030006050305010600 Content-Type: text/plain; charset=windows-1252; name="m_hdr-alignment-20130827.diff" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="m_hdr-alignment-20130827.diff" Index: sys/mbuf.h =================================================================== --- sys/mbuf.h (revision 254953) +++ sys/mbuf.h (working copy) @@ -53,12 +53,16 @@ * externally and attach it to the mbuf in a way similar to that of mbuf * clusters. * + * NB: These calculation do not take actual compiler-induced alignment and + * padding inside the complete struct mbuf into account. Appropriate attention + * is required when changing members of struct mbuf. + * * 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. */ -#define MLEN ((int)(MSIZE - sizeof(struct m_hdr))) -#define MHLEN ((int)(MLEN - sizeof(struct pkthdr))) +#define MLEN ((int)(MSIZE - sizeof(struct mh_hdr))) +#define MHLEN ((int)(MLEN - sizeof(struct mh_pkthdr))) #define MINCLSIZE (MHLEN + 1) #ifdef _KERNEL @@ -67,7 +71,7 @@ * type: * * mtod(m, t) -- Convert mbuf pointer to data pointer of correct type. - * mtodo(m, o) -- Same as above but with offset 'o' into data. + * mtodo(m, o) -- Same as above but with offset 'o' into data. */ #define mtod(m, t) ((t)((m)->m_data)) #define mtodo(m, o) ((void *)(((m)->m_data) + (o))) @@ -84,10 +88,10 @@ /* * Header present at the beginning of every mbuf. - * Size ILP32: 20 + * Size ILP32: 24 * LP64: 32 */ -struct m_hdr { +struct mh_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 */ @@ -94,10 +98,15 @@ 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). + * Size ILP32: 16 + * LP64: 24 */ struct m_tag { SLIST_ENTRY(m_tag) m_tag_link; /* List of packet tags */ @@ -112,7 +121,7 @@ * Size ILP32: 48 * LP64: 56 */ -struct pkthdr { +struct mh_pkthdr { struct ifnet *rcvif; /* rcv interface */ SLIST_HEAD(packet_tags, m_tag) tags; /* list of packet tags */ int32_t len; /* total packet length */ @@ -159,7 +168,7 @@ * Size ILP32: 28 * LP64: 48 */ -struct m_ext { +struct mh_ext { volatile u_int *ref_cnt; /* pointer to ref count info */ caddr_t ext_buf; /* start of buffer */ uint32_t ext_size; /* size of buffer, for ext_free */ @@ -176,18 +185,18 @@ * purposes. */ struct mbuf { - struct m_hdr m_hdr; + struct mh_hdr m_hdr; union { struct { - struct pkthdr MH_pkthdr; /* M_PKTHDR set */ + struct mh_pkthdr MH_pkthdr; /* M_PKTHDR set */ union { - struct m_ext MH_ext; /* M_EXT set */ + struct mh_ext MH_ext; /* M_EXT set */ char MH_databuf[MHLEN]; } MH_dat; } MH; char M_databuf[MLEN]; /* !M_PKTHDR, !M_EXT */ } M_dat; -}; +} __aligned(sizeof(uint64_t)); #define m_next m_hdr.mh_next #define m_len m_hdr.mh_len #define m_data m_hdr.mh_data Index: kern/uipc_mbuf.c =================================================================== --- kern/uipc_mbuf.c (revision 254953) +++ kern/uipc_mbuf.c (working copy) @@ -85,6 +85,17 @@ #endif /* + * 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(sizeof(((struct mbuf *)0)->m_hdr) == sizeof(struct mh_hdr)); +CTASSERT(sizeof(((struct mbuf *)0)->m_pkthdr) == sizeof(struct mh_pkthdr)); +CTASSERT(sizeof(((struct mbuf *)0)->m_ext) == sizeof(struct mh_ext)); +CTASSERT(sizeof(((struct mbuf *)0)->m_dat) == MLEN); +CTASSERT(sizeof(((struct mbuf *)0)->m_pktdat) == MHLEN); + +/* * m_get2() allocates minimum mbuf that would fit "size" argument. */ struct mbuf * @@ -389,7 +400,7 @@ if (m->m_flags & M_PKTHDR) { m_tag_delete_chain(m, NULL); m->m_flags &= ~M_PKTHDR; - bzero(&m->m_pkthdr, sizeof(struct pkthdr)); + bzero(&m->m_pkthdr, sizeof(struct mh_pkthdr)); } if (m != m0 && m->m_nextpkt != NULL) { KASSERT(m->m_nextpkt == NULL, --------------070207030006050305010600--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?521C87FF.8010100>