From owner-svn-src-user@FreeBSD.ORG Sat Nov 29 22:56:44 2008 Return-Path: Delivered-To: svn-src-user@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E48AB106567A for ; Sat, 29 Nov 2008 22:56:44 +0000 (UTC) (envelope-from mat.macy@gmail.com) Received: from rv-out-0506.google.com (rv-out-0506.google.com [209.85.198.230]) by mx1.freebsd.org (Postfix) with ESMTP id BA8A78FC17 for ; Sat, 29 Nov 2008 22:56:44 +0000 (UTC) (envelope-from mat.macy@gmail.com) Received: by rv-out-0506.google.com with SMTP id b25so1819351rvf.43 for ; Sat, 29 Nov 2008 14:56:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:received:received:message-id:date:from:sender :to:subject:cc:in-reply-to:mime-version:content-type :content-transfer-encoding:content-disposition:references :x-google-sender-auth; bh=sIyjBJHOsPKB/kidb/m4j51zyzJuWO7S17g19perJiA=; b=AG7iDsz67R1knOlcYwrAG8kSdpK8ndr+GiTF8daz/iPEHNFL44b3SQfXo73S7HY5TQ MATa3x/9tZoJqbQgiLfdhf0zkA1K2KV/TTYEbxaoPzMgnnDZEAOcQjrBbJVIV37UzBrr qtho2dGvVcLqPXauQU/U3aokbJaP1LYQLhdj4= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=message-id:date:from:sender:to:subject:cc:in-reply-to:mime-version :content-type:content-transfer-encoding:content-disposition :references:x-google-sender-auth; b=QUZvDdMFbVjjQxX8ss9pq//Jwv+O4ZCCA2noRdd6U1m8EyNtonAO6acXnoaaz2mGQg AEiKFKy1RBcZtxOewGIrJ+VW60FeQVZOYHqLDMy5SXJ32lqcNzwTLb+b6gTAjH619N2F jGyF/PEyGgE7q82kwYxmcXnkdiFTps1zkWBF0= Received: by 10.141.115.16 with SMTP id s16mr4417555rvm.209.1227999404435; Sat, 29 Nov 2008 14:56:44 -0800 (PST) Received: by 10.141.142.3 with HTTP; Sat, 29 Nov 2008 14:56:44 -0800 (PST) Message-ID: <3c1674c90811291456i80dcc67ie7278ae61dd786ea@mail.gmail.com> Date: Sat, 29 Nov 2008 22:56:44 +0000 From: "Kip Macy" Sender: mat.macy@gmail.com To: "Doug Rabson" In-Reply-To: <200811291734.mATHYkQK057833@svn.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline References: <200811291734.mATHYkQK057833@svn.freebsd.org> X-Google-Sender-Auth: c125bb1c2027f8e6 Cc: src-committers@freebsd.org, svn-src-user@freebsd.org Subject: Re: svn commit: r185444 - user/dfr/xenhvm/6/sys/dev/xen/netfront X-BeenThere: svn-src-user@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the experimental " user" src tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 29 Nov 2008 22:56:45 -0000 Hi Doug, Please push any such fixes back in to HEAD. Thanks, Kip On Sat, Nov 29, 2008 at 5:34 PM, Doug Rabson 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