[RESEND PATCH v1 0/7]vhost: Add support of kthread API

[RESEND PATCH v1 0/7]vhost: Add support of kthread API

[Cindy Lu]

In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
vhost removed the support for the kthread API. However, there are
still situations where there is a request to use kthread.
In this PATCH, the support of kthread is added back. Additionally,
a module_param is added to enforce which mode we are using, and
a new UAPI is introduced to allow the userspace app to set the
mode they want to use.

Tested the user application with QEMU.

Cindy Lu (7):
  vhost: Add a new module_param for enable kthread
  vhost: Add kthread support in function vhost_worker_queue()
  vhost: Add kthread support in function vhost_workers_free()
  vhost: Add the vhost_worker to support kthread
  vhost: Add the cgroup related function
  vhost: Add kthread support in function vhost_worker_create
  vhost: Add new UAPI to support change to task mode

 drivers/vhost/vhost.c      | 246 +++++++++++++++++++++++++++++++++++--
 drivers/vhost/vhost.h      |   1 +
 include/uapi/linux/vhost.h |   2 +
 3 files changed, 240 insertions(+), 9 deletions(-)

-- 
2.45.0

[RESEND PATCH v1 1/7] vhost: Add a new module_param for enable kthread

[Cindy Lu]

Add a new module parameter to enable kthread while loading modules.
This parameter will identify if the vhost will use kthread or a task.
The default value will be true.

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vhost/vhost.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 9ac25d08f473..be43181af659 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -41,6 +41,9 @@ static int max_iotlb_entries = 2048;
 module_param(max_iotlb_entries, int, 0444);
 MODULE_PARM_DESC(max_iotlb_entries,
 	"Maximum number of iotlb entries. (default: 2048)");
