On Fri, Apr 05, 2024 at 08:51:48PM +0200, jan@openbsd.org wrote:
> On Sun, Mar 03, 2024 at 05:13:26PM +0100, jan@openbsd.org wrote:
> > On Thu, Feb 29, 2024 at 07:52:23AM -0600, Brian Conway wrote:
> > > Original thread, sorry for breaking threading: https://marc.info/?l=openbsd-tech&m=170523759201797
> > > 
> > > Greetings. Are you still looking for testing on this? I went to give
> > > it a spin today, but it no longer applies cleanly due to other recent
> > > vio(4) work.
> > 
> > I will update and resend the diff after the release of 7.5.  So, there
> > it more time for testing.  Before the 7.6 release.
> 
> Here is an updated version of the vio(4) TSO diff.  It takes some
> advantage of the ether_extract_headers() improvements.  vlan(4) in
> combination with vio(4) will work, but does not use TSO for now.  Thus,
> no performance increase with vlan traffic.
> 
> I tested it with IPv4 and IPv6 on a Linux KVM host.

With tcpbench sending for OpenBSD to Linux I get an increase of
factor 7.  OpenBSD and Linux guest run in the same KVM host.

> Tests are welcome.

No regression seen.

> ok?

some remarks:

> +static inline uint16_t
> +vio_tso_cksum(uint32_t cksum, uint16_t paylen)
> +{
> +	/* Add payload length */
> +	cksum += paylen;
> +
> +	/* Fold back to 16 bit */
> +	cksum += cksum >> 16;
> +
> +	return (uint16_t)(cksum);
> +}

This is a generic checksum update function that is not TSO specific.

Can we call it vio_cksum_update()?  The htons belongs in the caller,
not calle.  And the parameter is not a specific paylen, but a generic
update value.

> +void
> +vio_tx_offload(struct virtio_net_hdr *hdr, struct mbuf *m)
> +{
> +	struct ether_extracted ext;
> +
> +	/*
> +	 * Checksum Offload
> +	 */
> +
> +	if (!ISSET(m->m_pkthdr.csum_flags, M_TCP_CSUM_OUT) &&
> +	    !ISSET(m->m_pkthdr.csum_flags, M_UDP_CSUM_OUT))
> +		return;
> +
> +	ether_extract_headers(m, &ext);
> +	hdr->csum_start = sizeof(*ext.eh);
> +#if NVLAN > 0
> +	if (ext.evh)
> +		hdr->csum_start = sizeof(*ext.evh);
> +#endif
> +	if (ext.ip4 || ext.ip6)
> +		hdr->csum_start += ext.iphlen;
> +
> +	if (m->m_pkthdr.csum_flags & M_TCP_CSUM_OUT)
> +		hdr->csum_offset = offsetof(struct tcphdr, th_sum);
> +	else
> +		hdr->csum_offset = offsetof(struct udphdr, uh_sum);

Use ISSET consistently.

Can we be a bit more paranois here?

	if (ext.tcp && ISSET(m->m_pkthdr.csum_flags, M_TCP_CSUM_OUT)) 
		hdr->csum_offset = offsetof(struct tcphdr, th_sum);
	else if (ext.udp && ISSET(m->m_pkthdr.csum_flags, M_UDP_CSUM_OUT)) 
		hdr->csum_offset = offsetof(struct udphdr, uh_sum);
	else
		return;

And move the consistency check before modifying any hdr fields.

> +	hdr->flags = VIRTIO_NET_HDR_F_NEEDS_CSUM;
> +
> +	/*
> +	 * TCP Segmentation Offload
> +	 */
> +
> +	if (!ISSET(m->m_pkthdr.csum_flags, M_TCP_TSO))
> +		return;
> +
> +	hdr->hdr_len = hdr->csum_start + ext.tcphlen;
> +	hdr->gso_size = m->m_pkthdr.ph_mss;
> +
> +	if (ext.ip4)
> +		hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV4;
> +#ifdef INET6
> +	else if (ext.ip6)
> +		hdr->gso_type = VIRTIO_NET_HDR_GSO_TCPV6;
> +#endif
> +	else
> +		tcpstat_inc(tcps_outbadtso);

Here a return is missing.  Do no try TSO for non IP packets.  Put
the check, error counter and return before touching any hdr fields.
If ext.tcp == NULL, also report an error.

> +	/* VirtIO-Net need pseudo header cksum with IP-payload length for TSO */
> +	ext.tcp->th_sum = vio_tso_cksum(ext.tcp->th_sum,
> +	    htons(ext.iplen - ext.iphlen));
> +
> +	tcpstat_add(tcps_outpkttso,
> +	    (ext.paylen + m->m_pkthdr.ph_mss - 1) / m->m_pkthdr.ph_mss);
> +}
> +