Date: Sat, 29 Nov 2008 22:56:44 +0000 From: "Kip Macy" <kmacy@freebsd.org> To: "Doug Rabson" <dfr@freebsd.org> Cc: src-committers@freebsd.org, svn-src-user@freebsd.org Subject: Re: svn commit: r185444 - user/dfr/xenhvm/6/sys/dev/xen/netfront Message-ID: <3c1674c90811291456i80dcc67ie7278ae61dd786ea@mail.gmail.com> In-Reply-To: <200811291734.mATHYkQK057833@svn.freebsd.org> References: <200811291734.mATHYkQK057833@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Doug, Please push any such fixes back in to HEAD. Thanks, Kip On Sat, Nov 29, 2008 at 5:34 PM, Doug Rabson <dfr@freebsd.org> wrote: > Author: dfr > Date: Sat Nov 29 17:34:46 2008 > New Revision: 185444 > URL: http://svn.freebsd.org/changeset/base/185444 > > Log: > Don't call ether_ioctl() with locks held. Loop in xn_rxeof() until the backend > stops adding stuff to the ring otherwise we miss RX interrupts which kills > performance. > > Modified: > user/dfr/xenhvm/6/sys/dev/xen/netfront/netfront.c > > Modified: user/dfr/xenhvm/6/sys/dev/xen/netfront/netfront.c > ============================================================================== > --- user/dfr/xenhvm/6/sys/dev/xen/netfront/netfront.c Sat Nov 29 17:33:38 2008 (r185443) > +++ user/dfr/xenhvm/6/sys/dev/xen/netfront/netfront.c Sat Nov 29 17:34:46 2008 (r185444) > @@ -858,110 +858,112 @@ xn_rxeof(struct netfront_info *np) > multicall_entry_t *mcl; > struct mbuf *m; > struct mbuf_head rxq, errq; > - int err, pages_flipped = 0; > + int err, pages_flipped = 0, work_to_do; > > - XN_RX_LOCK_ASSERT(np); > - if (!netfront_carrier_ok(np)) > - return; > + do { > + XN_RX_LOCK_ASSERT(np); > + if (!netfront_carrier_ok(np)) > + return; > > - mbufq_init(&errq); > - mbufq_init(&rxq); > + mbufq_init(&errq); > + mbufq_init(&rxq); > > - ifp = np->xn_ifp; > + ifp = np->xn_ifp; > > - rp = np->rx.sring->rsp_prod; > - rmb(); /* Ensure we see queued responses up to 'rp'. */ > + rp = np->rx.sring->rsp_prod; > + rmb(); /* Ensure we see queued responses up to 'rp'. */ > + > + i = np->rx.rsp_cons; > + while ((i != rp)) { > + memcpy(rx, RING_GET_RESPONSE(&np->rx, i), sizeof(*rx)); > + memset(extras, 0, sizeof(rinfo.extras)); > > - i = np->rx.rsp_cons; > - while ((i != rp)) { > - memcpy(rx, RING_GET_RESPONSE(&np->rx, i), sizeof(*rx)); > - memset(extras, 0, sizeof(rinfo.extras)); > - > - m = NULL; > - err = xennet_get_responses(np, &rinfo, rp, &m, > - &pages_flipped); > + m = NULL; > + err = xennet_get_responses(np, &rinfo, rp, &m, > + &pages_flipped); > > - if (unlikely(err)) { > + if (unlikely(err)) { > if (m) > - mbufq_tail(&errq, m); > - np->stats.rx_errors++; > - i = np->rx.rsp_cons; > - continue; > - } > + mbufq_tail(&errq, m); > + np->stats.rx_errors++; > + i = np->rx.rsp_cons; > + continue; > + } > > - m->m_pkthdr.rcvif = ifp; > - if ( rx->flags & NETRXF_data_validated ) { > - /* Tell the stack the checksums are okay */ > - /* > - * XXX this isn't necessarily the case - need to add > - * check > - */ > + m->m_pkthdr.rcvif = ifp; > + if ( rx->flags & NETRXF_data_validated ) { > + /* Tell the stack the checksums are okay */ > + /* > + * XXX this isn't necessarily the case - need to add > + * check > + */ > > - m->m_pkthdr.csum_flags |= > - (CSUM_IP_CHECKED | CSUM_IP_VALID | CSUM_DATA_VALID > - | CSUM_PSEUDO_HDR); > - m->m_pkthdr.csum_data = 0xffff; > - } > + m->m_pkthdr.csum_flags |= > + (CSUM_IP_CHECKED | CSUM_IP_VALID | CSUM_DATA_VALID > + | CSUM_PSEUDO_HDR); > + m->m_pkthdr.csum_data = 0xffff; > + } > > - np->stats.rx_packets++; > - np->stats.rx_bytes += m->m_pkthdr.len; > + np->stats.rx_packets++; > + np->stats.rx_bytes += m->m_pkthdr.len; > > - mbufq_tail(&rxq, m); > - np->rx.rsp_cons = ++i; > - } > + mbufq_tail(&rxq, m); > + np->rx.rsp_cons = ++i; > + } > > - if (pages_flipped) { > - /* Some pages are no longer absent... */ > + if (pages_flipped) { > + /* Some pages are no longer absent... */ > #ifdef notyet > - balloon_update_driver_allowance(-pages_flipped); > + balloon_update_driver_allowance(-pages_flipped); > #endif > - /* Do all the remapping work, and M->P updates, in one big > - * hypercall. > - */ > - if (!!xen_feature(XENFEAT_auto_translated_physmap)) { > - mcl = np->rx_mcl + pages_flipped; > - mcl->op = __HYPERVISOR_mmu_update; > - mcl->args[0] = (u_long)np->rx_mmu; > - mcl->args[1] = pages_flipped; > - mcl->args[2] = 0; > - mcl->args[3] = DOMID_SELF; > - (void)HYPERVISOR_multicall(np->rx_mcl, > - pages_flipped + 1); > + /* Do all the remapping work, and M->P updates, in one big > + * hypercall. > + */ > + if (!!xen_feature(XENFEAT_auto_translated_physmap)) { > + mcl = np->rx_mcl + pages_flipped; > + mcl->op = __HYPERVISOR_mmu_update; > + mcl->args[0] = (u_long)np->rx_mmu; > + mcl->args[1] = pages_flipped; > + mcl->args[2] = 0; > + mcl->args[3] = DOMID_SELF; > + (void)HYPERVISOR_multicall(np->rx_mcl, > + pages_flipped + 1); > + } > } > - } > > - while ((m = mbufq_dequeue(&errq))) > - m_freem(m); > - > - /* > - * Process all the mbufs after the remapping is complete. > - * Break the mbuf chain first though. > - */ > - while ((m = mbufq_dequeue(&rxq)) != NULL) { > - ifp->if_ipackets++; > - > - /* > - * Do we really need to drop the rx lock? > + while ((m = mbufq_dequeue(&errq))) > + m_freem(m); > + > + /* > + * Process all the mbufs after the remapping is complete. > + * Break the mbuf chain first though. > */ > - XN_RX_UNLOCK(np); > - /* Pass it up. */ > - (*ifp->if_input)(ifp, m); > - XN_RX_LOCK(np); > - } > + while ((m = mbufq_dequeue(&rxq)) != NULL) { > + ifp->if_ipackets++; > + > + /* > + * Do we really need to drop the rx lock? > + */ > + XN_RX_UNLOCK(np); > + /* Pass it up. */ > + (*ifp->if_input)(ifp, m); > + XN_RX_LOCK(np); > + } > > - np->rx.rsp_cons = i; > + np->rx.rsp_cons = i; > > #if 0 > - /* If we get a callback with very few responses, reduce fill target. */ > - /* NB. Note exponential increase, linear decrease. */ > - if (((np->rx.req_prod_pvt - np->rx.sring->rsp_prod) > > - ((3*np->rx_target) / 4)) && (--np->rx_target < np->rx_min_target)) > - np->rx_target = np->rx_min_target; > + /* If we get a callback with very few responses, reduce fill target. */ > + /* NB. Note exponential increase, linear decrease. */ > + if (((np->rx.req_prod_pvt - np->rx.sring->rsp_prod) > > + ((3*np->rx_target) / 4)) && (--np->rx_target < np->rx_min_target)) > + np->rx_target = np->rx_min_target; > #endif > > - network_alloc_rx_buffers(np); > + network_alloc_rx_buffers(np); > > - np->rx.sring->rsp_event = i + 1; > + RING_FINAL_CHECK_FOR_RESPONSES(&np->rx, work_to_do); > + } while (work_to_do); > } > > static void > @@ -1440,9 +1442,11 @@ xn_ioctl(struct ifnet *ifp, u_long cmd, > if (!(ifp->if_drv_flags & IFF_DRV_RUNNING)) > xn_ifinit_locked(sc); > arp_ifinit(ifp, ifa); > - } else > + XN_UNLOCK(sc); > + } else { > + XN_UNLOCK(sc); > error = ether_ioctl(ifp, cmd, data); > - XN_UNLOCK(sc); > + } > break; > case SIOCSIFMTU: > /* XXX can we alter the MTU on a VN ?*/ > -- If we desire respect for the law, we must first make the law respectable. - Louis D. Brandeis
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3c1674c90811291456i80dcc67ie7278ae61dd786ea>