Fix a crash in netmap when using the emulated mode.

This is a direct commit to stable/11 as the -head version was already fixed
by a recent import of a new netmap version.

Submitted by:	Vincenzo Maffione <v.maffione@gmail.com>
Sponsored by:	Rubicon Communications, LLC (Netgate)

sys/dev/netmap/netmap_freebsd.c

@@ -218,30 +218,16 @@ generic_xmit_frame(struct ifnet *ifp, struct mbuf *m,
 {
 	int ret;
 
-	/*
-	 * The mbuf should be a cluster from our special pool,
-	 * so we do not need to do an m_copyback but just copy
-	 * (and eventually, just reference the netmap buffer)
-	 */
+	/* Link the external storage to the netmap buffer, so that
+	 * no copy is necessary. */
+	m->m_ext.ext_buf = m->m_data = addr;
+	m->m_ext.ext_size = len;
 
-	if (GET_MBUF_REFCNT(m) != 1) {
-		D("invalid refcnt %d for %p",
-			GET_MBUF_REFCNT(m), m);
-		panic("in generic_xmit_frame");
-	}
-	// XXX the ext_size check is unnecessary if we link the netmap buf
-	if (m->m_ext.ext_size < len) {
-		RD(5, "size %d < len %d", m->m_ext.ext_size, len);
-		len = m->m_ext.ext_size;
-	}
-	if (0) { /* XXX seems to have negligible benefits */
-		m->m_ext.ext_buf = m->m_data = addr;
-	} else {
-		bcopy(addr, m->m_data, len);
-	}
 	m->m_len = m->m_pkthdr.len = len;
-	// inc refcount. All ours, we could skip the atomic
-	atomic_fetchadd_int(PNT_MBUF_REFCNT(m), 1);
+
+	/* mbuf refcnt is not contended, no need to use atomic
+	 * (a memory barrier is enough). */
+	SET_MBUF_REFCNT(m, 2);
 	M_HASHTYPE_SET(m, M_HASHTYPE_OPAQUE);
 	m->m_pkthdr.flowid = ring_nr;
 	m->m_pkthdr.rcvif = ifp; /* used for tx notification */

sys/dev/netmap/netmap_generic.c

@@ -90,53 +90,40 @@ __FBSDID("$FreeBSD$");
 /*
  * FreeBSD mbuf allocator/deallocator in emulation mode:
  *
- * We allocate EXT_PACKET mbuf+clusters, but need to set M_NOFREE
- * so that the destructor, if invoked, will not free the packet.
- *    In principle we should set the destructor only on demand,
- * but since there might be a race we better do it on allocation.
- * As a consequence, we also need to set the destructor or we
- * would leak buffers.
- */
-
-/*
- * mbuf wrappers
+ * We allocate mbufs with m_gethdr(), since the mbuf header is needed
+ * by the driver. We also attach a customly-provided external storage,
+ * which in this case is a netmap buffer. When calling m_extadd(), however
+ * we pass a NULL address, since the real address (and length) will be
+ * filled in by nm_os_generic_xmit_frame() right before calling
+ * if_transmit().
+ *
+ * The dtor function does nothing, however we need it since mb_free_ext()
+ * has a KASSERT(), checking that the mbuf dtor function is not NULL.
  */
 
-/* mbuf destructor, also need to change the type to EXT_EXTREF,
- * add an M_NOFREE flag, and then clear the flag and
- * chain into uma_zfree(zone_pack, mf)
- * (or reinstall the buffer ?)
- */
-#define SET_MBUF_DESTRUCTOR(m, fn)	do {		\
-	(m)->m_ext.ext_free = (void *)fn;	\
-	(m)->m_ext.ext_type = EXT_EXTREF;	\
-} while (0)
+static void void_mbuf_dtor(struct mbuf *m, void *arg1, void *arg2) { }
 
-static void
-netmap_default_mbuf_destructor(struct mbuf *m)
+static inline void
+SET_MBUF_DESTRUCTOR(struct mbuf *m, void *fn)
 {
-	/* restore original mbuf */
-	m->m_ext.ext_buf = m->m_data = m->m_ext.ext_arg1;
-	m->m_ext.ext_arg1 = NULL;
-	m->m_ext.ext_type = EXT_PACKET;
-	m->m_ext.ext_free = NULL;
-	if (GET_MBUF_REFCNT(m) == 0)
-		SET_MBUF_REFCNT(m, 1);
-	uma_zfree(zone_pack, m);
+	m->m_ext.ext_free = fn ? fn : (void *)void_mbuf_dtor;
 }
 
 static inline struct mbuf *
 netmap_get_mbuf(int len)
 {
 	struct mbuf *m;
-	m = m_getcl(M_NOWAIT, MT_DATA, M_PKTHDR);
-	if (m) {
-		m->m_flags |= M_NOFREE;	/* XXXNP: Almost certainly incorrect. */
-		m->m_ext.ext_arg1 = m->m_ext.ext_buf; // XXX save
-		m->m_ext.ext_free = (void *)netmap_default_mbuf_destructor;
-		m->m_ext.ext_type = EXT_EXTREF;
-		ND(5, "create m %p refcnt %d", m, GET_MBUF_REFCNT(m));
+
+	(void)len;
+
+	m = m_gethdr(M_NOWAIT, MT_DATA);
+	if (m == NULL) {
+		return m;
 	}
+
+	m_extadd(m, NULL /* buf */, 0 /* size */, void_mbuf_dtor,
+		 NULL, NULL, 0, EXT_NET_DRV);
+
 	return m;
 }
 
@@ -412,11 +399,6 @@ static void
 generic_mbuf_destructor(struct mbuf *m)
 {
 	netmap_generic_irq(MBUF_IFP(m), MBUF_TXQ(m), NULL);
-#ifdef __FreeBSD__
-	if (netmap_verbose)
-		RD(5, "Tx irq (%p) queue %d index %d" , m, MBUF_TXQ(m), (int)(uintptr_t)m->m_ext.ext_arg1);
-	netmap_default_mbuf_destructor(m);
-#endif /* __FreeBSD__ */
 	IFRATE(rate_ctx.new.txirq++);
 }
 
@@ -447,7 +429,7 @@ generic_netmap_tx_clean(struct netmap_kring *kring)
 				// XXX how do we proceed ? break ?
 				return -ENOMEM;
 			}
-		} else if (GET_MBUF_REFCNT(m) != 1) {
+		} else if (MBUF_REFCNT(m) != 1) {
 			break; /* This mbuf is still busy: its refcnt is 2. */
 		}
 		n++;
