Hi Luiz,
I attached the patch, to be applied to the source code contained in this
image:
ftp://ftp.freebsd.org/pub/FreeBSD/snapshots/amd64/amd64/ISO-IMAGES/11.0/FreeBSD-11.0-STABLE-amd64-20170105-r311441-disc1.iso.xz
The patch is simply a backport from -head of some modifications involving
the emulated netmap mode.
I tried the minimize the changes to only (1) fix the crash and (2) fix
another synchronization problem.
I did different tests on em and vtnet interfaces, and the thing seems to
work.
Thanks,
Vincenzo
2017-01-19 14:24 GMT+01:00 Luiz Otavio O Souza <lists...@gmail.com>:
> On 19 January 2017 at 09:11, Vincenzo Maffione wrote:
> > Hi,
> >
> > A change in mb_free_ext() introduced in FreeBSD 11 versions broke the
> > transmission support for netmap in emulated mode. This means immediate
> > kernel crashes for netmap users in non-native (emulated) mode.
> >
> > This problem has been fixed in FreeBSD-12-CURRENT, which contains a
> recent
> > version of Netmap. However, FreeBSD 11 versions (release, stable) are
> still
> > affected, as they contain old Netmap versions.
> > Is it possible to push the fix (provided by me) to the relevant
> > release/stable branches? What is the procedure (I'm not familiar with the
> > FreeBSD release engineering process)?
>
> Hi Vincenzo,
>
> Yeah, I saw that trying netmap on ARM (running with emulated drivers)
> and I can confirm that it is fixed on -head.
>
> Please, send me the fix and I'll get it committed on stable/11 (this
> is a common situation and the fix can be committed directly into the
> affected branch).
>
> Thanks!
> Luiz
>
--
Vincenzo Maffione
diff --git a/sys/dev/netmap/netmap_freebsd.c b/sys/dev/netmap/netmap_freebsd.c
index 25c9c5c..040d1ac 100644
--- a/sys/dev/netmap/netmap_freebsd.c
+++ b/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 */
diff --git a/sys/dev/netmap/netmap_generic.c b/sys/dev/netmap/netmap_generic.c
index 7254787..320f7b2 100644
--- a/sys/dev/netmap/netmap_generic.c
+++ b/sys/dev/netmap/netmap_generic.c
@@ -90,53 +90,40 @@ __FBSDID("$FreeBSD: stable/11/sys/dev/netmap/netmap_generic.c 298955 2016-05-03
/*
* 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. */
diff --git a/sys/dev/netmap/netmap_kern.h b/sys/dev/netmap/netmap_kern.h
index 3d0f9d9..98518de 100644
--- a/sys/dev/netmap/netmap_kern.h
+++ b/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);
_______________________________________________
freebsd-net@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
