On Thu, Oct 12, 2023 at 6:44?PM Xianting Tian
<xianting.tian at linux.alibaba.com> wrote:>
>
> ? 2023/10/12 ??3:55, Jason Wang ??:
> > On Thu, Oct 12, 2023 at 9:43?AM Xianting Tian
> > <xianting.tian at linux.alibaba.com> wrote:
> >> cgroup attach work and dev flush work will both be added to dev
work
> >> list in vhost_attach_cgroups() when set dev owner:
> >> static int vhost_attach_cgroups(struct vhost_dev
*dev)
> >> {
> >> struct vhost_attach_cgroups_struct attach;
> >>
> >> attach.owner = current;
> >> vhost_work_init(&attach.work,
> >> vhost_attach_cgroups_work);
> >> vhost_work_queue(dev, &attach.work); //
add cgroup
> >> attach work
> >> vhost_work_dev_flush(dev); // add
dev
> >> flush work
> >> return attach.ret;
> >> }
> >>
> >> And dev kworker will be waken up to handle the two works in
> >> vhost_worker():
> >> node = llist_del_all(&dev->work_list);
> >> node = llist_reverse_order(node);
> >> llist_for_each_entry_safe{
> >> work->fn(work);
> >> }
> >>
> >> As the list is reversed before processing in vhost_worker(),
so it is
> >> possible
> >> that dev flush work is processed before cgroup attach work.
> > This sounds weird. It's llist not list so when adding the new
entry
> > was added to the head that why we need llist_reverse_order() to
> > recover the order.
> >
> > Have you ever reproduced these issues?
>
> Sorry for the disturb, No issue now.
>
> It caused by our internal changes.
If it's an optimization or features, you are welcomed to post them.
Developing new features upstream has a lot of benefits.
Thanks
>
> >
> > Thanks
> >
> >> If so,
> >> vhost_attach_cgroups
> >> may return "attach.ret" before cgroup attach work is
handled, but
> >> "attach.ret" is random
> >> value as it is in stack.
> >>
> >> The possible fix maybe:
> >>
> >> static int vhost_attach_cgroups(struct vhost_dev *dev)
> >> {
> >> struct vhost_attach_cgroups_struct attach;
> >>
> >> attach.ret = 0;
> >> attach.owner = current;
> >> vhost_work_init(&attach.work,
vhost_attach_cgroups_work);
> >> vhost_work_queue(dev, &attach.work);
> >> vhost_work_dev_flush(dev);
> >> return attach.ret;
> >> }
> >>
> >> So this fix is just to initialize the attach.ret to 0, this fix
may
> >> not the final fix,
> >> We just want you experts know this issue exists, and we met it
> >> recently in our test.
> >>
> >> And the issue exists in may stable branches.
> >>
>