From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: revert m_defrag header align
To: Alexander Bluhm <alexander.bluhm@gmx.net>
Cc: tech@openbsd.org
Date: Fri, 23 Feb 2024 12:32:13 +0100

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.

I can't look into this right now but will on Monday.
 
> bluhm
> 
> Index: kern/uipc_mbuf.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/kern/uipc_mbuf.c,v
> diff -u -p -r1.289 uipc_mbuf.c
> --- kern/uipc_mbuf.c	21 Feb 2024 09:28:29 -0000	1.289
> +++ kern/uipc_mbuf.c	23 Feb 2024 10:45:15 -0000
> @@ -545,29 +545,27 @@ m_purge(struct mbuf *m)
>   * mbuf chain defragmenter. This function uses some evil tricks to defragment
>   * an mbuf chain into a single buffer without changing the mbuf pointer.
>   * This needs to know a lot of the mbuf internals to make this work.
> + * The resulting mbuf is not aligned to IP header to assist DMA transfers.
>   */
>  int
>  m_defrag(struct mbuf *m, int how)
>  {
>  	struct mbuf *m0;
> -	unsigned int adj;
>  
>  	if (m->m_next == NULL)
>  		return (0);
>  
>  	KASSERT(m->m_flags & M_PKTHDR);
>  
> -	adj = mtod(m, unsigned long) & (sizeof(long) - 1);
>  	if ((m0 = m_gethdr(how, m->m_type)) == NULL)
>  		return (ENOBUFS);
> -	if (m->m_pkthdr.len + adj > MHLEN) {
> -		MCLGETL(m0, how, m->m_pkthdr.len + adj);
> +	if (m->m_pkthdr.len > MHLEN) {
> +		MCLGETL(m0, how, m->m_pkthdr.len);
>  		if (!(m0->m_flags & M_EXT)) {
>  			m_free(m0);
>  			return (ENOBUFS);
>  		}
>  	}
> -	m0->m_data += adj;
>  	m_copydata(m, 0, m->m_pkthdr.len, mtod(m0, caddr_t));
>  	m0->m_pkthdr.len = m0->m_len = m->m_pkthdr.len;
>  
> @@ -586,9 +584,9 @@ m_defrag(struct mbuf *m, int how)
>  		memcpy(&m->m_ext, &m0->m_ext, sizeof(struct mbuf_ext));
>  		MCLINITREFERENCE(m);
>  		m->m_flags |= m0->m_flags & (M_EXT|M_EXTWR);
> -		m->m_data = m->m_ext.ext_buf + adj;
> +		m->m_data = m->m_ext.ext_buf;
>  	} else {
> -		m->m_data = m->m_pktdat + adj;
> +		m->m_data = m->m_pktdat;
>  		memcpy(m->m_data, m0->m_data, m0->m_len);
>  	}
>  	m->m_pkthdr.len = m->m_len = m0->m_len;
> 

-- 
:wq Claudio