+bool enforce_kthread = true;
+module_param(enforce_kthread, bool, 0444);
+MODULE_PARM_DESC(enforce_kthread, "enable vhost to use kthread (default: Y)");
 
 enum {
 	VHOST_MEMORY_F_LOG = 0x1,
-- 
2.45.0

Re: [RESEND PATCH v1 1/7] vhost: Add a new module_param for enable kthread

[kernel test robot]

Hi Cindy,

kernel test robot noticed the following build warnings:

[auto build test WARNING on mst-vhost/linux-next]
[also build test WARNING on linus/master v6.11-rc7 next-20240909]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Cindy-Lu/vhost-Add-kthread-support-in-function-vhost_worker_queue/20240909-110046
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git linux-next
patch link:    https://lore.kernel.org/r/20240909020138.1245873-2-lulu%40redhat.com
patch subject: [RESEND PATCH v1 1/7] vhost: Add a new module_param for enable kthread
config: x86_64-randconfig-123-20240910 (https://download.01.org/0day-ci/archive/20240910/202409101256.eWKWH1mw-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240910/202409101256.eWKWH1mw-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409101256.eWKWH1mw-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/vhost/vhost.c:44:6: sparse: sparse: symbol 'enforce_kthread' was not declared. Should it be static?
   drivers/vhost/vhost.c:1898:54: sparse: sparse: self-comparison always evaluates to false
   drivers/vhost/vhost.c:1899:54: sparse: sparse: self-comparison always evaluates to false
   drivers/vhost/vhost.c:1900:55: sparse: sparse: self-comparison always evaluates to false
   drivers/vhost/vhost.c:2155:46: sparse: sparse: self-comparison always evaluates to false
   drivers/vhost/vhost.c:2235:48: sparse: sparse: self-comparison always evaluates to false

vim +/enforce_kthread +44 drivers/vhost/vhost.c

    35	
    36	static ushort max_mem_regions = 64;
    37	module_param(max_mem_regions, ushort, 0444);
    38	MODULE_PARM_DESC(max_mem_regions,
    39		"Maximum number of memory regions in memory map. (default: 64)");
    40	static int max_iotlb_entries = 2048;
    41	module_param(max_iotlb_entries, int, 0444);
    42	MODULE_PARM_DESC(max_iotlb_entries,
    43		"Maximum number of iotlb entries. (default: 2048)");
  > 44	bool enforce_kthread = true;
    45	module_param(enforce_kthread, bool, 0444);
    46	MODULE_PARM_DESC(enforce_kthread, "enable vhost to use kthread (default: Y)");
    47	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[RESEND PATCH v1 2/7] vhost: Add kthread support in function vhost_worker_queue()

[Cindy Lu]

Added back the previously removed function vhost_worker_queue() and
renamed it to vhost_worker_queue_khtread(). The new vhost_worker_queue()
will select the different mode based on the value of the parameter.

The old function vhost_work_queue()  was change to support task in
commit 6e890c5d5021 ('vhost: use vhost_tasks for worker threads')
changed in
commit f9010dbdc ('fork, vhost: Use CLONE_THREAD to fix freezer/ps regression')
and also was change the name of function to vhost_worker_queue() in
commit 0921dddcb5 (vhost: take worker or vq instead of dev for queueing)

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vhost/vhost.c | 30 ++++++++++++++++++++++++++++--
 drivers/vhost/vhost.h |  1 +
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index be43181af659..ab4608fcf86b 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -236,8 +236,8 @@ void vhost_poll_stop(struct vhost_poll *poll)
 }
 EXPORT_SYMBOL_GPL(vhost_poll_stop);
 
-static void vhost_worker_queue(struct vhost_worker *worker,
-			       struct vhost_work *work)
+static void vhost_worker_queue_task(struct vhost_worker *worker,
+				    struct vhost_work *work)
 {
 	if (!test_and_set_bit(VHOST_WORK_QUEUED, &work->flags)) {
 		/* We can only add the work to the list after we're
@@ -249,6 +249,32 @@ static void vhost_worker_queue(struct vhost_worker *worker,
 	}
 }
 
+static void vhost_work_queue_kthread(struct vhost_worker *worker,
+				     struct vhost_work *work)
+{
+	if (!worker)
+		return;
+
+	if (!test_and_set_bit(VHOST_WORK_QUEUED, &work->flags)) {
+		/* We can only add the work to the list after we're
+		 * sure it was not in the list.
+		 * test_and_set_bit() implies a memory barrier.
+		 */
+		llist_add(&work->node, &worker->work_list);
+
+		wake_up_process(worker->task);
+	}
+}
+
+static void vhost_worker_queue(struct vhost_worker *worker,
+			       struct vhost_work *work)
+{
+	if (enforce_kthread) {
+		vhost_work_queue_kthread(worker, work);
+	} else {
+		vhost_worker_queue_task(worker, work);
+	}
+}
 bool vhost_vq_work_queue(struct vhost_virtqueue *vq, struct vhost_work *work)
 {
 	struct vhost_worker *worker;
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index bb75a292d50c..c7f126fd09e8 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -27,6 +27,7 @@ struct vhost_work {
 };
 
 struct vhost_worker {
+	struct task_struct	*task;
 	struct vhost_task	*vtsk;
 	struct vhost_dev	*dev;
 	/* Used to serialize device wide flushing with worker swapping. */
-- 
2.45.0

[RESEND PATCH v1 3/7] vhost: Add kthread support in function vhost_workers_free()

[Cindy Lu]

Added back the previously removed function vhost_workers_free() and
renamed it to vhost_workers_free_khtread(). The new vhost_workers_free()
will select the different mode based on the value of the parameter.

The old function vhost_workers_free was change to support task in
commit 6e890c5d5021 ('vhost: use vhost_tasks for worker threads')
also changed in
commit a284f09effe ('vhost: Fix crash during early vhost_transport_send_pkt calls')
change to xarray in
commit 1cdaafa1b8b ('vhost: replace single worker pointer with xarray')

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vhost/vhost.c | 52 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 5 deletions(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index ab4608fcf86b..da4482efd2cb 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -647,8 +647,21 @@ static void vhost_detach_mm(struct vhost_dev *dev)
 	dev->mm = NULL;
 }
 
-static void vhost_worker_destroy(struct vhost_dev *dev,
-				 struct vhost_worker *worker)
+static void vhost_worker_destroy_kthread(struct vhost_dev *dev,
+					 struct vhost_worker *worker)
+{
+	if (!worker)
+		return;
+
+	WARN_ON(!llist_empty(&worker->work_list));
+
+	xa_erase(&dev->worker_xa, worker->id);
+	kthread_stop(worker->task);
+	kfree(worker);
+}
+
+static void vhost_worker_destroy_task(struct vhost_dev *dev,
+				      struct vhost_worker *worker)
 {
 	if (!worker)
 		return;
@@ -659,7 +672,7 @@ static void vhost_worker_destroy(struct vhost_dev *dev,
 	kfree(worker);
 }
 
-static void vhost_workers_free(struct vhost_dev *dev)
+static void vhost_workers_free_task(struct vhost_dev *dev)
 {
 	struct vhost_worker *worker;
 	unsigned long i;
@@ -674,10 +687,36 @@ static void vhost_workers_free(struct vhost_dev *dev)
 	 * created but couldn't clean up (it forgot or crashed).
 	 */
 	xa_for_each(&dev->worker_xa, i, worker)
-		vhost_worker_destroy(dev, worker);
+		vhost_worker_destroy_task(dev, worker);
 	xa_destroy(&dev->worker_xa);
 }
 
+static void vhost_workers_free_kthread(struct vhost_dev *dev)
+{
+	struct vhost_worker *worker;
+	unsigned long i;
+
+	if (!dev->use_worker)
+		return;
+
+	for (i = 0; i < dev->nvqs; i++)
+		rcu_assign_pointer(dev->vqs[i]->worker, NULL);
+	/*
+	 * Free the default worker we created and cleanup workers userspace
+	 * created but couldn't clean up (it forgot or crashed).
+	 */
+	xa_for_each(&dev->worker_xa, i, worker)
+		vhost_worker_destroy_kthread(dev, worker);
+	xa_destroy(&dev->worker_xa);
+}
+
+static void vhost_workers_free(struct vhost_dev *dev)
+{
+	if (enforce_kthread)
+		vhost_workers_free_kthread(dev);
+	else
+		vhost_workers_free_task(dev);
+}
 static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
 {
 	struct vhost_worker *worker;
@@ -845,7 +884,10 @@ static int vhost_free_worker(struct vhost_dev *dev,
 	__vhost_worker_flush(worker);
 	mutex_unlock(&worker->mutex);
 
-	vhost_worker_destroy(dev, worker);
+	if (enforce_kthread)
+		vhost_worker_destroy_kthread(dev, worker);
+	else
+		vhost_worker_destroy_task(dev, worker);
 	return 0;
 }
 
-- 
2.45.0

[RESEND PATCH v1 4/7] vhost: Add the vhost_worker to support kthread

[Cindy Lu]

Add back the previously removed vhost_worker function to support the kthread
and rename it vhost_run_work_kthread_list.

The old function vhost_worker was change to support task in
commit 6e890c5d5021 ('vhost: use vhost_tasks for worker threads')
change to xarray in
commit 1cdaafa1b8b ('vhost: replace single worker pointer with xarray')

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vhost/vhost.c | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index da4482efd2cb..0bc4f75ae1b9 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -417,6 +417,44 @@ static void vhost_vq_reset(struct vhost_dev *dev,
 	__vhost_vq_meta_reset(vq);
 }
 
+static int vhost_run_work_kthread_list(void *data)
+{
+	struct vhost_worker *worker = data;
+	struct vhost_work *work, *work_next;
+	struct vhost_dev *dev = worker->dev;
+	struct llist_node *node;
+
+	kthread_use_mm(dev->mm);
+
+	for (;;) {
+		/* mb paired w/ kthread_stop */
+		set_current_state(TASK_INTERRUPTIBLE);
+
+		if (kthread_should_stop()) {
+			__set_current_state(TASK_RUNNING);
+			break;
+		}
+		node = llist_del_all(&worker->work_list);
+		if (!node)
+			schedule();
+
+		node = llist_reverse_order(node);
+		/* make sure flag is seen after deletion */
+		smp_wmb();
+		llist_for_each_entry_safe(work, work_next, node, node) {
+			clear_bit(VHOST_WORK_QUEUED, &work->flags);
+			__set_current_state(TASK_RUNNING);
+			kcov_remote_start_common(worker->kcov_handle);
+			work->fn(work);
+			kcov_remote_stop();
+			cond_resched();
+		}
+	}
+	kthread_unuse_mm(dev->mm);
+
+	return 0;
+}
+
 static bool vhost_run_work_list(void *data)
 {
 	struct vhost_worker *worker = data;
-- 
2.45.0

[RESEND PATCH v1 5/7] vhost: Add the cgroup related function

[Cindy Lu]

Add back the previously removed cgroup function to support the kthread
The biggest change for this part is in vhost_attach_cgroups() and
vhost_worker_cgroups_kthread(). This is because of the change in
struct dev->worker_xa.

The old function was remove in
commit 6e890c5d5021 ('vhost: use vhost_tasks for worker threads')

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vhost/vhost.c | 52 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 0bc4f75ae1b9..46b15e9c0693 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -22,6 +22,7 @@
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
 #include <linux/kthread.h>
+#include <linux/cgroup.h>
 #include <linux/module.h>
 #include <linux/sort.h>
 #include <linux/sched/mm.h>
@@ -648,6 +649,57 @@ long vhost_dev_check_owner(struct vhost_dev *dev)
 }
 EXPORT_SYMBOL_GPL(vhost_dev_check_owner);
 
+struct vhost_attach_cgroups_struct {
+	struct vhost_work work;
+	struct task_struct *owner;
+	int ret;
+};
+
+static void vhost_attach_cgroups_work(struct vhost_work *work)
+{
+	struct vhost_attach_cgroups_struct *s;
+
+	s = container_of(work, struct vhost_attach_cgroups_struct, work);
+	s->ret = cgroup_attach_task_all(s->owner, current);
+}
+
+static int vhost_worker_cgroups_kthread(struct vhost_worker *worker)
+{
+	struct vhost_flush_struct flush;
+	struct vhost_attach_cgroups_struct attach;
+
+	attach.owner = current;
+
+	vhost_work_init(&attach.work, vhost_attach_cgroups_work);
+	vhost_worker_queue(worker, &attach.work);
+
+	init_completion(&flush.wait_event);
+	vhost_work_init(&flush.work, vhost_flush_work);
+	vhost_worker_queue(worker, &flush.work);
+	wait_for_completion(&flush.wait_event);
+
+	return attach.ret;
+}
+
+static int vhost_attach_cgroups(struct vhost_dev *dev)
+{
+	struct vhost_worker *worker;
+	unsigned long i;
+	int ret;
+
+	/*
+	 * Free the default worker we created and cleanup workers userspace
+	 * created but couldn't clean up (it forgot or crashed).
+	 */
+
+	xa_for_each(&dev->worker_xa, i, worker) {
+		ret = vhost_worker_cgroups_kthread(worker);
+		if (ret)
+			return ret;
+	}
+	return ret;
+}
+
 /* Caller should have device mutex */
 bool vhost_dev_has_owner(struct vhost_dev *dev)
 {
-- 
2.45.0

[RESEND PATCH v1 6/7] vhost: Add kthread support in function vhost_worker_create

[Cindy Lu]

Split the function vhost_worker_create to support both task and kthread
       
Added back the previous old function vhost_worker_create and rename it to
vhost_worker_create_khtread to support the khtread.

The new vhost_worker_create will be selected which to use based on the
value of the parameter.

the old function vhost_worker_create was change to support task in
commit 6e890c5d5021 ('vhost: use vhost_tasks for worker threads')
also changed in
commit 1cdaafa1b8b ('vhost: replace single worker pointer with xarray')
commit c011bb669dd ('vhost: dynamically allocate vhost_worker')

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vhost/vhost.c | 55 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 46b15e9c0693..838bce29a98b 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -807,7 +807,8 @@ static void vhost_workers_free(struct vhost_dev *dev)
 	else
 		vhost_workers_free_task(dev);
 }
-static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
+
+static struct vhost_worker *vhost_worker_create_task(struct vhost_dev *dev)
 {
 	struct vhost_worker *worker;
 	struct vhost_task *vtsk;
@@ -848,6 +849,50 @@ static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
 	return NULL;
 }
 
+static struct vhost_worker *vhost_worker_create_kthread(struct vhost_dev *dev)
+{
+	struct vhost_worker *worker;
+	struct task_struct *task;
+	int ret;
+	u32 id;
+
+	worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
+	if (!worker)
+		return NULL;
+
+	worker->dev = dev;
+	worker->kcov_handle = kcov_common_handle();
+
+	mutex_init(&worker->mutex);
+	init_llist_head(&worker->work_list);
+
+	task = kthread_create(vhost_run_work_kthread_list, worker, "vhost-%d",
+			      current->pid);
+	if (IS_ERR(task)) {
+		ret = PTR_ERR(task);
+		goto free_worker;
+	}
+
+	worker->task = task;
+	wake_up_process(task); /* avoid contributing to loadavg */
+	ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
+	if (ret < 0)
+		goto stop_worker;
+	worker->id = id;
+
+	ret = vhost_attach_cgroups(dev);
+	if (ret)
+		goto stop_worker;
+
+	return worker;
+
+stop_worker:
+	kthread_stop(worker->task);
+free_worker:
+	kfree(worker);
+	return NULL;
+}
+
 /* Caller must have device mutex */
 static void __vhost_vq_attach_worker(struct vhost_virtqueue *vq,
 				     struct vhost_worker *worker)
@@ -936,6 +981,14 @@ static int vhost_vq_attach_worker(struct vhost_virtqueue *vq,
 	return 0;
 }
 
+static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
+{
+	if (enforce_kthread)
+		return vhost_worker_create_kthread(dev);
+	else
+		return vhost_worker_create_task(dev);
+}
+
 /* Caller must have device mutex */
 static int vhost_new_worker(struct vhost_dev *dev,
 			    struct vhost_worker_state *info)
-- 
2.45.0

[RESEND PATCH v1 7/7] vhost: Add new UAPI to support change to task mode

[Cindy Lu]

Add a new UAPI to support setting the vhost device to
use task mode. The user space application needs to use
VHOST_SET_ENFORCE_TASK to set the mode. This setting must
be set before VHOST_SET_OWNER is set.

Signed-off-by: Cindy Lu <lulu@redhat.com>
---
 drivers/vhost/vhost.c      | 16 +++++++++++++++-
 include/uapi/linux/vhost.h |  2 ++
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 838bce29a98b..8fd19a1489e0 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -2340,14 +2340,28 @@ long vhost_dev_ioctl(struct vhost_dev *d, unsigned int ioctl, void __user *argp)
 {
 	struct eventfd_ctx *ctx;
 	u64 p;
-	long r;
+	long r = 0;
 	int i, fd;
+	bool enforce_task;
 
 	/* If you are not the owner, you can become one */
 	if (ioctl == VHOST_SET_OWNER) {
 		r = vhost_dev_set_owner(d);
 		goto done;
 	}
+	if (ioctl == VHOST_SET_ENFORCE_TASK) {
+		/* Is there an owner already? */
+		if (vhost_dev_has_owner(d)) {
+			r = -EBUSY;
+			goto done;
+		}
+		if (copy_from_user(&enforce_task, argp, sizeof(enforce_task))) {
+			r = -EFAULT;
+			goto done;
+		}
+		enforce_kthread = !enforce_task;
+		goto done;
+	}
 
 	/* You must be the owner to do anything else */
 	r = vhost_dev_check_owner(d);
diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
index b95dd84eef2d..9853d62d2d34 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -235,4 +235,6 @@
  */
 #define VHOST_VDPA_GET_VRING_SIZE	_IOWR(VHOST_VIRTIO, 0x82,	\
 					      struct vhost_vring_state)
+
+#define VHOST_SET_ENFORCE_TASK _IOW(VHOST_VIRTIO, 0x83, bool)
 #endif
-- 
2.45.0

Re: [RESEND PATCH v1 0/7]vhost: Add support of kthread API

[Michael S. Tsirkin]

On Mon, Sep 09, 2024 at 10:00:38AM +0800, Cindy Lu wrote:
> In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
> vhost removed the support for the kthread API. However, there are
> still situations where there is a request to use kthread.
> In this PATCH, the support of kthread is added back. Additionally,
> a module_param is added to enforce which mode we are using, and
> a new UAPI is introduced to allow the userspace app to set the
> mode they want to use.
> 
> Tested the user application with QEMU.

Cindy, the APIs do not make sense, security does not make sense,
and you are not describing the issue and the fix.


The name should reflect what this does from userspace POV, not from
kernel API POV.  kthread and task mode is not something that any users
have any business even to consider.


To help you out, some ideas:

I think the issue is something like "vhost is now a child of the
owner thread. While this makes sense from containerization
POV, some old userspace is confused, as previously vhost not
and so was allowed to steal cpu resources from outside the container."

Now, what can be done? Given we already released a secure kernel,
I am not inclined to revert it back to be insecure by default.
But I'm fine with a modparam to allow userspace to get insecure
behaviour.


Now, a modparam is annoying in that it affects all userspace,
and people might be running a mix of old and new userspace.
So if we do that, we also want a flag that will get
safe behaviour even if unsafe one is allowed.


Is this clear enough, or do I need to elaborate more?

Thanks!

> Cindy Lu (7):
>   vhost: Add a new module_param for enable kthread
>   vhost: Add kthread support in function vhost_worker_queue()
>   vhost: Add kthread support in function vhost_workers_free()
>   vhost: Add the vhost_worker to support kthread
>   vhost: Add the cgroup related function
>   vhost: Add kthread support in function vhost_worker_create
>   vhost: Add new UAPI to support change to task mode
> 
>  drivers/vhost/vhost.c      | 246 +++++++++++++++++++++++++++++++++++--
>  drivers/vhost/vhost.h      |   1 +
>  include/uapi/linux/vhost.h |   2 +
>  3 files changed, 240 insertions(+), 9 deletions(-)
> 
> -- 
> 2.45.0

Re: [RESEND PATCH v1 0/7]vhost: Add support of kthread API

[Jason Wang]

On Tue, Sep 10, 2024 at 3:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Mon, Sep 09, 2024 at 10:00:38AM +0800, Cindy Lu wrote:
> > In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
> > vhost removed the support for the kthread API. However, there are
> > still situations where there is a request to use kthread.
> > In this PATCH, the support of kthread is added back. Additionally,
> > a module_param is added to enforce which mode we are using, and
> > a new UAPI is introduced to allow the userspace app to set the
> > mode they want to use.
> >
> > Tested the user application with QEMU.
>
> Cindy, the APIs do not make sense, security does not make sense,
> and you are not describing the issue and the fix.
>
>
> The name should reflect what this does from userspace POV, not from
> kernel API POV.  kthread and task mode is not something that any users
> have any business even to consider.
>
>
> To help you out, some ideas:
>
> I think the issue is something like "vhost is now a child of the
> owner thread. While this makes sense from containerization
> POV, some old userspace is confused, as previously vhost not
> and so was allowed to steal cpu resources from outside the container."
>
> Now, what can be done? Given we already released a secure kernel,
> I am not inclined to revert it back to be insecure by default.

It depends on how we define "secure". There's plenty of users of
kthread and if I was not wrong, mike may still need to fix some bugs.


> But I'm fine with a modparam to allow userspace to get insecure
> behaviour.
>
>
> Now, a modparam is annoying in that it affects all userspace,
> and people might be running a mix of old and new userspace.
> So if we do that, we also want a flag that will get
> safe behaviour even if unsafe one is allowed.

I am not sure this can help. My understanding is that the flag is
sufficient. Otherwise the layered product needs to know if there's old
user space or new which seems to be a challenge.

Thanks

>
>
> Is this clear enough, or do I need to elaborate more?
>
> Thanks!
>
> > Cindy Lu (7):
> >   vhost: Add a new module_param for enable kthread
> >   vhost: Add kthread support in function vhost_worker_queue()
> >   vhost: Add kthread support in function vhost_workers_free()
> >   vhost: Add the vhost_worker to support kthread
> >   vhost: Add the cgroup related function
> >   vhost: Add kthread support in function vhost_worker_create
> >   vhost: Add new UAPI to support change to task mode
> >
> >  drivers/vhost/vhost.c      | 246 +++++++++++++++++++++++++++++++++++--
> >  drivers/vhost/vhost.h      |   1 +
> >  include/uapi/linux/vhost.h |   2 +
> >  3 files changed, 240 insertions(+), 9 deletions(-)
> >
> > --
> > 2.45.0
>

Re: [RESEND PATCH v1 0/7]vhost: Add support of kthread API

[Michael S. Tsirkin]

On Tue, Sep 10, 2024 at 04:37:52PM +0800, Jason Wang wrote:
> On Tue, Sep 10, 2024 at 3:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Mon, Sep 09, 2024 at 10:00:38AM +0800, Cindy Lu wrote:
> > > In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
> > > vhost removed the support for the kthread API. However, there are
> > > still situations where there is a request to use kthread.
> > > In this PATCH, the support of kthread is added back. Additionally,
> > > a module_param is added to enforce which mode we are using, and
> > > a new UAPI is introduced to allow the userspace app to set the
> > > mode they want to use.
> > >
> > > Tested the user application with QEMU.
> >
> > Cindy, the APIs do not make sense, security does not make sense,
> > and you are not describing the issue and the fix.
> >
> >
> > The name should reflect what this does from userspace POV, not from
> > kernel API POV.  kthread and task mode is not something that any users
> > have any business even to consider.
> >
> >
> > To help you out, some ideas:
> >
> > I think the issue is something like "vhost is now a child of the
> > owner thread. While this makes sense from containerization
> > POV, some old userspace is confused, as previously vhost not
> > and so was allowed to steal cpu resources from outside the container."
> >
> > Now, what can be done? Given we already released a secure kernel,
> > I am not inclined to revert it back to be insecure by default.
> 
> It depends on how we define "secure". There's plenty of users of
> kthread and if I was not wrong, mike may still need to fix some bugs.
> 

which bugs?

> > But I'm fine with a modparam to allow userspace to get insecure
> > behaviour.
> >
> >
> > Now, a modparam is annoying in that it affects all userspace,
> > and people might be running a mix of old and new userspace.
> > So if we do that, we also want a flag that will get
> > safe behaviour even if unsafe one is allowed.
> 
> I am not sure this can help. My understanding is that the flag is
> sufficient. Otherwise the layered product needs to know if there's old
> user space or new which seems to be a challenge.
> 
> Thanks

this will be up to userspace to resolve.


> >
> >
> > Is this clear enough, or do I need to elaborate more?
> >
> > Thanks!
> >
> > > Cindy Lu (7):
> > >   vhost: Add a new module_param for enable kthread
> > >   vhost: Add kthread support in function vhost_worker_queue()
> > >   vhost: Add kthread support in function vhost_workers_free()
> > >   vhost: Add the vhost_worker to support kthread
> > >   vhost: Add the cgroup related function
> > >   vhost: Add kthread support in function vhost_worker_create
> > >   vhost: Add new UAPI to support change to task mode
> > >
> > >  drivers/vhost/vhost.c      | 246 +++++++++++++++++++++++++++++++++++--
> > >  drivers/vhost/vhost.h      |   1 +
> > >  include/uapi/linux/vhost.h |   2 +
> > >  3 files changed, 240 insertions(+), 9 deletions(-)
> > >
> > > --
> > > 2.45.0
> >

Re: [RESEND PATCH v1 0/7]vhost: Add support of kthread API

[Jason Wang]

On Tue, Sep 10, 2024 at 4:43 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Tue, Sep 10, 2024 at 04:37:52PM +0800, Jason Wang wrote:
> > On Tue, Sep 10, 2024 at 3:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Mon, Sep 09, 2024 at 10:00:38AM +0800, Cindy Lu wrote:
> > > > In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
> > > > vhost removed the support for the kthread API. However, there are
> > > > still situations where there is a request to use kthread.
> > > > In this PATCH, the support of kthread is added back. Additionally,
> > > > a module_param is added to enforce which mode we are using, and
> > > > a new UAPI is introduced to allow the userspace app to set the
> > > > mode they want to use.
> > > >
> > > > Tested the user application with QEMU.
> > >
> > > Cindy, the APIs do not make sense, security does not make sense,
> > > and you are not describing the issue and the fix.
> > >
> > >
> > > The name should reflect what this does from userspace POV, not from
> > > kernel API POV.  kthread and task mode is not something that any users
> > > have any business even to consider.
> > >
> > >
> > > To help you out, some ideas:
> > >
> > > I think the issue is something like "vhost is now a child of the
> > > owner thread. While this makes sense from containerization
> > > POV, some old userspace is confused, as previously vhost not
> > > and so was allowed to steal cpu resources from outside the container."
> > >
> > > Now, what can be done? Given we already released a secure kernel,
> > > I am not inclined to revert it back to be insecure by default.
> >
> > It depends on how we define "secure". There's plenty of users of
> > kthread and if I was not wrong, mike may still need to fix some bugs.
> >
>
> which bugs?

https://lore.kernel.org/all/06369c2c-c363-4def-9ce0-f018a9e10e8d@oracle.com/T/

>
> > > But I'm fine with a modparam to allow userspace to get insecure
> > > behaviour.
> > >
> > >
> > > Now, a modparam is annoying in that it affects all userspace,
> > > and people might be running a mix of old and new userspace.
> > > So if we do that, we also want a flag that will get
> > > safe behaviour even if unsafe one is allowed.
> >
> > I am not sure this can help. My understanding is that the flag is
> > sufficient. Otherwise the layered product needs to know if there's old
> > user space or new which seems to be a challenge.
> >
> > Thanks
>
> this will be up to userspace to resolve.

I actually mean the module parameter. It would be very hard for the
layered product to decide the value for that.

Thanks

>
>
> > >
> > >
> > > Is this clear enough, or do I need to elaborate more?
> > >
> > > Thanks!
> > >
> > > > Cindy Lu (7):
> > > >   vhost: Add a new module_param for enable kthread
> > > >   vhost: Add kthread support in function vhost_worker_queue()
> > > >   vhost: Add kthread support in function vhost_workers_free()
> > > >   vhost: Add the vhost_worker to support kthread
> > > >   vhost: Add the cgroup related function
> > > >   vhost: Add kthread support in function vhost_worker_create
> > > >   vhost: Add new UAPI to support change to task mode
> > > >
> > > >  drivers/vhost/vhost.c      | 246 +++++++++++++++++++++++++++++++++++--
> > > >  drivers/vhost/vhost.h      |   1 +
> > > >  include/uapi/linux/vhost.h |   2 +
> > > >  3 files changed, 240 insertions(+), 9 deletions(-)
> > > >
> > > > --
> > > > 2.45.0
> > >
>

Re: [RESEND PATCH v1 0/7]vhost: Add support of kthread API

[Michael S. Tsirkin]

On Wed, Sep 11, 2024 at 11:45:33AM +0800, Jason Wang wrote:
> On Tue, Sep 10, 2024 at 4:43 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Tue, Sep 10, 2024 at 04:37:52PM +0800, Jason Wang wrote:
> > > On Tue, Sep 10, 2024 at 3:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Mon, Sep 09, 2024 at 10:00:38AM +0800, Cindy Lu wrote:
> > > > > In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
> > > > > vhost removed the support for the kthread API. However, there are
> > > > > still situations where there is a request to use kthread.
> > > > > In this PATCH, the support of kthread is added back. Additionally,
> > > > > a module_param is added to enforce which mode we are using, and
> > > > > a new UAPI is introduced to allow the userspace app to set the
> > > > > mode they want to use.
> > > > >
> > > > > Tested the user application with QEMU.
> > > >
> > > > Cindy, the APIs do not make sense, security does not make sense,
> > > > and you are not describing the issue and the fix.
> > > >
> > > >
> > > > The name should reflect what this does from userspace POV, not from
> > > > kernel API POV.  kthread and task mode is not something that any users
> > > > have any business even to consider.
> > > >
> > > >
> > > > To help you out, some ideas:
> > > >
> > > > I think the issue is something like "vhost is now a child of the
> > > > owner thread. While this makes sense from containerization
> > > > POV, some old userspace is confused, as previously vhost not
> > > > and so was allowed to steal cpu resources from outside the container."
> > > >
> > > > Now, what can be done? Given we already released a secure kernel,
> > > > I am not inclined to revert it back to be insecure by default.
> > >
> > > It depends on how we define "secure". There's plenty of users of
> > > kthread and if I was not wrong, mike may still need to fix some bugs.
> > >
> >
> > which bugs?
> 
> https://lore.kernel.org/all/06369c2c-c363-4def-9ce0-f018a9e10e8d@oracle.com/T/

Isn't this exactly what Cindy is trying to address?
You made it sound like there is something else.

> >
> > > > But I'm fine with a modparam to allow userspace to get insecure
> > > > behaviour.
> > > >
> > > >
> > > > Now, a modparam is annoying in that it affects all userspace,
> > > > and people might be running a mix of old and new userspace.
> > > > So if we do that, we also want a flag that will get
> > > > safe behaviour even if unsafe one is allowed.
> > >
> > > I am not sure this can help. My understanding is that the flag is
> > > sufficient. Otherwise the layered product needs to know if there's old
> > > user space or new which seems to be a challenge.
> > >
> > > Thanks
> >
> > this will be up to userspace to resolve.
> 
> I actually mean the module parameter. It would be very hard for the
> layered product to decide the value for that.
> 
> Thanks

We can make it writeable, if that helps.


> >
> >
> > > >
> > > >
> > > > Is this clear enough, or do I need to elaborate more?
> > > >
> > > > Thanks!
> > > >
> > > > > Cindy Lu (7):
> > > > >   vhost: Add a new module_param for enable kthread
> > > > >   vhost: Add kthread support in function vhost_worker_queue()
> > > > >   vhost: Add kthread support in function vhost_workers_free()
> > > > >   vhost: Add the vhost_worker to support kthread
> > > > >   vhost: Add the cgroup related function
> > > > >   vhost: Add kthread support in function vhost_worker_create
> > > > >   vhost: Add new UAPI to support change to task mode
> > > > >
> > > > >  drivers/vhost/vhost.c      | 246 +++++++++++++++++++++++++++++++++++--
> > > > >  drivers/vhost/vhost.h      |   1 +
> > > > >  include/uapi/linux/vhost.h |   2 +
> > > > >  3 files changed, 240 insertions(+), 9 deletions(-)
> > > > >
> > > > > --
> > > > > 2.45.0
> > > >
> >

Re: [RESEND PATCH v1 0/7]vhost: Add support of kthread API

[Mike Christie]

On 9/10/24 10:45 PM, Jason Wang wrote:
>>> It depends on how we define "secure". There's plenty of users of
>>> kthread and if I was not wrong, mike may still need to fix some bugs.
>>>
>>
>> which bugs?
> 
> https://lore.kernel.org/all/06369c2c-c363-4def-9ce0-f018a9e10e8d@oracle.com/T/

The SIGKILL issue in that specific email is fixed. The patches are
merged upstream. I don't know of any other bugs like that (crashes,
hangs, etc). There is the one we can't reproduce so are not sure if
it's the vhost changes.

There is the change in behavior type of bug we discussed in that
thread:

https://lore.kernel.org/all/06369c2c-c363-4def-9ce0-f018a9e10e8d@oracle.com/T/#m026f7dd96e064e3a604155e45e7ae9d5c949ae23

Re: [RESEND PATCH v1 0/7]vhost: Add support of kthread API

[Cindy Lu]

On Thu, 12 Sept 2024 at 10:11, Jason Wang <jasowang@redhat.com> wrote:
>
>
>
> On Thu, Sep 12, 2024 at 12:17 AM Mike Christie <michael.christie@oracle.com> wrote:
>>
>> On 9/10/24 10:45 PM, Jason Wang wrote:
>> >>> It depends on how we define "secure". There's plenty of users of
>> >>> kthread and if I was not wrong, mike may still need to fix some bugs.
>> >>>
>> >>
>> >> which bugs?
>> >
>> > https://lore.kernel.org/all/06369c2c-c363-4def-9ce0-f018a9e10e8d@oracle.com/T/
>>
>> The SIGKILL issue in that specific email is fixed. The patches are
>> merged upstream. I don't know of any other bugs like that (crashes,
>> hangs, etc).
>
>
> Ah, ok. Good to know that.
>
>>
>> There is the one we can't reproduce so are not sure if
>> it's the vhost changes.
>>
>>
>> There is the change in behavior type of bug we discussed in that
>> thread:
>>
>> https://lore.kernel.org/all/06369c2c-c363-4def-9ce0-f018a9e10e8d@oracle.com/T/#m026f7dd96e064e3a604155e45e7ae9d5c949ae23
>
>
> Yes.
>
> Thanks
Thank you, Michael and Jason. I will continue to work on this
Thanks
cindy

Re: [RESEND PATCH v1 0/7]vhost: Add support of kthread API

[Mike Christie]

On 9/10/24 2:41 AM, Michael S. Tsirkin wrote:
> On Mon, Sep 09, 2024 at 10:00:38AM +0800, Cindy Lu wrote:
>> In commit 6e890c5d5021 ("vhost: use vhost_tasks for worker threads"),
>> vhost removed the support for the kthread API. However, there are
>> still situations where there is a request to use kthread.
>> In this PATCH, the support of kthread is added back. Additionally,
>> a module_param is added to enforce which mode we are using, and
>> a new UAPI is introduced to allow the userspace app to set the
>> mode they want to use.
>>
>> Tested the user application with QEMU.
> 
> Cindy, the APIs do not make sense, security does not make sense,
> and you are not describing the issue and the fix.
> 
> 
> The name should reflect what this does from userspace POV, not from
> kernel API POV.  kthread and task mode is not something that any users
> have any business even to consider.
> 
> 
> To help you out, some ideas:
> 
> I think the issue is something like "vhost is now a child of the
> owner thread. While this makes sense from containerization
> POV, some old userspace is confused, as previously vhost not
> and so was allowed to steal cpu resources from outside the container."
> 
> Now, what can be done? Given we already released a secure kernel,
> I am not inclined to revert it back to be insecure by default.
> But I'm fine with a modparam to allow userspace to get insecure
> behaviour.
> 
> 
> Now, a modparam is annoying in that it affects all userspace,
> and people might be running a mix of old and new userspace.
> So if we do that, we also want a flag that will get
> safe behaviour even if unsafe one is allowed.
> 
> 
> Is this clear enough, or do I need to elaborate more?
> 
Thanks for working on this Cindy. I've been trying to implement
this in a way where we don't have to duplicate a lot of code but
have been hitting various issues. For example, I've been trying
to modify the vhost_task code so it can emulate the kthread
behavior we had before.

If people are ok with something similar as in this patchset where
we have both vhost_tasks and kthreads, then I can send something.

Re: [RESEND PATCH v1 0/7]vhost: Add support of kthread API

[Michael S. Tsirkin]

On Wed, Sep 11, 2024 at 11:20:30AM -0500, Mike Christie wrote:
> If people are ok with something similar as in this patchset where
> we have both vhost_tasks and kthreads, then I can send something.


It would be better, as you say, to modify the vhost_task code so it can
emulate the kthread behavior. However, given that apparently some users
are unhappy, what you describe would be a good stopgap solution
until we have that.

The main difficulty seems to be to describe what the
behaviour is, exactly, so userspace can make informed
decisions on what to use.

-- 
MST

Re: [RESEND PATCH v1 0/7]vhost: Add support of kthread API

[Jason Wang]

On Thu, Sep 12, 2024 at 3:02 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Sep 11, 2024 at 11:20:30AM -0500, Mike Christie wrote:
> > If people are ok with something similar as in this patchset where
> > we have both vhost_tasks and kthreads, then I can send something.
>
>
> It would be better, as you say, to modify the vhost_task code so it can
> emulate the kthread behavior. However, given that apparently some users
> are unhappy, what you describe would be a good stopgap solution
> until we have that.
>
> The main difficulty seems to be to describe what the
> behaviour is, exactly, so userspace can make informed
> decisions on what to use.

Exactly.

Thanks

>
> --
> MST
>