On Wed, May 26, 2010 at 10:06:54AM +0100, Stuart Henderson wrote:
> On 2010/05/21 23:54, Chris Bayly wrote:
> > Okay, I believe I've finally simplified this down to something manageable,
> > and hopefully a heck of alot more repeatable.
> >
> > I have a second Alix 3D3 board running -current as of the 19th.
> >
> > I can make it start leaking mbufs with the following setup.
> >
> > - Completely bone stock OpenBSD install.
> > - At install time setup one interface (vr0) as dhcp
> > - After install, issue the following commands:
> >
> > ifconfig vr1 up # Note: no cable connected on vr1
> > ifconfig bridge0 add vr0
> > ifconfig bridge0 add vr1
> > ifconfig bridge0 up
> >
> > netstat -m # will now show an increasing number of mbuf 2048 byte clusters
> > # being used
>
> in vr_start() [if_vr.c:1228] we have this
>
> if (ifp->if_flags & IFF_OACTIVE || sc->vr_link == 0)
> return;
>
> the sc->vr_link check was added to prevent watchdog timeouts,
> but because these frames aren't transmitted we have a leak.
>
> not sure if this is the best way, but it seems to do the trick:
>
> Index: if_vr.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/pci/if_vr.c,v
> retrieving revision 1.105
> diff -u -p -r1.105 if_vr.c
> --- if_vr.c 19 May 2010 15:27:35 -0000 1.105
> +++ if_vr.c 26 May 2010 09:05:51 -0000
> @@ -1225,7 +1225,7 @@ vr_start(struct ifnet *ifp)
>
> sc = ifp->if_softc;
>
> - if (ifp->if_flags & IFF_OACTIVE || sc->vr_link == 0)
> + if (ifp->if_flags & IFF_OACTIVE)
> return;
>
> cur_tx = sc->vr_cdata.vr_tx_prod;
> @@ -1233,6 +1233,11 @@ vr_start(struct ifnet *ifp)
> IFQ_DEQUEUE(&ifp->if_snd, m_head);
> if (m_head == NULL)
> break;
> + /* transmitting without link results in watchdog timeouts */
> + if (sc->vr_link == 0) {
> + m_freem(m_head);
> + return;
> + }
>
> /* Pack the data into the descriptor. */
> if (vr_encap(sc, cur_tx, m_head)) {
>
Hmm. The IFQ should have a limit and when reaching that packets will be
dropped. So after 256 packets no more packets should leak.
Unless something is calling IF_ENQUEUE() without checking IF_QFULL() first.
Ugh, the these ifq makros are stupid.
Still I think the current behaviour is strange and your diff makes more
sense. Any idea why vr(4) is using IFQ_DEQUEUE() + IF_PREPEND instead of
doing the IFQ_POLL(), IFQ_DEQUEUE() dance?
--
:wq Claudio
