On Wed, Nov 27, 2024 at 5:20 AM <michael.chris...@oracle.com> wrote:
>
> 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.
sure, will change this
>
>
> > +
> > 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.
> */
>
Thanks, Mike， I will change this
>
>
> > + 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.
>
sure, Will remove this
> > + 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.
>
>
Will fix this. Thanks
>
> > + 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.
>
Sure, Thanks I will rewrite this part
Thanks
cindy
