alpha and em mtu

[Sten Spans]

On Tue, 23 Nov 2004, John Baldwin wrote:

> On Monday 22 November 2004 10:09 pm, Sten Spans wrote:
>> On Tue, 23 Nov 2004, Sten Spans wrote:
>>>> doesn't seem to print anything, but ...
>>>>
>>>> if_em.c
>>>>   2442
>>>>   2443         /*if (ifp->if_mtu <= ETHERMTU) { */
>>>>   2444                 m_adj(mp, ETHER_ALIGN);
>>>>   2445         /*} */
>>>>   2446
>>>>
>>>> does seem to fix the crash, also trashes the performance,
>>>> but that's another matter. It looks like mbuf alignment is
>>>> needed, if_bge seems to provide reasonable examples.
>>>
>>> And looking at netbsd/openbsd clarifies the whole issue,
>>>
>>> #ifdef __STRICT_ALIGNMENT
>>> 			/*
>>> 			 * The ethernet payload is not 32-bit aligned when
>>> 			 * Jumbo packets are enabled, so on architectures
>>> with
>>> 			 * strict alignment we need to shift the entire
>>> packet
>>> 			 * ETHER_ALIGN bytes. Ugh.
>>> 			 */
>>>
>>>
>>> This diff probably should be merged.
>>> http://www.openbsd.org/cgi-bin/cvsweb/src/sys/dev/pci/if_em.c.diff?r1=1.2
>>> 2&r2=1.23
>>>
>>> Although I don't know wether freebsd has the STRICT_ALIGNMENT define.
>>
>> This is an initial patch based on the openbsd code,
>> which solves the if_em issue, and seems to give ok performance
>> ( note to self: turn off debugging when testing network performance ).
>>
>> Comments welcome.
>
> We do need some kind of __STRICT_ALIGNMENT macro I think if we don't already
> have one as archs other than i386 might not need the alignment.

bge needs some fixing as well then:

    2773 #ifndef __i386__
    2774                 /*
    2775                  * The i386 allows unaligned accesses, but for other
    2776                  * platforms we must make sure the payload is aligned.
    2777                  */
    2778                 if (sc->bge_rx_alignment_bug) {
    2779                         bcopy(m->m_data, m->m_data + ETHER_ALIGN,
    2780                             cur_rx->bge_len);
    2781                         m->m_data += ETHER_ALIGN;
    2782                 }
    2783 #endif

Not sure if this a better fix for the same problem...

> At the least
> there could be a block near the top of if_em.h that was:
>
> #if defined(__alpha__)
> #define	STRICT_ALIGNMENT
> #endif
>
> and other architectures could be fixed by simply adding another
> 'defined(__foo__)' clause without having to change the ifdef and comments
> down in the code itself.

I've attatched an updated version of the patch, with your comment
included.

> As to the correctness of the em(4) change,
> hopefully Robert can speak to that.

Have you had time to look at this robert ?

-- 
Sten Spans

"There is a crack in everything, that's how the light gets in."
Leonard Cohen - Anthem
-------------- next part --------------
--- if_em.c.orig	Tue Nov 23 03:03:23 2004
+++ if_em.c	Sun Nov 28 00:53:51 2004
@@ -2784,8 +2784,57 @@
 			/* Assign correct length to the current fragment */
 			mp->m_len = len;
 
+#ifndef __STRICT_ALIGNMENT
+			/*
+			 * The ethernet payload is not 32-bit aligned when
+			 * Jumbo packets are enabled, so on architectures with
+			 * strict alignment we need to shift the entire packet
+			 * ETHER_ALIGN bytes. Ugh.
+			 */
+			if (ifp->if_mtu > ETHERMTU) {
+				unsigned char tmp_align_buf[ETHER_ALIGN];
+				int tmp_align_buf_len = 0;
+
+				if (prev_len_adj > adapter->align_buf_len)
+					prev_len_adj -= adapter->align_buf_len;
+				else
+					prev_len_adj = 0;
+
+				if (mp->m_len > MCLBYTES - ETHER_ALIGN) {
+					bcopy(mp->m_data +
+					    (MCLBYTES - ETHER_ALIGN),
+					    &tmp_align_buf,
+					    ETHER_ALIGN);
+					tmp_align_buf_len = mp->m_len -
+					    (MCLBYTES - ETHER_ALIGN);
+					mp->m_len -= ETHER_ALIGN;
+				} 
+
+				if (mp->m_len) {
+					bcopy(mp->m_data,
+					    mp->m_data + ETHER_ALIGN,
+					    mp->m_len);
+					if (!adapter->align_buf_len)
+						mp->m_data += ETHER_ALIGN;
+				}
+
+				if (adapter->align_buf_len) {
+					mp->m_len += adapter->align_buf_len;
+					bcopy(&adapter->align_buf,
+					    mp->m_data,
+					    adapter->align_buf_len);
+				}
+
+				if (tmp_align_buf_len) 
+					bcopy(&tmp_align_buf,
+					    &adapter->align_buf,
+					    tmp_align_buf_len);
+				adapter->align_buf_len = tmp_align_buf_len;
+			}
+#endif /* __STRICT_ALIGNMENT */
+
 			if (adapter->fmp == NULL) {
-				mp->m_pkthdr.len = len;
+				mp->m_pkthdr.len = mp->m_len;
 				adapter->fmp = mp;	 /* Store the first mbuf */
 				adapter->lmp = mp;
 			} else {
@@ -2801,7 +2850,7 @@
 				}
 				adapter->lmp->m_next = mp;
 				adapter->lmp = adapter->lmp->m_next;
-				adapter->fmp->m_pkthdr.len += len;
+				adapter->fmp->m_pkthdr.len += mp->m_len;
 			}
 
                         if (eop) {
--- if_em.h.orig	Wed Nov 10 03:18:52 2004
+++ if_em.h	Sun Nov 28 00:53:13 2004
@@ -79,6 +79,10 @@
 
 #include <dev/em/if_em_hw.h>
 
+#if defined(__alpha__)
+#define		__STRICT_ALIGNMENT
+#endif
+
 /* Tunables */
 
 /*
@@ -346,6 +350,12 @@
 	int             io_rid;
 	u_int8_t        unit;
 	struct mtx	mtx;
+
+#ifndef __STRICT_ALIGNMENT
+	/* Used for carrying forward alignment adjustments */
+	unsigned char	align_buf[ETHER_ALIGN];	/* tail of unaligned packet */
+	u_int8_t	align_buf_len;		/* bytes in tail */
+#endif /* __STRICT_ALIGNMENT */
 
 	/* Info about the board itself */
 	u_int32_t       part_num;