Author: loos
Date: Wed Jan 25 21:35:15 2017
New Revision: 312783
URL: https://svnweb.freebsd.org/changeset/base/312783
Log:
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.maffi...@gmail.com>
Sponsored by: Rubicon Communications, LLC (Netgate)
Modified:
stable/11/sys/dev/netmap/netmap_freebsd.c
stable/11/sys/dev/netmap/netmap_generic.c
stable/11/sys/dev/netmap/netmap_kern.h
Modified: stable/11/sys/dev/netmap/netmap_freebsd.c
==============================================================================
--- stable/11/sys/dev/netmap/netmap_freebsd.c Wed Jan 25 21:25:26 2017
(r312782)
+++ stable/11/sys/dev/netmap/netmap_freebsd.c Wed Jan 25 21:35:15 2017
(r312783)
@@ -218,30 +218,16 @@ generic_xmit_frame(struct ifnet *ifp, st
{
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 */
Modified: stable/11/sys/dev/netmap/netmap_generic.c
==============================================================================
--- stable/11/sys/dev/netmap/netmap_generic.c Wed Jan 25 21:25:26 2017
(r312782)
+++ stable/11/sys/dev/netmap/netmap_generic.c Wed Jan 25 21:35:15 2017
(r312783)
@@ -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_kr
// 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_kr
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. */
Modified: stable/11/sys/dev/netmap/netmap_kern.h
==============================================================================
--- stable/11/sys/dev/netmap/netmap_kern.h Wed Jan 25 21:25:26 2017
(r312782)
+++ stable/11/sys/dev/netmap/netmap_kern.h Wed Jan 25 21:35:15 2017
(r312783)
@@ -97,13 +97,11 @@ struct netmap_adapter *netmap_getna(if_t
#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);
_______________________________________________
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
