On Fri, Feb 09, 2024 at 12:11:52PM +0100, Claudio Jeker wrote:
> On Fri, Feb 09, 2024 at 11:37:32AM +0100, Alexander Bluhm wrote:
> > Hi,
> > 
> > After a lot of debugging and testing with Hrvoje and mglocker@, we
> > have found the cause for occasional watchdog timeouts with em TSO
> > diff.
> > 
> > In this setup TCP packets are routed from ix(4) with LRO to em(4)
> > with TSO.  It happens that ix hardware coalesces packets that are
> > in total smaller than MTU.  In real live this is rare as either you
> > have large TCP packets in a row, or small TCP packets from time to
> > time.
> > 
> > Small packets that went though LRO have the TSO bit set.  But TSO
> > is not necessary as only few small TCP packets have been reassembled.
> > This confuses em hardware.  So clear TSO flag during interface
> > output path in that case.
> > 
> > This is only relevant when forwarding to hardware TSO.  Our stack
> > does not generate such packets and can handle them.  Diff below
> > prevents them to reach hardware TSO.
> > 
> > ok?
> 
> But shouldn't the system send out those small packets split up again?
> At least I think the idea is that the effect of LRO is reversed while
> sending out so that the same amount of packets are sent as without LRO/TSO.

The small packets in our case have 2 bytes payload after LRO.  We
instruct em to split them in 1 byte packets and that fails with
watchdog timeouts.  It works if I clear the TSO bit.

But you are right, my diff may break path MTU discovery.  So
m_pkthdr.len <= mtu is not ideal.  Is there a minmum TSO size
hardware can handle?  We might use that instead of MTU.

bluhm

> > Index: netinet/tcp_output.c
> > ===================================================================
> > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/tcp_output.c,v
> > diff -u -p -r1.141 tcp_output.c
> > --- netinet/tcp_output.c	26 Nov 2023 22:08:10 -0000	1.141
> > +++ netinet/tcp_output.c	8 Feb 2024 21:17:50 -0000
> > @@ -1360,6 +1360,12 @@ tcp_if_output_tso(struct ifnet *ifp, str
> >  	/* caller must fail later or fragment */
> >  	if (!ISSET((*mp)->m_pkthdr.csum_flags, M_TCP_TSO))
> >  		return 0;
> > +	/* send without hardware TSO if interface can handle packet size */
> > +	if ((*mp)->m_pkthdr.len <= mtu) {
> > +		CLR((*mp)->m_pkthdr.csum_flags, M_TCP_TSO);
> > +		return 0;
> > +	}
> > +	/* fragment or fail if interface cannot handle size after chopping */
> >  	if ((*mp)->m_pkthdr.ph_mss > mtu) {
> >  		CLR((*mp)->m_pkthdr.csum_flags, M_TCP_TSO);
> >  		return 0;
> > 
> 
> -- 
> :wq Claudio