Re: [PATCH 2/7] conf: Add context fields, epoll types and --vhost-kernel flag to pasta

[Laurent Vivier <lvivier@redhat.com>]

On 9/4/26 23:28, aerosouund wrote:
> From: Ammar Yasser <aerosound161@gmail.com>
> 
> --vhost-kernel controls whether to use vhost-net acceleration for pasta.
> It takes one of three modes: "on" requires acceleration and fails if it
> isn't available, "off" never uses it, and "auto" uses it if it's
> available and falls back to plain tap operation otherwise. "auto" is the
> default, so an unmodified invocation picks up acceleration wherever the
> kernel offers it, and behaves as before where it doesn't.
> 
> EPOLL_TYPE_VHOST_CALL means the kernel wants to notify us about data it
> has written. EPOLL_TYPE_VHOST_ERROR means the kernel encountered an
> internal error on vhost and wants to tell us something went wrong.
> 
> Collect the vhost state in a single struct vhost_ctx with the following
> fields:
> 
>    - mode:     which of the three modes above was requested
>    - fd:       file descriptor for /dev/vhost-net, which doubles as our
>                indication that setup succeeded
>    - features: virtio feature bits negotiated with vhost-net
>    - vq[]:     kick, call and err eventfds for both queues
> 
> Modify tap_hdr_iov to create an iov with the size of a
> virtio_net_mrg_rxbuf if vhost initialization has been successful (we are
> in mode 'on' or 'auto' and we didn't need the tap fallback)
> 
> Move the L2_MAX_LEN_XX constants from tap.h to passt.h to avoid the
> circular dependency between both files. VHOST_DESC_BYTES is defined as
> the maximum l2 frame size + the size of the virtio net header, if the
> length constant remains in tap.h passt.h would need to include it, but
> tap.h needs to include passt.h to get the definition of `struct ctx`.
> Moving the length constants to passt.h makes it free of dependency on
> tap.h
> 
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> Signed-off-by: Ammar Yasser <aerosound161@gmail.com>
> ---
>   conf.c       | 19 +++++++++++++
>   epoll_type.h |  4 +++
>   passt.c      |  3 ++
>   passt.h      | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++--
>   tap.h        | 36 ++++++------------------
>   5 files changed, 112 insertions(+), 29 deletions(-)
> 
> diff --git a/conf.c b/conf.c
> index faf2681..a7b934a 100644
> --- a/conf.c
> +++ b/conf.c
> @@ -736,6 +736,9 @@ pasta_opts:
>   		"    default: auto\n"
>   		"  --host-lo-to-ns-lo	Translate host-loopback forwards to\n"
>   		"			namespace loopback\n"
> +		"  --vhost-kernel MODE	Use vhost-kernel acceleration\n"
> +		"    MODE is \"on\", \"off\" or \"auto\"\n"
> +		"    default: auto\n"
>   		"  --userns NSPATH 	Target user namespace to join\n"
>   		"  --netns PATH|NAME	Target network namespace to join\n"
>   		"  --netns-only		Don't join existing user namespace\n"
> @@ -766,6 +769,7 @@ enum passt_modes conf_mode(int argc, char *argv[])
>   	int vhost_user = 0;
>   	const struct option optvu[] = {
>   		{"vhost-user",	no_argument,		&vhost_user,	1 },
> +		{"vhost-kernel", required_argument,	NULL,		0 },

I don't think it's really needed.

>   		{ 0 },
>   	};
>   	char argv0[PATH_MAX], *basearg0;
> @@ -1348,6 +1352,7 @@ void conf(struct ctx *c, int argc, char **argv)
>   		{"stats", required_argument,		NULL,		31 },
>   		{"conf-path",	required_argument,	NULL,		'c' },
>   		{"chroot-fallback", no_argument,	NULL, 		32 },
> +		{"vhost-kernel", required_argument,	NULL,		33 },
>   		{ 0 },
>   	};
>   	const char *optstring = "+dqfel:hs:c:F:I:p:P:m:a:n:M:g:i:o:D:S:H:461t:u:T:U:";
> @@ -1588,6 +1593,20 @@ void conf(struct ctx *c, int argc, char **argv)
>   			break;
>   		case 32:
>   			c->chroot_fallback = true;
> +			break;
> +		case 33:
> +			if (c->mode != MODE_PASTA)
> +				die("--vhost-kernel is for pasta mode only");
> +
> +			if (!strcmp(optarg, "auto"))
> +				c->vhost.mode = VHOST_MODE_AUTO;
> +			else if (!strcmp(optarg, "on"))
> +				c->vhost.mode = VHOST_MODE_ON;
> +			else if (!strcmp(optarg, "off"))
> +				c->vhost.mode = VHOST_MODE_OFF;
> +			else
> +				die("Invalid vhost-kernel mode: %s", optarg);
> +
>   			break;
>   		case 'd':
>   			c->debug = 1;
> diff --git a/epoll_type.h b/epoll_type.h
> index 061325a..e3206d1 100644
> --- a/epoll_type.h
> +++ b/epoll_type.h
> @@ -50,6 +50,10 @@ enum epoll_type {
>   	EPOLL_TYPE_CONF_LISTEN,
>   	/* Configuration socket */
>   	EPOLL_TYPE_CONF,
> +	/* vhost-kernel call socket */
> +	EPOLL_TYPE_VHOST_CALL,
> +	/* vhost-kernel error socket */
> +	EPOLL_TYPE_VHOST_ERROR,
>   
>   	EPOLL_NUM_TYPES,
>   };
> diff --git a/passt.c b/passt.c
> index 5054551..6002231 100644
> --- a/passt.c
> +++ b/passt.c
> @@ -65,6 +65,7 @@ char pkt_buf[PKT_BUF_BYTES]	__attribute__ ((aligned(PAGE_SIZE)));
>   struct ctx passt_ctx = {
>   	.pidfile_fd		= -1,
>   	.fd_tap			= -1,
> +	.vhost			= { .fd = -1 },
>   	.fd_tap_listen		= -1,
>   	.fd_control_listen	= -1,
>   	.fd_repair_listen	= -1,
> @@ -92,6 +93,8 @@ char *epoll_type_str[] = {
>   	[EPOLL_TYPE_NL_NEIGH]		= "netlink neighbour notifier socket",
>   	[EPOLL_TYPE_CONF_LISTEN]	= "configuration listening socket",
>   	[EPOLL_TYPE_CONF]		= "configuration socket",
> +	[EPOLL_TYPE_VHOST_CALL]		= "vhost-kernel call socket",
> +	[EPOLL_TYPE_VHOST_ERROR]	= "vhost-kernel error socket",
>   };
>   static_assert(ARRAY_SIZE(epoll_type_str) == EPOLL_NUM_TYPES,
>   	      "epoll_type_str[] doesn't match enum epoll_type");
> diff --git a/passt.h b/passt.h
> index 51ccd4f..5e0de3e 100644
> --- a/passt.h
> +++ b/passt.h
> @@ -13,8 +13,11 @@ union epoll_ref;
>   
>   #include <stdbool.h>
>   #include <assert.h>
> +#include <limits.h>
>   #include <sys/epoll.h>
>   
> +#include <linux/virtio_net.h>
> +
>   #include "pif.h"
>   #include "packet.h"
>   #include "siphash.h"
> @@ -35,8 +38,43 @@ union epoll_ref;
>   #define MAC_OUR_LAA	\
>   	((uint8_t [ETH_ALEN]){0x9a, 0x55, 0x9a, 0x55, 0x9a, 0x55})
>   
> -/* Large enough for ~128 maximum size frames */
> -#define PKT_BUF_BYTES		(8UL << 20)
> +
> +/** L2_MAX_LEN_PASTA - Maximum frame length for pasta mode (with L2 header)
> + *
> + * The kernel tuntap device imposes a maximum frame size of 65535 including
> + * 'hard_header_len' (14 bytes for L2 Ethernet in the case of "tap" mode).
> + */
> +#define L2_MAX_LEN_PASTA	USHRT_MAX
> +
> +/** L2_MAX_LEN_PASST - Maximum frame length for passt mode (with L2 header)
> + *
> + * The only structural limit the QEMU socket protocol imposes on frames is
> + * (2^32-1) bytes, but that would be ludicrously long in practice.  For now,
> + * limit it somewhat arbitrarily to 65535 bytes.  FIXME: Work out an appropriate
> + * limit with more precision.
> + */
> +#define L2_MAX_LEN_PASST	USHRT_MAX
> +
> +/** L2_MAX_LEN_VU - Maximum frame length for vhost-user mode (with L2 header)
> + *
> + * vhost-user allows multiple buffers per frame, each of which can be quite
> + * large, so the inherent frame size limit is rather large.  Much larger than is
> + * actually useful for IP.  For now limit arbitrarily to 65535 bytes. FIXME:
> + * Work out an appropriate limit with more precision.
> + */
> +#define L2_MAX_LEN_VU		USHRT_MAX
> +
> +/* Number of descriptors in each vhost-net virtqueue */
> +#define VHOST_NDESCS		128
> +
> +/* Bytes of pkt_buf backing one from-guest descriptor: a maximum size frame
> + * plus the virtio-net header the kernel writes in front of it
> + */
> +#define VHOST_DESC_BYTES	(L2_MAX_LEN_PASTA +			\
> +				 sizeof(struct virtio_net_hdr_mrg_rxbuf))
> +
> +/* One maximum size frame per vhost-net descriptor */bbb
> +#define PKT_BUF_BYTES		(VHOST_NDESCS * VHOST_DESC_BYTES)

