From: Alexander Bluhm <bluhm@openbsd.org>
Subject: Re: vio(4): use hardware offload with vlan(4)
To: Jan Klemkow <jan@openbsd.org>
Cc: sf@openbsd.org, tech@openbsd.org
Date: Sun, 9 Jun 2024 02:12:39 +0200

On Fri, Jun 07, 2024 at 04:04:52PM +0200, Jan Klemkow wrote:
> On Fri, Jun 07, 2024 at 08:39:44AM +0200, sf@openbsd.org wrote:
> > On Thu, 6 Jun 2024, Jan Klemkow wrote:
> > On my system, this diff speeds up sending from a VM to the host with VLAN 
> > from ~ 410 MByte/s to ~ 660 MByte/s, which is approx. the same speed as 
> > without VLAN.
> > 
> > The diff looks good to me, but I am not very familiar with the VLAN code. 
> > For IFCAP_VLAN_HWTAGGING, drivers wrap the if_capabilities bit with #if 
> > NVLAN > 0 . Do we want that for IFCAP_VLAN_HWOFFLOAD, too?
> 
> Its not that important here, but I put the ifdef around both VLAN
> macros for consistency.
> 
> ok?

tested and OK bluhm@

> Index: sbin/ifconfig/ifconfig.8
> ===================================================================
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.8,v
> diff -u -p -r1.399 ifconfig.8
> --- sbin/ifconfig/ifconfig.8	11 Jan 2024 17:22:04 -0000	1.399
> +++ sbin/ifconfig/ifconfig.8	7 Jun 2024 13:56:53 -0000
> @@ -294,6 +294,9 @@ tag.
>  On transmit, the device can add the
>  .Xr vlan 4
>  tag.
> +.It Sy VLAN_HWOFFLOAD
> +On transmit, the device can handle checksum or TSO offload without
> +.Sy VLAN_HWTAGGING .
>  .It Sy WOL
>  The device supports Wake on LAN (WoL).
>  .It Sy hardmtu
> Index: sbin/ifconfig/ifconfig.c
> ===================================================================
> RCS file: /cvs/src/sbin/ifconfig/ifconfig.c,v
> diff -u -p -r1.472 ifconfig.c
> --- sbin/ifconfig/ifconfig.c	18 May 2024 02:44:22 -0000	1.472
> +++ sbin/ifconfig/ifconfig.c	7 Jun 2024 13:56:53 -0000
> @@ -125,7 +125,7 @@
>  
>  #define HWFEATURESBITS							\
>  	"\024\1CSUM_IPv4\2CSUM_TCPv4\3CSUM_UDPv4"			\
> -	"\5VLAN_MTU\6VLAN_HWTAGGING\10CSUM_TCPv6"			\
> +	"\5VLAN_MTU\6VLAN_HWTAGGING\7VLAN_HWOFFLOAD\10CSUM_TCPv6"	\
>  	"\11CSUM_UDPv6\15TSOv4\16TSOv6\17LRO\20WOL"
>  
>  struct ifencap {
> Index: sys/dev/pv/if_vio.c
> ===================================================================
> RCS file: /cvs/src/sys/dev/pv/if_vio.c,v
> diff -u -p -r1.37 if_vio.c
> --- sys/dev/pv/if_vio.c	4 Jun 2024 09:51:52 -0000	1.37
> +++ sys/dev/pv/if_vio.c	7 Jun 2024 13:56:53 -0000
> @@ -604,7 +604,11 @@ vio_attach(struct device *parent, struct
>  	ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
>  	ifp->if_start = vio_start;
>  	ifp->if_ioctl = vio_ioctl;
> -	ifp->if_capabilities = IFCAP_VLAN_MTU;
> +	ifp->if_capabilities = 0;
> +#if NVLAN > 0
> +	ifp->if_capabilities |= IFCAP_VLAN_MTU;
> +	ifp->if_capabilities |= IFCAP_VLAN_HWOFFLOAD;
> +#endif
>  	if (virtio_has_feature(vsc, VIRTIO_NET_F_CSUM))
>  		ifp->if_capabilities |= IFCAP_CSUM_TCPv4|IFCAP_CSUM_UDPv4|
>  		    IFCAP_CSUM_TCPv6|IFCAP_CSUM_UDPv6;
> Index: sys/net/if.h
> ===================================================================
> RCS file: /cvs/src/sys/net/if.h,v
> diff -u -p -r1.216 if.h
> --- sys/net/if.h	11 Apr 2024 15:08:18 -0000	1.216
> +++ sys/net/if.h	7 Jun 2024 13:56:53 -0000
> @@ -249,6 +249,7 @@ struct if_status_description {
>  #define	IFCAP_CSUM_UDPv4	0x00000004	/* can do IPv4/UDP csum */
>  #define	IFCAP_VLAN_MTU		0x00000010	/* VLAN-compatible MTU */
>  #define	IFCAP_VLAN_HWTAGGING	0x00000020	/* hardware VLAN tag support */
> +#define	IFCAP_VLAN_HWOFFLOAD	0x00000040	/* hw offload w/ inline tag */
>  #define	IFCAP_CSUM_TCPv6	0x00000080	/* can do IPv6/TCP checksums */
>  #define	IFCAP_CSUM_UDPv6	0x00000100	/* can do IPv6/UDP checksums */
>  #define	IFCAP_TSOv4		0x00001000	/* IPv4/TCP segment offload */
> Index: sys/net/if_vlan.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_vlan.c,v
> diff -u -p -r1.218 if_vlan.c
> --- sys/net/if_vlan.c	23 Dec 2023 10:52:54 -0000	1.218
> +++ sys/net/if_vlan.c	7 Jun 2024 13:56:53 -0000
> @@ -523,7 +523,7 @@ vlan_up(struct vlan_softc *sc)
>  	/*
>  	 * Note: In cases like vio(4) and em(4) where the offsets of the
>  	 * csum can be freely defined, we could actually do csum offload
> -	 * for VLAN and QINQ packets.
> +	 * for QINQ packets.
>  	 */
>  	if (sc->sc_type != ETHERTYPE_VLAN) {
>  		/*
> @@ -531,10 +531,14 @@ vlan_up(struct vlan_softc *sc)
>  		 * ethernet type (0x8100).
>  		 */
>  		ifp->if_capabilities = 0;
> -	} else if (ISSET(ifp0->if_capabilities, IFCAP_VLAN_HWTAGGING)) {
> +	} else if (ISSET(ifp0->if_capabilities, IFCAP_VLAN_HWTAGGING) ||
> +	    ISSET(ifp0->if_capabilities, IFCAP_VLAN_HWOFFLOAD)) {
>  		/*
>  		 * Chips that can do hardware-assisted VLAN encapsulation, can
>  		 * calculate the correct checksum for VLAN tagged packets.
> +		 *
> +		 * Hardware which does checksum offloading, but not VLAN tag
> +		 * injection, have to set IFCAP_VLAN_HWOFFLOAD.
>  		 */
>  		ifp->if_capabilities = ifp0->if_capabilities &
>  		    (IFCAP_CSUM_MASK | IFCAP_TSOv4 | IFCAP_TSOv6);