Date: Wed, 25 Jan 2017 21:35:15 +0000 (UTC) From: Luiz Otavio O Souza <loos@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-11@freebsd.org Subject: svn commit: r312783 - stable/11/sys/dev/netmap Message-ID: <201701252135.v0PLZFcJ022343@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
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.maffione@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);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201701252135.v0PLZFcJ022343>