128 * (65535 + 12) = 8390016

but we have in passt.c

madvise(pkt_buf, sizeof(pkt_buf), MADV_HUGEPAGE);

originaly we had:

2dbed699e78e ("passt: Align pkt_buf to PAGE_SIZE (start and size), try to fit in huge pages")

And your change is reverting the logic of:

b6945e055376 ("Simplify sizing of pkt_buf")

>   
>   extern char pkt_buf		[PKT_BUF_BYTES];
>   
> @@ -156,6 +194,40 @@ struct ip6_ctx {
>   
>   #include <netinet/if_ether.h>
>   
> +/**
> + * enum vhost_mode - Whether to use vhost-kernel acceleration
> + * @VHOST_MODE_AUTO:	Use it if it's available, fall back to plain tap if not
> + * @VHOST_MODE_ON:	Require it, fail if it's not available
> + * @VHOST_MODE_OFF:	Never use it
> + */
> +enum vhost_mode {
> +	VHOST_MODE_AUTO = 0,
> +	VHOST_MODE_ON,
> +	VHOST_MODE_OFF,
> +};
> +
> +/**
> + * struct vhost_ctx - Execution context for vhost-kernel acceleration
> + * @mode:	Whether to use acceleration at all, see enum vhost_mode
> + * @fd:		File descriptor for /dev/vhost-net, -1 if not set up
> + * @features:	virtio feature bits negotiated with vhost-net
> + * @vq:		Per-virtqueue eventfds ([0] is from-guest, [1] is to-guest)
> + * @vq.kick_fd:	Written by us, to tell the kernel we queued something
> + * @vq.call_fd:	Written by the kernel, to tell us it queued something
> + * @vq.err_fd:	Written by the kernel on an internal vhost error
> + */
> +struct vhost_ctx {
> +	enum vhost_mode mode;
> +	int fd;
> +	uint64_t features;
> +
> +	struct {
> +		int kick_fd;
> +		int call_fd;
> +		int err_fd;
> +	} vq[2];
> +};
> +
>   /**
>    * struct ctx - Execution context
>    * @mode:		Operation mode, qemu/UNIX domain socket or namespace/tap
> @@ -185,6 +257,7 @@ struct ip6_ctx {
>    * @our_tap_mac:	Pasta/passt's MAC on the tap link
>    * @guest_mac:		MAC address of guest or namespace, seen or configured
>    * @hash_secret:	128-bit secret for siphash functions
> + * @vhost:		vhost-kernel acceleration context, pasta mode only
>    * @ifi4:		Template interface for IPv4, -1: none, 0: IPv4 disabled
>    * @ip4:		IPv4 configuration
>    * @dns_search:		DNS search list
> @@ -264,6 +337,8 @@ struct ctx {
>   
>   	uint64_t hash_secret[2];
>   
> +	struct vhost_ctx vhost;
> +
>   	int ifi4;
>   	struct ip4_ctx ip4;
>   
> diff --git a/tap.h b/tap.h
> index 1625975..ce1efa8 100644
> --- a/tap.h
> +++ b/tap.h
> @@ -12,31 +12,6 @@
>   #include "passt.h"
>   #include "tap_hdr.h"
>   
> -/** L2_MAX_LEN_PASTA - Maximum frame length for pasta mode (with L2 header)
> - *
> - * The kernel tuntap device imposes a maximum frame size of 65535 including
> - * 'hard_header_len' (14 bytes for L2 Ethernet in the case of "tap" mode).
> - */
> -#define L2_MAX_LEN_PASTA	USHRT_MAX
> -
> -/** L2_MAX_LEN_PASST - Maximum frame length for passt mode (with L2 header)
> - *
> - * The only structural limit the QEMU socket protocol imposes on frames is
> - * (2^32-1) bytes, but that would be ludicrously long in practice.  For now,
> - * limit it somewhat arbitrarily to 65535 bytes.  FIXME: Work out an appropriate
> - * limit with more precision.
> - */
> -#define L2_MAX_LEN_PASST	USHRT_MAX
> -
> -/** L2_MAX_LEN_VU - Maximum frame length for vhost-user mode (with L2 header)
> - *
> - * vhost-user allows multiple buffers per frame, each of which can be quite
> - * large, so the inherent frame size limit is rather large.  Much larger than is
> - * actually useful for IP.  For now limit arbitrarily to 65535 bytes. FIXME:
> - * Work out an appropriate limit with more precision.
> - */
> -#define L2_MAX_LEN_VU		USHRT_MAX
> -
>   struct udphdr;
>   
>   /**
> @@ -50,10 +25,17 @@ struct udphdr;
>   static inline struct iovec tap_hdr_iov(const struct ctx *c,
>   				       struct tap_hdr *thdr)
>   {
> -	return (struct iovec){
> +	struct iovec ret = {
>   		.iov_base = thdr,
> -		.iov_len = c->mode == MODE_PASST ? sizeof(*thdr) : 0,
>   	};
> +
> +	if ((c->vhost.fd != -1)) {

double parentheses.

> +		ret.iov_len = sizeof(struct virtio_net_hdr_mrg_rxbuf);
> +	} else {
> +		ret.iov_len = c->mode == MODE_PASST ? sizeof(thdr->vnet_len) : 0;
> +	};

no need for the semicolon.

This change should be in PATCH 1

> +
> +	return ret;
>   }
>   
>   /**