@@ -476,62 +458,39 @@ generic_netmap_tx_clean(struct netmap_kring *kring)
 	return n;
 }
 
-
-/*
- * We have pending packets in the driver between nr_hwtail +1 and hwcur.
- * Compute a position in the middle, to be used to generate
- * a notification.
- */
-static inline u_int
-generic_tx_event_middle(struct netmap_kring *kring, u_int hwcur)
-{
-	u_int n = kring->nkr_num_slots;
-	u_int ntc = nm_next(kring->nr_hwtail, n-1);
-	u_int e;
-
-	if (hwcur >= ntc) {
-		e = (hwcur + ntc) / 2;
-	} else { /* wrap around */
-		e = (hwcur + n + ntc) / 2;
-		if (e >= n) {
-			e -= n;
-		}
-	}
-
-	if (unlikely(e >= n)) {
-		D("This cannot happen");
-		e = 0;
-	}
-
-	return e;
-}
-
-/*
- * We have pending packets in the driver between nr_hwtail+1 and hwcur.
- * Schedule a notification approximately in the middle of the two.
- * There is a race but this is only called within txsync which does
- * a double check.
- */
 static void
 generic_set_tx_event(struct netmap_kring *kring, u_int hwcur)
 {
+	u_int lim = kring->nkr_num_slots - 1;
 	struct mbuf *m;
 	u_int e;
+	u_int ntc = nm_next(kring->nr_hwtail, lim); /* next to clean */
 
-	if (nm_next(kring->nr_hwtail, kring->nkr_num_slots -1) == hwcur) {
+	if (ntc == hwcur) {
 		return; /* all buffers are free */
 	}
-	e = generic_tx_event_middle(kring, hwcur);
+
+	/*
+	 * We have pending packets in the driver between hwtail+1
+	 * and hwcur, and we have to chose one of these slot to
+	 * generate a notification.
+	 * There is a race but this is only called within txsync which
+	 * does a double check.
+	 */
+
+	/* Choose the first pending slot, to be safe against driver
+	 * reordering mbuf transmissions. */
+	e = ntc;
 
 	m = kring->tx_pool[e];
-	ND(5, "Request Event at %d mbuf %p refcnt %d", e, m, m ? GET_MBUF_REFCNT(m) : -2 );
+	ND(5, "Request Event at %d mbuf %p refcnt %d", e, m, m ? MBUF_REFCNT(m) : -2 );
 	if (m == NULL) {
 		/* This can happen if there is already an event on the netmap
 		   slot 'e': There is nothing to do. */
 		return;
 	}
 	kring->tx_pool[e] = NULL;
-	SET_MBUF_DESTRUCTOR(m, generic_mbuf_destructor);
+	SET_MBUF_DESTRUCTOR(m, (void *)generic_mbuf_destructor);
 
 	// XXX wmb() ?
 	/* Decrement the refcount an free it if we have the last one. */

sys/dev/netmap/netmap_kern.h

@@ -97,13 +97,11 @@ struct netmap_adapter *netmap_getna(if_t ifp);
 #endif
 
 #if __FreeBSD_version >= 1100027
-#define GET_MBUF_REFCNT(m)      ((m)->m_ext.ext_cnt ? *((m)->m_ext.ext_cnt) : -1)
-#define SET_MBUF_REFCNT(m, x)   *((m)->m_ext.ext_cnt) = x
-#define PNT_MBUF_REFCNT(m)      ((m)->m_ext.ext_cnt)
+#define MBUF_REFCNT(m)		((m)->m_ext.ext_count)
+#define SET_MBUF_REFCNT(m, x)   (m)->m_ext.ext_count = x
 #else
-#define GET_MBUF_REFCNT(m)      ((m)->m_ext.ref_cnt ? *((m)->m_ext.ref_cnt) : -1)
+#define MBUF_REFCNT(m)		((m)->m_ext.ref_cnt ? *((m)->m_ext.ref_cnt) : -1)
 #define SET_MBUF_REFCNT(m, x)   *((m)->m_ext.ref_cnt) = x
-#define PNT_MBUF_REFCNT(m)      ((m)->m_ext.ref_cnt)
 #endif
 
 MALLOC_DECLARE(M_NETMAP);