From: Jan Klemkow <jan@openbsd.org>
Subject: Re: igc(4): fix recv. jumbo frames
To: tech@openbsd.org
Cc: Alexander Bluhm <bluhm@openbsd.org>, Theo de Raadt <deraadt@openbsd.org>
Date: Thu, 1 Aug 2024 18:48:17 +0200

On Wed, Jul 31, 2024 at 10:39:25AM -0600, Theo de Raadt wrote:
> Alexander Bluhm <bluhm@openbsd.org> wrote:
> 
> > On Tue, Jul 30, 2024 at 08:12:08AM +0200, Jan Klemkow wrote:
> > > The DMA mapping and allocation of mbufs for jumbo frames uses different
> > > sizes.  Thus, we are ending up with corrupt mbufs, which leads to panics
> > > in later part of the TCP/IP stack:
> > > 
> > > panic: kernel diagnostic assertion "M_DATABUF(m) + M_SIZE(m) >= (m->m_data + m->m_len)"
> > > failed: file "/usr/src/sys/kern/uipc_mbuf.c", line 1364
> > > 
> > > With the following diff, we use the same size for mapping and
> > > allocation.
> > > 
> > > ok?
> > 
> > I have tested this diff successfully.  Before jumbo frames with
> > igc(4) paniced, now TCP is faster.
> > 
> > But are we allocating too much in the non-jumbo case?
> >     sc->rx_mbuf_sz = MAX_JUMBO_FRAME_SIZE + ETHER_ALIGN;
> >     m = MCLGETL(NULL, M_DONTWAIT, sc->rx_mbuf_sz)
> > 
> > This means for each receive packet we allocate a 16k mbuf cluster.
> > Usually only 1500 byte are needed.
> > 
> > ix(4) has this code and manages to chain the mbufs correctly.
> >     /* Use 2k clusters, even for jumbo frames */
> >     sc->rx_mbuf_sz = MCLBYTES + ETHER_ALIGN;
> 
> This hardware chains fragments in the ring?
> 
> It's a tradeoff.  jumbos are rare, so why reserve so much memory to them
> on hardware which can do better.

I oriented my diff at the FreeBSD code.  Also, the Linux code looks like
as they don't use scatter/gather DMA.

But, It works with the following diff below.

The extra size of ETHER_ALIGN seems be wrong.  It also leads to panics
caused by corrupted mbufs.  FreeBSD just use MCLBYTES for rx mbufs size.
Also NetBSD removed ETHER_ALIGN from their copy of our driver [1].

[1]: https://github.com/NetBSD/src/commit/fb38d839b48b9b6204dbbee1672454d6e719ba01

ok?

bye,
Jan

Index: dev/pci/if_igc.c
===================================================================
RCS file: /mount/openbsd/cvs/src/sys/dev/pci/if_igc.c,v
diff -u -p -r1.25 if_igc.c
--- dev/pci/if_igc.c	24 May 2024 06:02:53 -0000	1.25
+++ dev/pci/if_igc.c	1 Aug 2024 15:45:58 -0000
@@ -881,7 +881,7 @@ igc_init(void *arg)
 	}
 	igc_initialize_transmit_unit(sc);
 
-	sc->rx_mbuf_sz = MCLBYTES + ETHER_ALIGN;
+	sc->rx_mbuf_sz = MCLBYTES;
 	/* Prepare receive descriptors and buffers. */
 	if (igc_setup_receive_structures(sc)) {
 		printf("%s: Could not setup receive structures\n",
@@ -2159,7 +2159,7 @@ igc_allocate_receive_buffers(struct igc_
 	rxbuf = rxr->rx_buffers;
 	for (i = 0; i < sc->num_rx_desc; i++, rxbuf++) {
 		error = bus_dmamap_create(rxr->rxdma.dma_tag,
-		    MAX_JUMBO_FRAME_SIZE, 1, MAX_JUMBO_FRAME_SIZE, 0,
+		    MAX_JUMBO_FRAME_SIZE, IGC_MAX_SCATTER, MCLBYTES, 0,
 		    BUS_DMA_NOWAIT, &rxbuf->map);
 		if (error) {
 			printf("%s: Unable to create RX DMA map\n",