From 5af0299e377b6c5bc89b7801600048fa515bd1b4 Mon Sep 17 00:00:00 2001 From: Augustin Cavalier Date: Fri, 6 Jul 2018 10:32:45 -0400 Subject: [PATCH] freebsd11_network: Implement m_ext reference-counting and M_NOFREE. Some FreeBSD developers report that this is required for HT mode, which might explain why it's so broken on Haiku. I was also told the iwm driver requires it for multi-frame RX, but as per the previous commit it still KDLs even with it. This commit also includes a refactor of the mbuf header implementation, which now more closely mirrors FreeBSD's. --- .../freebsd11_network/compat/sys/cdefs.h | 22 +++- .../freebsd11_network/compat/sys/mbuf-fbsd.h | 100 ++++++++++++------ .../freebsd11_network/compat/sys/mbuf.h | 44 ++++---- src/libs/compat/freebsd11_network/fbsd_mbuf.c | 45 ++++---- src/libs/compat/freebsd11_network/mbuf.c | 47 ++++++-- 5 files changed, 170 insertions(+), 88 deletions(-) diff --git a/src/libs/compat/freebsd11_network/compat/sys/cdefs.h b/src/libs/compat/freebsd11_network/compat/sys/cdefs.h index 6813d75d94..22db41d4c1 100644 --- a/src/libs/compat/freebsd11_network/compat/sys/cdefs.h +++ b/src/libs/compat/freebsd11_network/compat/sys/cdefs.h @@ -172,6 +172,22 @@ #define __rangeof(type, start, end) \ (__offsetof(type, end) - __offsetof(type, start)) +/* + * Given the pointer x to the member m of the struct s, return + * a pointer to the containing structure. When using GCC, we first + * assign pointer x to a local variable, to check that its type is + * compatible with member m. + */ +#if __GNUC_PREREQ__(3, 1) +#define __containerof(x, s, m) ({ \ + const volatile __typeof(((s *)0)->m) *__x = (x); \ + __DEQUALIFY(s *, (const volatile char *)__x - __offsetof(s, m));\ +}) +#else +#define __containerof(x, s, m) \ + __DEQUALIFY(s *, (const volatile char *)(x) - __offsetof(s, m)) +#endif + /* * Compiler-dependent macros to help declare dead (non-returning) and * pure (no side effects) functions, and unused variables. They are @@ -312,7 +328,11 @@ #endif #ifndef __DEVOLATILE -#define __DEVOLATILE(type, var) ((type)(__uintptr_t)(volatile void *)(var)) +#define __DEVOLATILE(type, var) ((type)(uintptr_t)(volatile void *)(var)) +#endif + +#ifndef __DEQUALIFY +#define __DEQUALIFY(type, var) ((type)(uintptr_t)(const volatile void *)(var)) #endif #if __GNUC_PREREQ__(4,6) && !defined(__cplusplus) diff --git a/src/libs/compat/freebsd11_network/compat/sys/mbuf-fbsd.h b/src/libs/compat/freebsd11_network/compat/sys/mbuf-fbsd.h index 59fa9d4062..6a42d40076 100644 --- a/src/libs/compat/freebsd11_network/compat/sys/mbuf-fbsd.h +++ b/src/libs/compat/freebsd11_network/compat/sys/mbuf-fbsd.h @@ -25,71 +25,92 @@ * LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY * OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF * SUCH DAMAGE. - * - * @(#)mbuf.h 8.5 (Berkeley) 2/19/95 - * $FreeBSD: src/sys/sys/mbuf.h,v 1.170.2.6 2006/03/23 23:24:32 sam Exp $ */ #ifndef _FBSD_COMPAT_SYS_MBUF_FBSD_H_ #define _FBSD_COMPAT_SYS_MBUF_FBSD_H_ /* - * Set the m_data pointer of a newly-allocated mbuf (m_get/MGET) to place - * an object of the specified size at the end of the mbuf, longword aligned. + * Return the address of the start of the buffer associated with an mbuf, + * handling external storage, packet-header mbufs, and regular data mbufs. */ -#define M_ALIGN(m, len) do { \ - (m)->m_data += (MLEN - (len)) & ~(sizeof(long) - 1); \ -} while (0) +#define M_START(m) \ + (((m)->m_flags & M_EXT) ? (m)->m_ext.ext_buf : \ + ((m)->m_flags & M_PKTHDR) ? &(m)->m_pktdat[0] : \ + &(m)->m_dat[0]) /* - * As above, for mbufs allocated with m_gethdr/MGETHDR - * or initialized by M_COPY_PKTHDR. + * Return the size of the buffer associated with an mbuf, handling external + * storage, packet-header mbufs, and regular data mbufs. */ -#define MH_ALIGN(m, len) do { \ - (m)->m_data += (MHLEN - (len)) & ~(sizeof(long) - 1); \ -} while (0) +#define M_SIZE(m) \ + (((m)->m_flags & M_EXT) ? (m)->m_ext.ext_size : \ + ((m)->m_flags & M_PKTHDR) ? MHLEN : \ + MLEN) /* -#define MEXT_IS_REF(m) (((m)->m_ext.ref_cnt != NULL) \ - && (*((m)->m_ext.ref_cnt) > 1)) + * Set the m_data pointer of a newly allocated mbuf to place an object of the + * specified size at the end of the mbuf, longword aligned. + * + * NB: Historically, we had M_ALIGN(), MH_ALIGN(), and MEXT_ALIGN() as + * separate macros, each asserting that it was called at the proper moment. + * This required callers to themselves test the storage type and call the + * right one. Rather than require callers to be aware of those layout + * decisions, we centralize here. */ -#define MEXT_IS_REF(m) 0 +static __inline void +m_align(struct mbuf *m, int len) +{ +#ifdef INVARIANTS + const char *msg = "%s: not a virgin mbuf"; +#endif + int adjust; + + KASSERT(m->m_data == M_START(m), (msg, __func__)); + + adjust = M_SIZE(m) - len; + m->m_data += adjust &~ (sizeof(long)-1); +} + +#define M_ALIGN(m, len) m_align(m, len) +#define MH_ALIGN(m, len) m_align(m, len) +#define MEXT_ALIGN(m, len) m_align(m, len) /* - * Evaluate TRUE if it's safe to write to the mbuf m's data region (this - * can be both the local data payload, or an external buffer area, - * depending on whether M_EXT is set). + * Evaluate TRUE if it's safe to write to the mbuf m's data region (this can + * be both the local data payload, or an external buffer area, depending on + * whether M_EXT is set). */ +#define M_WRITABLE(m) (!((m)->m_flags & M_RDONLY) && \ + (!(((m)->m_flags & M_EXT)) || \ + (m_extrefcnt(m) == 1))) /* -#define M_WRITABLE(m) (!((m)->m_flags & M_RDONLY) && (!((m)->m_flags \ - & M_EXT) || !MEXT_IS_REF(m))) - */ -#define M_WRITABLE(m) (!((m)->m_flags & M_EXT) || !MEXT_IS_REF(m)) - -/* - * Compute the amount of space available - * before the current start of data in an mbuf. + * Compute the amount of space available before the current start of data in + * an mbuf. * * The M_WRITABLE() is a temporary, conservative safety measure: the burden * of checking writability of the mbuf data area rests solely with the caller. + * + * NB: In previous versions, M_LEADINGSPACE() would only check M_WRITABLE() + * for mbufs with external storage. We now allow mbuf-embedded data to be + * read-only as well. */ #define M_LEADINGSPACE(m) \ - ((m)->m_flags & M_EXT ? \ - (M_WRITABLE(m) ? (m)->m_data - (m)->m_ext.ext_buf : 0): \ - (m)->m_flags & M_PKTHDR ? (m)->m_data - (m)->m_pktdat : \ - (m)->m_data - (m)->m_dat) + (M_WRITABLE(m) ? ((m)->m_data - M_START(m)) : 0) /* * Compute the amount of space available after the end of data in an mbuf. * * The M_WRITABLE() is a temporary, conservative safety measure: the burden * of checking writability of the mbuf data area rests solely with the caller. + * + * NB: In previous versions, M_TRAILINGSPACE() would only check M_WRITABLE() + * for mbufs with external storage. We now allow mbuf-embedded data to be + * read-only as well. */ #define M_TRAILINGSPACE(m) \ - ((m)->m_flags & M_EXT ? \ - (M_WRITABLE(m) ? (m)->m_ext.ext_buf + (m)->m_ext.ext_size \ - - ((m)->m_data + (m)->m_len) : 0) : \ - &(m)->m_dat[MLEN] - ((m)->m_data + (m)->m_len)) + (M_WRITABLE(m) ? \ + ((M_START(m) + M_SIZE(m)) - ((m)->m_data + (m)->m_len)) : 0) /* * Arrange to prepend space of size plen to mbuf m. @@ -123,6 +144,15 @@ m_clrprotoflags(struct mbuf *m) } } +static inline u_int +m_extrefcnt(struct mbuf *m) +{ + KASSERT(m->m_flags & M_EXT, ("%s: M_EXT missing", __func__)); + + return ((m->m_ext.ext_flags & EXT_FLAG_EMBREF) ? m->m_ext.ext_count : + *m->m_ext.ext_cnt); +} + /* mbufq */ struct mbufq { diff --git a/src/libs/compat/freebsd11_network/compat/sys/mbuf.h b/src/libs/compat/freebsd11_network/compat/sys/mbuf.h index eb767edfd9..aaeeb772ae 100644 --- a/src/libs/compat/freebsd11_network/compat/sys/mbuf.h +++ b/src/libs/compat/freebsd11_network/compat/sys/mbuf.h @@ -89,6 +89,15 @@ #define EXT_JUMBO9 5 // 9 * 1024 bytes #define EXT_NET_DRV 100 // custom ext_buf provided by net driver +#define EXT_EXTREF 255 // has externally maintained ext_cnt ptr + +/* + * Flags for external mbuf buffer types. + * NB: limited to the lower 24 bits. + */ +#define EXT_FLAG_EMBREF 0x000001 /* embedded ext_count */ +#define EXT_FLAG_EXTREF 0x000002 /* external ext_cnt, notyet */ + #define CSUM_IP 0x0001 #define CSUM_TCP 0x0002 #define CSUM_UDP 0x0004 @@ -109,15 +118,6 @@ extern int max_hdr; extern int max_datalen; // MHLEN - max_hdr -struct m_hdr { - struct mbuf* mh_next; - struct mbuf* mh_nextpkt; - caddr_t mh_data; - int mh_len; - int mh_flags; - short mh_type; -}; - struct pkthdr { struct ifnet* rcvif; int len; @@ -137,9 +137,14 @@ struct m_tag { }; struct m_ext { - caddr_t ext_buf; - unsigned int ext_size; - int ext_type; + union { + volatile u_int ext_count; /* value of ref count info */ + 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 */ + uint32_t ext_type:8, /* type of external storage */ + ext_flags:24; /* external storage mbuf flags */ }; struct mbuf { @@ -153,8 +158,14 @@ struct mbuf { SLIST_ENTRY(mbuf) m_slistpkt; STAILQ_ENTRY(mbuf) m_stailqpkt; }; + 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; +#if !defined(__LP64__) + uint32_t m_pad; /* pad for 64bit alignment */ +#endif - struct m_hdr m_hdr; union { struct { struct pkthdr MH_pkthdr; @@ -168,12 +179,6 @@ struct mbuf { }; -#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_act m_nextpkt #define m_pkthdr M_dat.MH.MH_pkthdr #define m_ext M_dat.MH.MH_dat.MH_ext @@ -183,7 +188,6 @@ struct mbuf { void m_catpkt(struct mbuf *m, struct mbuf *n); void m_adj(struct mbuf*, int); -void m_align(struct mbuf*, int); int m_append(struct mbuf*, int, c_caddr_t); void m_cat(struct mbuf*, struct mbuf*); int m_clget(struct mbuf*, int); diff --git a/src/libs/compat/freebsd11_network/fbsd_mbuf.c b/src/libs/compat/freebsd11_network/fbsd_mbuf.c index 443603abae..13593edb9f 100644 --- a/src/libs/compat/freebsd11_network/fbsd_mbuf.c +++ b/src/libs/compat/freebsd11_network/fbsd_mbuf.c @@ -653,13 +653,29 @@ bad: static void mb_dupcl(struct mbuf *n, struct mbuf *m) { - KASSERT((m->m_flags & M_EXT) == M_EXT, ("%s: M_EXT not set", __func__)); - KASSERT((n->m_flags & M_EXT) == 0, ("%s: M_EXT set", __func__)); + volatile u_int *refcnt; - n->m_ext.ext_buf = m->m_ext.ext_buf; - n->m_ext.ext_size = m->m_ext.ext_size; - n->m_ext.ext_type = m->m_ext.ext_type; + KASSERT(m->m_flags & M_EXT, ("%s: M_EXT not set on %p", __func__, m)); + KASSERT(!(n->m_flags & M_EXT), ("%s: M_EXT set on %p", __func__, n)); + + n->m_ext = m->m_ext; n->m_flags |= M_EXT; + n->m_flags |= m->m_flags & M_RDONLY; + + /* See if this is the mbuf that holds the embedded refcount. */ + if (m->m_ext.ext_flags & EXT_FLAG_EMBREF) { + refcnt = n->m_ext.ext_cnt = &m->m_ext.ext_count; + n->m_ext.ext_flags &= ~EXT_FLAG_EMBREF; + } else { + KASSERT(m->m_ext.ext_cnt != NULL, + ("%s: no refcounting pointer on %p", __func__, m)); + refcnt = m->m_ext.ext_cnt; + } + + if (*refcnt == 1) + *refcnt += 1; + else + atomic_add_int(refcnt, 1); } /* @@ -1057,25 +1073,6 @@ m_unshare(struct mbuf *m0, int how) return (m0); } -/* - * Set the m_data pointer of a newly-allocated mbuf - * to place an object of the specified size at the - * end of the mbuf, longword aligned. - */ -void -m_align(struct mbuf *m, int len) -{ - int adjust; - - if (m->m_flags & M_EXT) - adjust = m->m_ext.ext_size - len; - else if (m->m_flags & M_PKTHDR) - adjust = MHLEN - len; - else - adjust = MLEN - len; - m->m_data += adjust &~ (sizeof(long)-1); -} - /* * Copy an entire packet, including header (which must be present). * An optimization of the common case `m_copym(m, 0, M_COPYALL, how)'. diff --git a/src/libs/compat/freebsd11_network/mbuf.c b/src/libs/compat/freebsd11_network/mbuf.c index b29617f791..4aa37b8a91 100644 --- a/src/libs/compat/freebsd11_network/mbuf.c +++ b/src/libs/compat/freebsd11_network/mbuf.c @@ -88,11 +88,10 @@ construct_ext_sized_mbuf(struct mbuf *memoryBuffer, int how, int size) memoryBuffer->m_data = memoryBuffer->m_ext.ext_buf; memoryBuffer->m_flags |= M_EXT; - /* mb->m_ext.ext_free = NULL; */ - /* mb->m_ext.ext_args = NULL; */ memoryBuffer->m_ext.ext_size = size; memoryBuffer->m_ext.ext_type = extType; - /* mb->m_ext.ref_cnt = NULL; */ + memoryBuffer->m_ext.ext_flags = EXT_FLAG_EMBREF; + memoryBuffer->m_ext.ext_count = 1; return 0; } @@ -227,12 +226,44 @@ m_freem(struct mbuf *memoryBuffer) static void mb_free_ext(struct mbuf *memoryBuffer) { - /* - if (m->m_ext.ref_count != NULL) - panic("unsupported"); - */ - object_cache *cache = NULL; + volatile u_int *refcnt; + struct mbuf *mref; + int freembuf; + + KASSERT(memoryBuffer->m_flags & M_EXT, ("%s: M_EXT not set on %p", + __func__, memoryBuffer)); + + /* See if this is the mbuf that holds the embedded refcount. */ + if (memoryBuffer->m_ext.ext_flags & EXT_FLAG_EMBREF) { + refcnt = &memoryBuffer->m_ext.ext_count; + mref = memoryBuffer; + } else { + KASSERT(memoryBuffer->m_ext.ext_cnt != NULL, + ("%s: no refcounting pointer on %p", __func__, memoryBuffer)); + refcnt = memoryBuffer->m_ext.ext_cnt; + mref = __containerof(refcnt, struct mbuf, m_ext.ext_count); + } + + /* + * Check if the header is embedded in the cluster. It is + * important that we can't touch any of the mbuf fields + * after we have freed the external storage, since mbuf + * could have been embedded in it. For now, the mbufs + * embedded into the cluster are always of type EXT_EXTREF, + * and for this type we won't free the mref. + */ + if (memoryBuffer->m_flags & M_NOFREE) { + freembuf = 0; + KASSERT(memoryBuffer->m_ext.ext_type == EXT_EXTREF, + ("%s: no-free mbuf %p has wrong type", __func__, memoryBuffer)); + } else + freembuf = 1; + + /* Free attached storage only if this mbuf is the only reference to it. */ + if (!(*refcnt == 1 || atomic_add(refcnt, -1) == 1) + && !(freembuf && memoryBuffer != mref)) + return; if (memoryBuffer->m_ext.ext_type == EXT_CLUSTER) cache = sChunkCache;