ARM network trouble after recent mbuf changes
Andre Oppermann
andre at freebsd.org
Tue Aug 27 11:05:41 UTC 2013
On 27.08.2013 11:38, Michael Tuexen wrote:
> On Aug 27, 2013, at 8:53 AM, Andre Oppermann <andre at 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
-------------- next part --------------
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,
More information about the freebsd-arm
mailing list