Re: [PATCH v11 3/3] vhost: Add configuration controls for vhost worker's mode

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jun 09, 2025 at 03:33:09PM +0800, Cindy Lu wrote:
> This patch introduces functionality to control the vhost worker mode:
> 
> - Add two new IOCTLs:
>   * VHOST_SET_FORK_FROM_OWNER: Allows userspace to select between
>     task mode (fork_owner=1) and kthread mode (fork_owner=0)
>   * VHOST_GET_FORK_FROM_OWNER: Retrieves the current thread mode
>     setting
> 
> - Expose module parameter 'fork_from_owner_default' to allow system
>   administrators to configure the default mode for vhost workers
> 
> - Add KConfig option CONFIG_VHOST_ENABLE_FORK_OWNER_CONTROL to
>   control the availability of these IOCTLs and parameter, allowing
>   distributions to disable them if not needed
> 
> - The VHOST_NEW_WORKER functionality requires fork_owner to be set
>   to true, with validation added to ensure proper configuration
> 
> Signed-off-by: Cindy Lu <lulu@redhat.com>


getting there. yet something to improve.

> ---
>  drivers/vhost/Kconfig      | 17 +++++++++++++++
>  drivers/vhost/vhost.c      | 44 ++++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/vhost.h | 25 ++++++++++++++++++++++
>  3 files changed, 86 insertions(+)
> 
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> index 020d4fbb947c..49e1d9dc92b7 100644
> --- a/drivers/vhost/Kconfig
> +++ b/drivers/vhost/Kconfig
> @@ -96,3 +96,20 @@ config VHOST_CROSS_ENDIAN_LEGACY
>  	  If unsure, say "N".
>  
>  endif
> +
> +config CONFIG_VHOST_ENABLE_FORK_OWNER_CONTROL
> +	bool "Enable CONFIG_VHOST_ENABLE_FORK_OWNER_CONTROL"
> +	default n
> +	help
> +	  This option enables two IOCTLs: VHOST_SET_FORK_FROM_OWNER and
> +	  VHOST_GET_FORK_FROM_OWNER. These allow userspace applications
> +	  to modify the vhost worker mode for vhost devices.
> +
> +	  Also expose module parameter 'fork_from_owner_default' to allow users
> +	  to configure the default mode for vhost workers.
> +
> +	  By default, `CONFIG_VHOST_ENABLE_FORK_OWNER_CONTROL` is set to `n`,
> +	  which disables the IOCTLs and parameter.
> +	  When enabled (y), users can change the worker thread mode as needed.
> +
> +	  If unsure, say "N".
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 37d3ed8be822..903d9c3f6784 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -43,6 +43,11 @@ module_param(max_iotlb_entries, int, 0444);
>  MODULE_PARM_DESC(max_iotlb_entries,
>  	"Maximum number of iotlb entries. (default: 2048)");
>  static bool fork_from_owner_default = true;

Add empty lines around ifdef to make it clear what code do they
delimit.


> +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_CONTROL
> +module_param(fork_from_owner_default, bool, 0444);
> +MODULE_PARM_DESC(fork_from_owner_default,
> +		 "Set task mode as the default(default: Y)");
> +#endif
>  
>  enum {
>  	VHOST_MEMORY_F_LOG = 0x1,
> @@ -1019,6 +1024,13 @@ long vhost_worker_ioctl(struct vhost_dev *dev, unsigned int ioctl,
>  	switch (ioctl) {
>  	/* dev worker ioctls */
>  	case VHOST_NEW_WORKER:
> +		/*
> +		 * vhost_tasks will account for worker threads under the parent's
> +		 * NPROC value but kthreads do not. To avoid userspace overflowing
> +		 * the system with worker threads fork_owner must be true.
> +		 */
> +		if (!dev->fork_owner)
> +			return -EFAULT;

An empty line here would make the code clearer.

>  		ret = vhost_new_worker(dev, &state);
>  		if (!ret && copy_to_user(argp, &state, sizeof(state)))
>  			ret = -EFAULT;
> @@ -1136,6 +1148,7 @@ void vhost_dev_reset_owner(struct vhost_dev *dev, struct vhost_iotlb *umem)
>  
>  	vhost_dev_cleanup(dev);
>  
> +	dev->fork_owner = fork_from_owner_default;
>  	dev->umem = umem;
>  	/* We don't need VQ locks below since vhost_dev_cleanup makes sure
>  	 * VQs aren't running.
> @@ -2289,6 +2302,37 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
>  		goto done;
>  	}
>  
> +#ifdef CONFIG_VHOST_ENABLE_FORK_OWNER_CONTROL
> +	u8 fork_owner;

Do not declare variables in the middle of a scope please.
This one is not needed in this scope, so just move it down
to within if (yes you will repeat the declaration twice then).



> +
> +	if (ioctl == VHOST_SET_FORK_FROM_OWNER) {
> +		/*fork_owner can only be modified before owner is set*/

bad comment style.

> +		if (vhost_dev_has_owner(d)) {
> +			r = -EBUSY;
> +			goto done;
> +		}
> +		if (copy_from_user(&fork_owner, argp, sizeof(u8))) {

get_user is a better fit for this. In particular, typesafe.


> +			r = -EFAULT;
> +			goto done;
> +		}
> +		if (fork_owner > 1) {

so 0 and 1 are the only legal values?
maybe add an enum or defines in the header then.


> +			r = -EINVAL;
> +			goto done;
> +		}
> +		d->fork_owner = (bool)fork_owner;

		!!fork_owner is shorter and idiomatic.

> +		r = 0;
> +		goto done;
> +	}
> +	if (ioctl == VHOST_GET_FORK_FROM_OWNER) {
> +		fork_owner = d->fork_owner;
> +		if (copy_to_user(argp, &fork_owner, sizeof(u8))) {

put_user

> +			r = -EFAULT;
> +			goto done;
> +		}
> +		r = 0;
> +		goto done;
> +	}
> +#endif
>  	/* You must be the owner to do anything else */
>  	r = vhost_dev_check_owner(d);
>  	if (r)
> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> index d4b3e2ae1314..e51d6a347607 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -235,4 +235,29 @@
>   */
>  #define VHOST_VDPA_GET_VRING_SIZE	_IOWR(VHOST_VIRTIO, 0x82,	\
>  					      struct vhost_vring_state)
> +
> +/**
> + * VHOST_SET_FORK_FROM_OWNER - Set the fork_owner flag for the vhost device,
> + * This ioctl must called before VHOST_SET_OWNER.
> + * Only available when CONFIG_VHOST_ENABLE_FORK_OWNER_CONTROL=y
> + *
> + * @param fork_owner: An 8-bit value that determines the vhost thread mode
> + *
> + * When fork_owner is set to 1(default value):
> + *   - Vhost will create vhost worker as tasks forked from the owner,
> + *     inheriting all of the owner's attributes.
> + *
> + * When fork_owner is set to 0:
> + *   - Vhost will create vhost workers as kernel threads.
> + */
> +#define VHOST_SET_FORK_FROM_OWNER _IOW(VHOST_VIRTIO, 0x83, __u8)
> +
> +/**
> + * VHOST_GET_FORK_OWNER - Get the current fork_owner flag for the vhost device.
> + * Only available when CONFIG_VHOST_ENABLE_FORK_OWNER_CONTROL=y
> + *
> + * @return: An 8-bit value indicating the current thread mode.
> + */
> +#define VHOST_GET_FORK_FROM_OWNER _IOR(VHOST_VIRTIO, 0x84, __u8)
> +
>  #endif
> -- 
> 2.45.0