> On 8 Apr 2015, at 06:42, Stefan Fritsch <[email protected]> wrote: > > Hi, > > I have committed the vio diff with some tweaks to remove some more now > unused code. But I think this diff is actually incorrect: > > On Tue, 31 Mar 2015, Kimberley Manning wrote: >> Index: if_pcn.c >> =================================================================== >> RCS file: /cvs/src/sys/dev/pci/if_pcn.c,v >> retrieving revision 1.36 >> diff -u -p -r1.36 if_pcn.c >> --- if_pcn.c 14 Mar 2015 03:38:48 -0000 1.36 >> +++ if_pcn.c 27 Mar 2015 12:17:24 -0000 >> @@ -851,25 +851,23 @@ pcn_start(struct ifnet *ifp) >> * were short on resources. In this case, we'll copy >> * and try again. >> */ >> - if (bus_dmamap_load_mbuf(sc->sc_dmat, dmamap, m0, >> - BUS_DMA_WRITE|BUS_DMA_NOWAIT) != 0) { >> - MGETHDR(m, M_DONTWAIT, MT_DATA); >> - if (m == NULL) >> - break; >> - if (m0->m_pkthdr.len > MHLEN) { >> - MCLGET(m, M_DONTWAIT); >> - if ((m->m_flags & M_EXT) == 0) { >> - m_freem(m); >> - break; >> - } >> - } >> - m_copydata(m0, 0, m0->m_pkthdr.len, mtod(m, caddr_t)); >> - m->m_pkthdr.len = m->m_len = m0->m_pkthdr.len; >> - error = bus_dmamap_load_mbuf(sc->sc_dmat, dmamap, >> - m, BUS_DMA_WRITE|BUS_DMA_NOWAIT); >> - if (error) >> - break; >> - } >> + error = bus_dmamap_load_mbuf(sc->sc_dmat, dmamap, m0, >> + BUS_DMA_WRITE|BUS_DMA_NOWAIT); >> + switch (error) { >> + case 0: >> + break; >> + case EFBIG: >> + if ((error = m_defrag(m0, M_DONTWAIT)) == 0 && >> + (error = bus_dmamap_load_mbuf(sc->sc_dmat, >> dmamap, >> + m0, BUS_DMA_WRITE|BUS_DMA_NOWAIT)) == 0) >> + break; >> + >> + /* FALLTHROUGH */ >> + default: >> + IFQ_DEQUEUE(&ifp->if_snd, m0); >> + m_freem(m); > > m is always NULL. Therefore you leak m0 in the error path. You should > remove the m variable which is no longer necessary, and possibly rename m0 > to m. But the rename should probably be done in a second diff for easier > review. > >> + continue; >> + } > > Other drivers don't dequeue mbufs in out of mem situations. I think you > should just set IFF_OACTIVE and break out of the for loop (not shown in > the diff). The old code just does the break, but I think setting > IFF_OACTIVE would be correct here.
OACTIVE means the tx ring is full. it (generally) gets cleared by a driver when the tx ring is emptied. if the tx ring is empty but we set it when the system runs out of resources (ie, memory shortages, dma mappings, iommu slot exhaustion, solar flares, etc), then there's no event to clear OACTIVE with. if we cant map the mbuf we should drop it and move on. dlg > > >> >> /* >> * Ensure we have enough descriptors free to describe >> >> >
