Date: Tue, 21 Apr 2009 10:24:07 +0100 (BST) From: Robert Watson <rwatson@FreeBSD.org> To: Pyun YongHyeon <yongari@FreeBSD.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r191344 - head/sys/dev/xl Message-ID: <alpine.BSF.2.00.0904211022130.5067@fledge.watson.org> In-Reply-To: <200904210034.n3L0YV9i062814@svn.freebsd.org> References: <200904210034.n3L0YV9i062814@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, 21 Apr 2009, Pyun YongHyeon wrote:
> Clear IFF_DRV_OACTIVE flag if one of queued packets was transmitted.
> Previously it used to clear the flag only when the transmit queue
> is empty which may slow down Tx performance.
> While I'm here check whether driver is running and whether we can
> queue more packets in if_start handler. This fixes occasional
> watchdog timeouts.
Historically, IFF_DRV_OACTIVE was present to allow the driver to prevent calls
to if_start when the driver was already in the transmit steady state (i.e.,
interrupt-driven transmit). When the transmit interrupt saw it was out of
queued packets and interrupt-driven transmission was ending, it would clear
the flag so that future enqueues to the now-empty queue would generate an
if_start to trigger interrupt-driven transmission again. With that in mind --
are you sure this is the right change? Shouldn't the descriptor ring be
refilled when an interrupt comes in from the device after an appropriate
interval?
Robert N M Watson
Computer Laboratory
University of Cambridge
>
> Reported by: xer < xernet <> hotmail dot it >
> Tested by: xer < xernet <> hotmail dot it >
>
> Modified:
> head/sys/dev/xl/if_xl.c
>
> Modified: head/sys/dev/xl/if_xl.c
> ==============================================================================
> --- head/sys/dev/xl/if_xl.c Mon Apr 20 23:25:38 2009 (r191343)
> +++ head/sys/dev/xl/if_xl.c Tue Apr 21 00:34:31 2009 (r191344)
> @@ -2097,13 +2097,13 @@ xl_txeof(struct xl_softc *sc)
> m_freem(cur_tx->xl_mbuf);
> cur_tx->xl_mbuf = NULL;
> ifp->if_opackets++;
> + ifp->if_drv_flags &= ~IFF_DRV_OACTIVE;
>
> cur_tx->xl_next = sc->xl_cdata.xl_tx_free;
> sc->xl_cdata.xl_tx_free = cur_tx;
> }
>
> if (sc->xl_cdata.xl_tx_head == NULL) {
> - ifp->if_drv_flags &= ~IFF_DRV_OACTIVE;
> sc->xl_wdog_timer = 0;
> sc->xl_cdata.xl_tx_tail = NULL;
> } else {
> @@ -2540,6 +2540,9 @@ xl_start_locked(struct ifnet *ifp)
>
> XL_LOCK_ASSERT(sc);
>
> + if ((ifp->if_drv_flags & (IFF_DRV_RUNNING | IFF_DRV_OACTIVE)) !=
> + IFF_DRV_RUNNING)
> + return;
> /*
> * Check for an available queue slot. If there are none,
> * punt.
> @@ -2668,7 +2671,8 @@ xl_start_90xB_locked(struct ifnet *ifp)
>
> XL_LOCK_ASSERT(sc);
>
> - if (ifp->if_drv_flags & IFF_DRV_OACTIVE)
> + if ((ifp->if_drv_flags & (IFF_DRV_RUNNING | IFF_DRV_OACTIVE)) !=
> + IFF_DRV_RUNNING)
> return;
>
> idx = sc->xl_cdata.xl_tx_prod;
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?alpine.BSF.2.00.0904211022130.5067>