Re: add m_defrag to pcn driver
On Wednesday 08 April 2015 09:48:14, David Gwynne wrote: +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. Sounds reasonable but many drivers (e.g. em, ix, bge) don't do that but set OACTIVE in this case. Is there a reason for that? From a casual glance at the source it is not clear to me that they have some other way to recover. Some have a watchdog that will reset the device after some time.
Re: add m_defrag to pcn driver
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 - 1.36
+++ if_pcn.c 27 Mar 2015 12:17:24 -
@@ -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.
/*
* Ensure we have enough descriptors free to describe
Re: add m_defrag to pcn driver
On 8 Apr 2015, at 06:42, Stefan Fritsch s...@sfritsch.de 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 - 1.36
+++ if_pcn.c 27 Mar 2015 12:17:24 -
@@ -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
Re: add m_defrag to pcn driver
Martin Pieuchot mpi at openbsd.org writes: On 31/03/15(Tue) 21:56, Kimberley Manning wrote: Hi, This diff makes the pcn driver use m_defrag for fragmented mbuf chains, I like this kind of cleanups. As for vio(4) could you try the diff or are you looking for testers? Thanks. I've tried both the vio and pcn diffs and they worked for me, so dlg told me to write here. I'm not sure what happens to diffs after this point to be honest. Are you after something specific or are you changing the drivers for coherency reason? Just coherency.
Re: add m_defrag to pcn driver
On 31/03/15(Tue) 21:56, Kimberley Manning wrote:
Hi,
This diff makes the pcn driver use m_defrag for fragmented mbuf chains,
I like this kind of cleanups. As for vio(4) could you try the diff or
are you looking for testers?
Are you after something specific or are you changing the drivers for
coherency reason?
cheers,
Kim
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 - 1.36
+++ if_pcn.c 27 Mar 2015 12:17:24 -
@@ -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);
+ continue;
+}
/*
* Ensure we have enough descriptors free to describe
Re: add m_defrag to pcn driver
On Wednesday 01 April 2015 12:46:47, Kimberley Manning wrote: This diff makes the pcn driver use m_defrag for fragmented mbuf chains, I like this kind of cleanups. As for vio(4) could you try the diff or are you looking for testers? Thanks. I've tried both the vio and pcn diffs and they worked for me, so dlg told me to write here. I'm not sure what happens to diffs after this point to be honest. I can take a look at the vio diff and commit it in a few days. BTW, is there a way to trigger heavily fragmented mbufs in order to test the code path?
add m_defrag to pcn driver
Hi,
This diff makes the pcn driver use m_defrag for fragmented mbuf chains,
cheers,
Kim
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.c14 Mar 2015 03:38:48 - 1.36
+++ if_pcn.c27 Mar 2015 12:17:24 -
@@ -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);
+ continue;
+}
/*
* Ensure we have enough descriptors free to describe
