From: Alexander Bluhm Subject: Re: revert m_defrag header align To: tech@openbsd.org Date: Mon, 26 Feb 2024 16:17:10 +0100 On Mon, Feb 26, 2024 at 03:24:17PM +0100, Claudio Jeker wrote: > On Fri, Feb 23, 2024 at 04:54:59PM +0100, Alexander Bluhm wrote: > > On Fri, Feb 23, 2024 at 12:32:13PM +0100, Claudio Jeker wrote: > > > On Fri, Feb 23, 2024 at 11:55:43AM +0100, Alexander Bluhm wrote: > > > > Hi, > > > > > > > > I think we should revert the diff that aligns the mbuf to IP header > > > > in m_defrag(). The cause of the change was that ether_extract_headers() > > > > crashed in em(4) when it was reading TCP header for offloading on > > > > sparc64. This has been fixed by using memcpy(). > > > > > > > > Now viommu_dvmamap_load_raw() on sparc64 calls iommu_iomap_insert_page() > > > > twice as often with overlapping pages. m_defrag() is intended as > > > > last resort to make DMA transfers to the hardware. Therefore page > > > > alingment is more important than IP header alignment. > > > > > > > > ok? > > > > > > This makes little sense. We offset a 1500 byte packet by 2 bytes inside a > > > 8k page. This should not result in extra page use. Something else is off > > > here. > > > > My printf debugging tells a different story. Debugging diff > > attached, so you can see what I was looking at. > > > > Note that I had to add a i > map->_dm_segcnt check to avoid that > > too many segments are loaded into DMA. This prevents the "iomap > > insert error: 12 for pa 0xded4e6000" errors I was seeing before. > > > > ixl supports only 8 segments. The driver first tries to load the > > 1500 byte packets combined in 2k clusters and fails. This mbuf > > chain in the TCP output buffer is a result of socket splicing small > > packets. TSO creates a 31922 bytes packet consisting of about 22 > > clusters. The first atempt to load it into DMA fails. > > > > _bus_dmamap_load_mbuf: j 9, i 9, _dm_segcnt 8, pktlen 31922 > > > > Then m_defrag creates one big cluster that contains the 31922 byte > > packet. ixl supports up to 8 DMA segments each 16k, at least this > > is what the driver says. The cluster is split into 3 segments. > > > > _bus_dmamap_load_mbuf: ds_addr 0x004eba0002, ds_len 8192, j 1, i 3, _dm_segcnt 8, pktlen 31922 > > _bus_dmamap_load_mbuf: ds_addr 0x004eba2002, ds_len 8192, j 1, i 3, _dm_segcnt 8, pktlen 31922 > > _bus_dmamap_load_mbuf: ds_addr 0x004eba4002, ds_len 15538, j 1, i 3, _dm_segcnt 8, pktlen 31922 > > > > Note that the segments start 2 byte after the page due to header > > alignment of the mbuf. That means that each segments covers two > > pages, the page for the remaining 2 byte is overlapping and loaded > > twice. > > > > viommu_dvmamap_load_raw: a 0x004eba0000, aend 0x004eba4000 > > viommu_dvmamap_load_raw: a 0x004eba2000, aend 0x004eba4000 > > viommu_dvmamap_load_raw: a 0x004eba2000, aend 0x004eba6000 > > viommu_dvmamap_load_raw: a 0x004eba4000, aend 0x004eba6000 > > viommu_dvmamap_load_raw: a 0x004eba4000, aend 0x004eba8000 > > viommu_dvmamap_load_raw: a 0x004eba6000, aend 0x004eba8000 > > > > With m_defrag header alignment reverted, we are back to 4 pages > > instead of 6. As the restrictions of ixl DMA for 64k TSO are tight, > > we should not waste resources here. Header alignment after m_defrag > > does not make sense for DMA. > > > > Unfortunately after fixing the two bugs, large TSO on ixl still has > > problems. Memory layout after socket splcing is the worst test > > case. Jan has tested ixl on a different sparc64 that uses iommu > > instead of viommu. There everything works. em and ix TSO works > > also on viommu. Your m_defrag alignment diff had no regressions, > > as it works for em and ix, and ixl on viommu was broken before. > > > > The critical corner case is TSO + ixl + viommu sparc64. > > > > To make progress in debugging I have to > > 1. revert m_defrag header align > > 2. limit _bus_dmamap_load_mbuf to map->_dm_segcnt > > 3. find the remaining bugs > > > > The problem is _bus_dmamap_load_mbuf() that has a bug. > > while (m) { > vaddr_t vaddr = mtod(m, vaddr_t); > long buflen = (long)m->m_len; > > len += buflen; > while (buflen > 0 && i < MAX_DMA_SEGS) { > paddr_t pa; > long incr; > > incr = min(buflen, NBPG); > > In our case this is a 64k mcluster with 31922. So buflen is bigger than > NBPG (8k) and so incr is set to 8192. I did put i > map->_dm_segcnt in the condition after the loop to detect too many segment use. But of course this is not the only bug. > Later on the code does: > > buflen -= incr; > vaddr += incr; > > This does not work if vaddr is not at the start of a page. > Changing the min() to > incr = min(buflen, PAGE_SIZE - ((u_long)vaddr & PGOFSET)); > may fix this. I still think we should switch m_defrag() back to aligned mbuf creation. Aligning to IP header does make sense when it comes to DMA transfers. The problem arises when calling ether_extract_headers() after m_defrag(). We have a memcpy workaround for that. Currently the m_defrag() align makes the situation worse, although _bus_dmamap_load_mbuf() does not work anyway. It seems noone ever did large DMA transfers with this code. > On the other hand most other archs implemented > _bus_dmamap_load_mbuf() as a wrapper around _dmamap_load_buffer() > so I wonder if sparc64 should do the same. > > Also the uio function in sparc64 has the same logic bug and same issue. > > I hate that we have so many versions of _bus_dmamap_load_mbuf() in our > tree since it makes it so much harder to adjust that code. > > -- > :wq Claudio