netdev - 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: <CACLfguU_xqxu5b0D8RoEFAb5Z8Xp4H8v2f5UeLG74kSZcthfOw@mail.gmail.com>
Date: Wed, 27 Nov 2024 14:43:44 +0800
From: Cindy Lu <lulu@...hat.com>
To: michael.christie@...cle.com
Cc: 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 Wed, Nov 27, 2024 at 5:20 AM <michael.christie@...cle.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