em(4) testers needed

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

em(4) testers needed

Brad Smith-14
The following diff needs to be tested by anyone using an em(4)
interface on a strict alignment architecture... alpha, armish,
macppc, sparc64, hppa, even if you are NOT using Jumbo's. That's
right, that means everyone with a strict alignment system. Please
test this and get back to me letting me know how it goes.

Note: The diff has not been included in the latest snaps.


As noted in the following PR, VLANs currently do not work with an em(4)
interface with Jumbos in use on a strict alignment architecture.. in
this case macppc, though I can reproduce this on sparc64.

http://cvs.openbsd.org/cgi-bin/query-pr-wrapper?full=yes&numbers=5185

I asked jason@ about the issue and he is of the opinion that the technique
used in the commit in 1.131 is pretty neat, but most likely isn't
providing proper alignment for the VLAN layer. The symptom is that
VLAN tagged packets of a size of 2046 or larger arriving on an em(4)
interface will be corrupted.

The following diff removes the new technique being used and reverts
back to the original alignment code, but has it check for the max
receive frame size instead of the MTU, so the fixup code is actually
used when appropriate, MRU != MTU.

--
revision 1.131
date: 2006/05/31 02:09:18;  author: brad;  state: Exp;  lines: +56 -61
fix Jumbo frames on strict alignment architectures by allocating a new mbuf and
copying the Ethernet header to the new mbuf. The new mbuf is then prepended
into the existing mbuf chain.

>From FreeBSD

ok reyk@ pascoe@ jason@
--

I have tested this on sparc64 with -current and the PR submitter has tested
this on macppc with an equivalent diff for 3.9.


Index: if_em.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/if_em.c,v
retrieving revision 1.146
diff -u -p -r1.146 if_em.c
--- if_em.c 22 Aug 2006 15:51:18 -0000 1.146
+++ if_em.c 29 Aug 2006 23:21:23 -0000
@@ -151,9 +151,6 @@ void em_txeof(struct em_softc *);
 int  em_allocate_receive_structures(struct em_softc *);
 int  em_allocate_transmit_structures(struct em_softc *);
 void em_rxeof(struct em_softc *, int);
-#ifdef __STRICT_ALIGNMENT
-void em_fixup_rx(struct em_softc *);
-#endif
 void em_receive_checksum(struct em_softc *, struct em_rx_desc *,
  struct mbuf *);
 void em_transmit_checksum_setup(struct em_softc *, struct mbuf *,
@@ -2435,6 +2432,55 @@ em_rxeof(struct em_softc *sc, int count)
  /* Assign correct length to the current fragment */
  mp->m_len = len;
 
+#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.
+ */
+ if (sc->hw.max_frame_size > (MCLBYTES - ETHER_ALIGN)) {
+ unsigned char tmp_align_buf[ETHER_ALIGN];
+ int tmp_align_buf_len = 0;
+
+ if (prev_len_adj > sc->align_buf_len)
+ prev_len_adj -= sc->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 (!sc->align_buf_len)
+ mp->m_data += ETHER_ALIGN;
+ }
+
+ if (sc->align_buf_len) {
+ mp->m_len += sc->align_buf_len;
+ bcopy(&sc->align_buf,
+    mp->m_data,
+    sc->align_buf_len);
+ }
+
+ if (tmp_align_buf_len)
+ bcopy(&tmp_align_buf,
+    &sc->align_buf,
+    tmp_align_buf_len);
+ sc->align_buf_len = tmp_align_buf_len;
+ }
+#endif /* __STRICT_ALIGNMENT */
+
  if (sc->fmp == NULL) {
  mp->m_pkthdr.len = mp->m_len;
  sc->fmp = mp; /* Store the first mbuf */
@@ -2460,10 +2506,6 @@ em_rxeof(struct em_softc *sc, int count)
  ifp->if_ipackets++;
  em_receive_checksum(sc, current_desc,
     sc->fmp);
-#ifdef __STRICT_ALIGNMENT
- if (sc->hw.max_frame_size > (MCLBYTES - ETHER_ALIGN))
- em_fixup_rx(sc);
-#endif
  m = sc->fmp;
  sc->fmp = NULL;
  sc->lmp = NULL;
@@ -2511,49 +2553,6 @@ em_rxeof(struct em_softc *sc, int count)
  i = sc->num_rx_desc - 1;
  E1000_WRITE_REG(&sc->hw, RDT, i);
 }
-
-#ifdef __STRICT_ALIGNMENT
-/*
- * When Jumbo frames are enabled we should realign the entire payload on
- * strict alignment architecures. This is a serious design mistake of the
- * 8254x chipset as it nullifies DMA operations. 8254x allows the RX buffer
- * size to be 2048/4096/8192/16384. What we really want is 2048 - ETHER_ALIGN
- * to align its payload. On non strict alignment architectures 8254x still
- * performs unaligned memory access which will reduce the performance too. To
- * avoid copying over an entire frame to align, we allocate a new mbuf and
- * copy the Ethernet header to the new mbuf. The new mbuf is then prepended
- * into the existing mbuf chain.
- *
- * Be aware, best performance of the 8254x chipset is achived only when Jumbo
- * frames are not used at all on strict alignment architectures.
- */
-void
-em_fixup_rx(struct em_softc *sc)
-{
- struct mbuf *m, *n;
-
- m = sc->fmp;
- if (m->m_len <= (MCLBYTES - ETHER_HDR_LEN)) {
- bcopy(m->m_data, m->m_data + ETHER_HDR_LEN, m->m_len);
- m->m_data += ETHER_HDR_LEN;
- } else {
- MGETHDR(n, M_DONTWAIT, MT_DATA);
- if (n != NULL) {
- bcopy(m->m_data, n->m_data, ETHER_HDR_LEN);
- m->m_data += ETHER_HDR_LEN;
- m->m_len -= ETHER_HDR_LEN;
- n->m_len = ETHER_HDR_LEN;
- M_MOVE_PKTHDR(n, m);
- n->m_next = m;
- sc->fmp = n;
- } else {
- sc->dropped_pkts++;
- m_freem(sc->fmp);
- sc->fmp = NULL;
- }
- }
-}
-#endif
 
 /*********************************************************************
  *
Index: if_em.h
===================================================================
RCS file: /cvs/src/sys/dev/pci/if_em.h,v
retrieving revision 1.27
diff -u -p -r1.27 if_em.h
--- if_em.h 4 Aug 2006 14:25:24 -0000 1.27
+++ if_em.h 29 Aug 2006 23:21:23 -0000
@@ -310,6 +310,12 @@ struct em_softc {
  void *sc_powerhook;
  void *sc_shutdownhook;
 
+#ifdef __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;
  u_int8_t link_active;

Reply | Threaded
Open this post in threaded view
|

Re: em(4) testers needed

Theo de Raadt
> The following diff needs to be tested by anyone using an em(4)
> interface on a strict alignment architecture... alpha, armish,
> macppc, sparc64, hppa, even if you are NOT using Jumbo's. That's
> right, that means everyone with a strict alignment system. Please
> test this and get back to me letting me know how it goes.

No, that is not correct!

This diff needs to be tested on EVERY ARCHITECTURE with such an
interface, not just those Brad lists.