From owner-cvs-all@FreeBSD.ORG Fri Nov 14 13:35:16 2003 Return-Path: Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id E757116A4CE; Fri, 14 Nov 2003 13:35:15 -0800 (PST) Received: from fledge.watson.org (fledge.watson.org [204.156.12.50]) by mx1.FreeBSD.org (Postfix) with ESMTP id A068343F85; Fri, 14 Nov 2003 13:35:14 -0800 (PST) (envelope-from robert@fledge.watson.org) Received: from fledge.watson.org (localhost [127.0.0.1]) by fledge.watson.org (8.12.9p2/8.12.9) with ESMTP id hAELXIMg023962; Fri, 14 Nov 2003 16:33:18 -0500 (EST) (envelope-from robert@fledge.watson.org) Received: from localhost (robert@localhost)hAELXIdP023959; Fri, 14 Nov 2003 16:33:18 -0500 (EST) (envelope-from robert@fledge.watson.org) Date: Fri, 14 Nov 2003 16:33:18 -0500 (EST) From: Robert Watson X-Sender: robert@fledge.watson.org To: Luigi Rizzo In-Reply-To: <20031114132714.A88606@xorpc.icir.org> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII cc: cvs-src@FreeBSD.org cc: Sam Leffler cc: cvs-all@FreeBSD.org cc: src-committers@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/an if_an.c if_anreg.h src/sys/dev/bfe if_bfe.c src/sys/dev/my if_my.c src/sys/dev/owi if_owi.c if_wivar.h src/sys/dev/re if_re.c src/sys/dev/wl if_wl.c src/sys/pci if_dc.c if_dcreg.h if_pcn.c if_pcnreg.h if_rl.c ... X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 14 Nov 2003 21:35:16 -0000 On Fri, 14 Nov 2003, Luigi Rizzo wrote: > On Fri, Nov 14, 2003 at 11:00:33AM -0800, Sam Leffler wrote: > > sam 2003/11/14 11:00:33 PST > ... > > Log: > > Drop the driver lock around calls to if_input to avoid a LOR when > > the packets are immediately returned for sending (e.g. when bridging > > or packet forwarding). There are more efficient ways to do this > > but for now use the least intrusive approach. > > the number of places that this commit had to touch makes me wonder > whether it wouldn't be better to include somehow these calls into the > if_input routine somehow... I can think of two concerns with that: first, that interface locks may be handled differently for different interfaces, and second, that in the future we may want to avoid an unlock/lock pair for each packet input. I've been running with local patches intended to reduce the number of unlock/lock pairs for coalesced interrupt handling by only unlocking to deliver chains of mbufs to if_input rather than individual mbufs. I.e., an approximation of the attached (can't remember if this is the exact patch since those boxes are at home). It could also be argued that the number of places that had to be touched in Sam's recent commit was a property of the locking strategy evolving from "ad hoc" to something more like a strategy. Robert N M Watson FreeBSD Core Team, TrustedBSD Projects robert@fledge.watson.org Network Associates Laboratories Index: if_fxp.c =================================================================== RCS file: /data/ncvs/src/sys/dev/fxp/if_fxp.c,v retrieving revision 1.194 diff -u -r1.194 if_fxp.c --- if_fxp.c 5 Sep 2003 22:37:31 -0000 1.194 +++ if_fxp.c 3 Oct 2003 16:43:11 -0000 @@ -1623,6 +1623,7 @@ fxp_intr_body(struct fxp_softc *sc, struct ifnet *ifp, u_int8_t statack, int count) { + struct mbuf *mqueue, *mqueue_tail; struct mbuf *m; struct fxp_rx *rxp; struct fxp_rfa *rfa; @@ -1685,6 +1686,7 @@ * record the pending RNR in the FXP_FLAG_DEFERRED_RNR flag so * that the info will be used in the subsequent polling cycle. */ + mqueue = mqueue_tail = NULL; for (;;) { rxp = sc->fxp_desc.rx_head; m = rxp->rx_mbuf; @@ -1766,10 +1768,21 @@ * packets received, dropping the lock, and then * calling if_input() on each one. */ - FXP_UNLOCK(sc); + if (mqueue != NULL) { + mqueue_tail->m_nextpkt = m; + mqueue_tail = m; + } else + mqueue = mqueue_tail = m; + } + } + if (mqueue != NULL) { + FXP_UNLOCK(sc); + while (mqueue != NULL) { + m = mqueue; + mqueue = mqueue->m_nextpkt; (*ifp->if_input)(ifp, m); - FXP_LOCK(sc); } + FXP_LOCK(sc); } if (rnr) { fxp_scb_wait(sc);