Date: Fri, 6 Jun 2014 18:36:02 +0000 (UTC) From: Luigi Rizzo <luigi@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r267180 - head/sys/dev/netmap Message-ID: <201406061836.s56Ia2ea079947@svn.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: luigi Date: Fri Jun 6 18:36:02 2014 New Revision: 267180 URL: http://svnweb.freebsd.org/changeset/base/267180 Log: better handling of netmap emulation over standard device drivers: plug a potential mbuf leak, and detect bogus drivers that return ENOBUFS even when the packet has been queued. MFC after: 3 days Modified: head/sys/dev/netmap/netmap_freebsd.c head/sys/dev/netmap/netmap_generic.c Modified: head/sys/dev/netmap/netmap_freebsd.c ============================================================================== --- head/sys/dev/netmap/netmap_freebsd.c Fri Jun 6 18:32:05 2014 (r267179) +++ head/sys/dev/netmap/netmap_freebsd.c Fri Jun 6 18:36:02 2014 (r267180) @@ -61,7 +61,8 @@ /* ======================== FREEBSD-SPECIFIC ROUTINES ================== */ -rawsum_t nm_csum_raw(uint8_t *data, size_t len, rawsum_t cur_sum) +rawsum_t +nm_csum_raw(uint8_t *data, size_t len, rawsum_t cur_sum) { /* TODO XXX please use the FreeBSD implementation for this. */ uint16_t *words = (uint16_t *)data; @@ -80,7 +81,8 @@ rawsum_t nm_csum_raw(uint8_t *data, size /* Fold a raw checksum: 'cur_sum' is in host byte order, while the * return value is in network byte order. */ -uint16_t nm_csum_fold(rawsum_t cur_sum) +uint16_t +nm_csum_fold(rawsum_t cur_sum) { /* TODO XXX please use the FreeBSD implementation for this. */ while (cur_sum >> 16) @@ -89,7 +91,8 @@ uint16_t nm_csum_fold(rawsum_t cur_sum) return htobe16((~cur_sum) & 0xFFFF); } -uint16_t nm_csum_ipv4(struct nm_iphdr *iph) +uint16_t +nm_csum_ipv4(struct nm_iphdr *iph) { #if 0 return in_cksum_hdr((void *)iph); @@ -98,7 +101,8 @@ uint16_t nm_csum_ipv4(struct nm_iphdr *i #endif } -void nm_csum_tcpudp_ipv4(struct nm_iphdr *iph, void *data, +void +nm_csum_tcpudp_ipv4(struct nm_iphdr *iph, void *data, size_t datalen, uint16_t *check) { #ifdef INET @@ -120,7 +124,8 @@ void nm_csum_tcpudp_ipv4(struct nm_iphdr #endif } -void nm_csum_tcpudp_ipv6(struct nm_ipv6hdr *ip6h, void *data, +void +nm_csum_tcpudp_ipv6(struct nm_ipv6hdr *ip6h, void *data, size_t datalen, uint16_t *check) { #ifdef INET6 @@ -143,7 +148,8 @@ void nm_csum_tcpudp_ipv6(struct nm_ipv6h int netmap_catch_rx(struct netmap_adapter *na, int intercept) { - struct netmap_generic_adapter *gna = (struct netmap_generic_adapter *)na; + struct netmap_generic_adapter *gna = + (struct netmap_generic_adapter *)na; struct ifnet *ifp = na->ifp; if (intercept) { @@ -209,11 +215,29 @@ generic_xmit_frame(struct ifnet *ifp, st { int ret; - m->m_len = m->m_pkthdr.len = 0; + /* + * 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) + */ - // copy data to the mbuf - m_copyback(m, 0, len, addr); - // inc refcount. We are alone, so we can skip the atomic + if (*m->m_ext.ref_cnt != 1) { + D("invalid refcnt %d for %p", + *m->m_ext.ref_cnt, 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(m->m_ext.ref_cnt, 1); m->m_flags |= M_FLOWID; m->m_pkthdr.flowid = ring_nr; @@ -238,7 +262,7 @@ netmap_getna(if_t ifp) int generic_find_num_desc(struct ifnet *ifp, unsigned int *tx, unsigned int *rx) { - D("called"); + D("called, in tx %d rx %d", *tx, *rx); return 0; } @@ -246,13 +270,14 @@ generic_find_num_desc(struct ifnet *ifp, void generic_find_num_queues(struct ifnet *ifp, u_int *txq, u_int *rxq) { - D("called"); + D("called, in txq %d rxq %d", *txq, *rxq); *txq = netmap_generic_rings; *rxq = netmap_generic_rings; } -void netmap_mitigation_init(struct nm_generic_mit *mit, struct netmap_adapter *na) +void +netmap_mitigation_init(struct nm_generic_mit *mit, struct netmap_adapter *na) { ND("called"); mit->mit_pending = 0; @@ -260,26 +285,30 @@ void netmap_mitigation_init(struct nm_ge } -void netmap_mitigation_start(struct nm_generic_mit *mit) +void +netmap_mitigation_start(struct nm_generic_mit *mit) { ND("called"); } -void netmap_mitigation_restart(struct nm_generic_mit *mit) +void +netmap_mitigation_restart(struct nm_generic_mit *mit) { ND("called"); } -int netmap_mitigation_active(struct nm_generic_mit *mit) +int +netmap_mitigation_active(struct nm_generic_mit *mit) { ND("called"); return 0; } -void netmap_mitigation_cleanup(struct nm_generic_mit *mit) +void +netmap_mitigation_cleanup(struct nm_generic_mit *mit) { ND("called"); } Modified: head/sys/dev/netmap/netmap_generic.c ============================================================================== --- head/sys/dev/netmap/netmap_generic.c Fri Jun 6 18:32:05 2014 (r267179) +++ head/sys/dev/netmap/netmap_generic.c Fri Jun 6 18:36:02 2014 (r267180) @@ -81,8 +81,8 @@ __FBSDID("$FreeBSD$"); #include <dev/netmap/netmap_kern.h> #include <dev/netmap/netmap_mem2.h> -#define rtnl_lock() ND("rtnl_lock called"); -#define rtnl_unlock() ND("rtnl_unlock called"); +#define rtnl_lock() ND("rtnl_lock called") +#define rtnl_unlock() ND("rtnl_unlock called") #define MBUF_TXQ(m) ((m)->m_pkthdr.flowid) #define MBUF_RXQ(m) ((m)->m_pkthdr.flowid) #define smp_mb() @@ -101,7 +101,6 @@ __FBSDID("$FreeBSD$"); /* * mbuf wrappers */ -#define netmap_get_mbuf(len) m_getcl(M_NOWAIT, MT_DATA, M_PKTHDR|M_NOFREE) /* mbuf destructor, also need to change the type to EXT_EXTREF, * add an M_NOFREE flag, and then clear the flag and @@ -113,6 +112,32 @@ __FBSDID("$FreeBSD$"); (m)->m_ext.ext_type = EXT_EXTREF; \ } while (0) +static void +netmap_default_mbuf_destructor(struct mbuf *m) +{ + /* 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 (*(m->m_ext.ref_cnt) == 0) + *(m->m_ext.ref_cnt) = 1; + uma_zfree(zone_pack, m); +} + +static inline struct mbuf * +netmap_get_mbuf(int len) +{ + struct mbuf *m; + m = m_getcl(M_NOWAIT, MT_DATA, M_PKTHDR | M_NOFREE); + if (m) { + 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, *m->m_ext.ref_cnt); + } + return m; +} #define GET_MBUF_REFCNT(m) ((m)->m_ext.ref_cnt ? *(m)->m_ext.ref_cnt : -1) @@ -230,7 +255,7 @@ generic_netmap_register(struct netmap_ad #endif /* REG_RESET */ if (enable) { /* Enable netmap mode. */ - /* Init the mitigation support. */ + /* Init the mitigation support on all the rx queues. */ gna->mit = malloc(na->num_rx_rings * sizeof(struct nm_generic_mit), M_DEVBUF, M_NOWAIT | M_ZERO); if (!gna->mit) { @@ -380,15 +405,11 @@ out: static void generic_mbuf_destructor(struct mbuf *m) { - if (netmap_verbose) - D("Tx irq (%p) queue %d", m, MBUF_TXQ(m)); netmap_generic_irq(MBUF_IFP(m), MBUF_TXQ(m), NULL); #ifdef __FreeBSD__ - m->m_ext.ext_type = EXT_PACKET; - m->m_ext.ext_free = NULL; - if (*(m->m_ext.ref_cnt) == 0) - *(m->m_ext.ref_cnt) = 1; - uma_zfree(zone_pack, m); + 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++); } @@ -478,12 +499,12 @@ generic_set_tx_event(struct netmap_kring e = generic_tx_event_middle(kring, hwcur); m = kring->tx_pool[e]; + ND(5, "Request Event at %d mbuf %p refcnt %d", e, m, m ? GET_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; } - ND("Event at %d mbuf %p refcnt %d", e, m, GET_MBUF_REFCNT(m)); kring->tx_pool[e] = NULL; SET_MBUF_DESTRUCTOR(m, generic_mbuf_destructor); @@ -777,6 +798,10 @@ generic_netmap_attach(struct ifnet *ifp) generic_find_num_desc(ifp, &num_tx_desc, &num_rx_desc); ND("Netmap ring size: TX = %d, RX = %d", num_tx_desc, num_rx_desc); + if (num_tx_desc == 0 || num_rx_desc == 0) { + D("Device has no hw slots (tx %u, rx %u)", num_tx_desc, num_rx_desc); + return EINVAL; + } gna = malloc(sizeof(*gna), M_DEVBUF, M_NOWAIT | M_ZERO); if (gna == NULL) {
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201406061836.s56Ia2ea079947>