From owner-freebsd-current@FreeBSD.ORG Thu May 14 21:27:30 2009 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 8243010658FA for ; Thu, 14 May 2009 21:27:30 +0000 (UTC) (envelope-from pisymbol@gmail.com) Received: from yx-out-2324.google.com (yx-out-2324.google.com [74.125.44.28]) by mx1.freebsd.org (Postfix) with ESMTP id 20F138FC32 for ; Thu, 14 May 2009 21:27:29 +0000 (UTC) (envelope-from pisymbol@gmail.com) Received: by yx-out-2324.google.com with SMTP id 8so875352yxb.13 for ; Thu, 14 May 2009 14:27:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=domainkey-signature:mime-version:received:in-reply-to:references :date:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=do9gIMNJfLu8Vx3CCktL6rBx+/XmjZqQ5O+j+lUUPYo=; b=rBAxvZVr3ymJXfxeJTqp0P9cOvcQq41Gh+sPk9CqhLh8TKfwLt0G9/8p/G8CrcGp/p Uq3yGYsTckNrchOmBR5UCzoOVToqQdQf4eV46M7+8gqYGcwd4h6C78/j+RMFKhfdzA6x lHXCiY1714zz+fvDdqbFOB4Xh/uomvAOb2/ec= DomainKey-Signature: a=rsa-sha1; c=nofws; d=gmail.com; s=gamma; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type:content-transfer-encoding; b=oTdTtKbmJ67roUM3vWfLUJV5lNud0K6s+2vx9L2aeIyRq0qWvrDWiYWxsEqp4034Lu LhAWogI/rz5+iu+8TzyzF/s8DwEcEDbEZApERkDCUWanqjdDSX6fGCzh/tABqYhtTsaC nhjo71aX8p+Dn5gO+vYjVaKGcy/0lK4h8LUrk= MIME-Version: 1.0 Received: by 10.100.215.17 with SMTP id n17mr3713152ang.87.1242336449300; Thu, 14 May 2009 14:27:29 -0700 (PDT) In-Reply-To: <200905141700.40439.jhb@freebsd.org> References: <3c0b01820905141202w113966dp4bfbab73d84d585@mail.gmail.com> <4A0C7544.6010304@delphij.net> <200905141700.40439.jhb@freebsd.org> Date: Thu, 14 May 2009 17:27:29 -0400 Message-ID: <3c0b01820905141427i7b858504m1ab74fd49882716c@mail.gmail.com> From: Alexander Sack To: John Baldwin Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Cc: freebsd-current@freebsd.org, d@delphij.net Subject: Re: Broadcom bge(4) panics while shutting down X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 14 May 2009 21:27:32 -0000 On Thu, May 14, 2009 at 5:00 PM, John Baldwin wrote: > On Thursday 14 May 2009 3:47:16 pm Xin LI wrote: >> Hi, Alexander, >> >> Alexander Sack wrote: >> > Hello: >> > >> > Under heavy traffic (100% utilization GIGE on a 2 port BGE card) >> > running BGE CURRENT driver I see panics on shutdown. =A0The reason is >> > because bge_rxeof() while processing its RX ring of BD's drops the >> > softc lock when it hands it off to its input function. =A0If bge_stop(= ) >> > is waiting for it, it will then proceed to acquire lock and then >> > quiesce the hardware (reseting the card, clearing out BDs etc.). =A0On= ce >> > bge_stop() releases the softc lock, then bge_rxeof() under an >> > interrupt context (no polling here) will reacquire and continue to >> > process the ring which is a bad idea. =A0It should check to see if the >> > card is still running before continuing processing BDs (i.e. once >> > IF_DRV_RUNNING has been reset by bge_stop(), bge_rxeof() is done, bail >> > out). >> > >> > Here is my first go around with this patch: >> > >> > >> > -- if_bge.c.CURRENT 2009-05-14 14:39:39.000000000 -0400 >> > +++ if_bge.c =A0 =A0 =A0 =A02009-05-14 14:39:24.000000000 -0400 >> > @@ -3081,6 +3081,10 @@ >> > =A0 =A0 =A0 =A0 =A0 =A0 uint16_t =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0vlan_t= ag =3D 0; >> > =A0 =A0 =A0 =A0 =A0 =A0 int =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ha= ve_tag =3D 0; >> > >> > + =A0 =A0 =A0 =A0 =A0 if (!(ifp->if_drv_flags & IFF_DRV_RUNNING)) { >> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return; >> > + =A0 =A0 =A0 =A0 =A0 } >> > + >> > =A0#ifdef DEVICE_POLLING >> > =A0 =A0 =A0 =A0 =A0 =A0 if (ifp->if_capenable & IFCAP_POLLING) { >> > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (sc->rxcycles <=3D 0) >> > >> > >> > This prevents any panics during shutdown under heavy load and AS IT >> > TURNS out (I feel stupid for not looking) that em(4) already had this >> > check in its em_rxeof() function (right at the top of the loop). =A0I'= m >> > more than happy changing it to the em style but above seems reasonable >> > to me though I have to verify there isn't anything missing off the >> > loop from a hardware standpoint (I don't think so because bge_stop() >> > did all the dirty work so I believe touching any registers after that >> > from bge_rxeof() is a bad idea). >> > >> > Preliminary testing shows no more panics start and stopping ports >> > under heavy load (panics were almost immediate otherwise). >> > >> > Thoughts? >> >> I think this would solve the problem but I'm not sure whether this would >> increase some overhead on the RX path. =A0It seems that there is a race >> between bge_release_resources() and bge_intr(), I mean, it might be a >> good idea to "drain" bge_intr() instead? > > Usually just detach() drains the interrupt handler. =A0However, an 'ifcon= fig > bge0 down' could probably provoke this as well. =A0I would probably do th= e > check right after re-acquiring the lock at the bottom of the loop before > touching anything else. Yea John, you got a point about that. I submitted the patch with the check in the while logic thinking that which I BELIEVE is functionality equivalent (don't ask me which one is faster), i.e. as soon as we require it, check it since bge_stop() might have reset it. If you get a chance, can you look at the PR and let me know if you think it looks good? I really want this fixed in 7.x to be honest since its a pain in the headache (I was working on another subsystem when I ran into this). -aps