linux-kernel - Re: [PATCH v3 4/9] vhost: Add kthread support in function vhost_worker

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5dcd0aa9-f098-4aac-a053-9693761ff550@oracle.com>
Date: Tue, 26 Nov 2024 15:19:52 -0600
From: michael.christie@...cle.com
To: Cindy Lu <lulu@...hat.com>, jasowang@...hat.com, mst@...hat.com,
        sgarzare@...hat.com, linux-kernel@...r.kernel.org,
        virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org
Subject: Re: [PATCH v3 4/9] vhost: Add kthread support in function
 vhost_worker_create

On 11/5/24 1:25 AM, Cindy Lu wrote:
>  static struct vhost_worker *vhost_worker_create(struct vhost_dev *dev)
>  {
>  	struct vhost_worker *worker;
> -	struct vhost_task *vtsk;
> +	struct vhost_task *vtsk = NULL;
> +	struct task_struct *task = NULL;
>  	char name[TASK_COMM_LEN];
>  	int ret;
>  	u32 id;
>  
> +	/* Allocate resources for the worker */
>  	worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT);
>  	if (!worker)
>  		return NULL;
>  
> +	worker->fn = kzalloc(sizeof(struct vhost_task_fn), GFP_KERNEL_ACCOUNT);
> +	if (!worker->fn) {
> +		kfree(worker);
> +		return NULL;
> +	}

Why dynamically allocate this?

You could probably even just kill the vhost_task_fn struct since we just
have to the 2 callouts.

> +
>  	worker->dev = dev;
>  	snprintf(name, sizeof(name), "vhost-%d", current->pid);
>  
> -	vtsk = vhost_task_create(vhost_run_work_list, vhost_worker_killed,
> -				 worker, name);
> -	if (!vtsk)
> -		goto free_worker;
> -
>  	mutex_init(&worker->mutex);
>  	init_llist_head(&worker->work_list);
>  	worker->kcov_handle = kcov_common_handle();
> -	worker->vtsk = vtsk;
>  
> -	vhost_task_start(vtsk);
> +	if (dev->inherit_owner) {
> +		/* Create and start a vhost task */

Maybe instead of this comment and the one below write something about
what inherit_owner means. We can see from the code we are creating a
vhost/kthread, but it's not really obvious why. Something like:

/*
 * If inherit_owner is true we use vhost_tasks to create
 * the worker so all settings/limits like cgroups, NPROC,
 * scheduler, etc are inherited from the owner. If false,
 * we use kthreads and only attach to the same cgroups
 * as the owner for compat with older kernels.
 */

> +		vtsk = vhost_task_create(vhost_run_work_list,
> +					 vhost_worker_killed, worker, name);
> +		if (!vtsk)
> +			goto free_worker;
> +
> +		worker->vtsk = vtsk;
> +		worker->fn->wakeup = vhost_task_wakeup_fn;
> +		worker->fn->stop = vhost_task_stop_fn;
> +
> +		vhost_task_start(vtsk);
> +	} else {
> +		/* Create and start a kernel thread */
> +		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;
> +		worker->fn->wakeup = vhost_kthread_wakeup_fn;
> +		worker->fn->stop = vhost_kthread_stop_fn;
> +
> +		wake_up_process(task);
> +		/* Attach to the vhost cgroup */

You don't need this comment do you? The function name tells us the same
info.

> +		ret = vhost_attach_cgroups(dev);

I don't think this works. Patch 3/9 did:

+	xa_for_each(&dev->worker_xa, i, worker) {
+		ret = vhost_worker_cgroups_kthread(worker);

but we don't add the worker to the xa until below.

You also want to just call vhost_worker_cgroups_kthread above, because
you only want to add the one task and not loop over all of them.

I would then also maybe rename vhost_worker_cgroups_kthread to something
like vhost_attach_task_to_cgroups.

> +		if (ret)
> +			goto stop_worker;
> +	}
>  
>  	ret = xa_alloc(&dev->worker_xa, &id, worker, xa_limit_32b, GFP_KERNEL);
>  	if (ret < 0)
>  		goto stop_worker;
>  	worker->id = id;
> -
>  	return worker;
> -
>  stop_worker:
> -	vhost_task_stop(vtsk);
> +	worker->fn->stop(dev->inherit_owner ? (void *)vtsk : (void *)task);

I don't think you need to cast since the function takes a void pointer.
Same comment for the other patches like 6/9 where you are calling the
callout and casting.