On Thu, Feb 15, 2024 at 10:23:49AM +0100, Claudio Jeker wrote:
> Accessing the packet data inside the rxeof function is not ideal.
> Mainly we fault in DMA memory into the CPU cache and then do the same
> again later on the CPU handling the data.
> In an ideal world the driver would only touch the metadata but never
> access the DMA memory so that there is no cache pollution. As usual
> reality is far from an ideal world.

We need the information to tell the hardware how to offload.  Loading
the values to the CPU to configure the hardware seems to be faster
than not doing offloading.

An alternative would be to store relevant fields in mbuf header and
pass it down from TCP, though IP and all the pseudo devices.  That
sounds quite fragile as every layer has to adjust correctly.

> I wonder if other LRO capable devices will need similar changes and if in
> that case this should be moved to the generic input processing.

I would say ether_extract_headers() is such a generic function that
does just enough analysis for hardware offloading.  On top it does
sanity length checks do avoid invalid programming of the network
hardware.

One could argue that ether_extract_headers() is doing too much for
drivers that don't need all the details.  On the other hand they
also benefit from validated length fields.

> Diff looks reasonable, so OK claudio@.

also OK bluhm@

> > Index: if_ix.c
> > ===================================================================
> > RCS file: /cvs/src/sys/dev/pci/if_ix.c,v
> > diff -u -p -r1.208 if_ix.c
> > --- if_ix.c	14 Feb 2024 22:41:48 -0000	1.208
> > +++ if_ix.c	15 Feb 2024 05:47:22 -0000
> > @@ -3260,20 +3260,24 @@ ixgbe_rxeof(struct rx_ring *rxr)
> >  
> >  			if (pkts > 1) {
> >  				struct ether_extracted ext;
> > -				uint32_t hdrlen, paylen;
> > +				uint32_t paylen;
> >  
> > -				/* Calculate header size. */
> > +				/*
> > +				 * Calculate the payload size:
> > +				 *
> > +				 * The packet length returned by the NIC
> > +				 * (sendmp->m_pkthdr.len) can contain
> > +				 * padding, which we don't want to count
> > +				 * in to the payload size.  Therefore, we
> > +				 * calculate the real payload size based
> > +				 * on the total ip length field (ext.iplen).
> > +				 */
> >  				ether_extract_headers(sendmp, &ext);
> > -				hdrlen = sizeof(*ext.eh);
> > -#if NVLAN > 0
> > -				if (ISSET(sendmp->m_flags, M_VLANTAG) ||
> > -				    ext.evh)
> > -					hdrlen += ETHER_VLAN_ENCAP_LEN;
> > -#endif
> > +				paylen = ext.iplen;
> >  				if (ext.ip4 || ext.ip6)
> > -					hdrlen += ext.iphlen;
> > +					paylen -= ext.iphlen;
> >  				if (ext.tcp) {
> > -					hdrlen += ext.tcphlen;
> > +					paylen -= ext.tcphlen;
> >  					tcpstat_inc(tcps_inhwlro);
> >  					tcpstat_add(tcps_inpktlro, pkts);
> >  				} else {
> > @@ -3285,8 +3289,6 @@ ixgbe_rxeof(struct rx_ring *rxr)
> >  				 * mark it as TSO, set a correct mss,
> >  				 * and recalculate the TCP checksum.
> >  				 */
> > -				paylen = sendmp->m_pkthdr.len > hdrlen ?
> > -				    sendmp->m_pkthdr.len - hdrlen : 0;
> >  				if (ext.tcp && paylen >= pkts) {
> >  					SET(sendmp->m_pkthdr.csum_flags,
> >  					    M_TCP_TSO);
> > 
> 
> -- 
> :wq Claudio