Re: Report a possible vhost bug in stable branches

2023-10-12 Thread Jason Wang
On Thu, Oct 12, 2023 at 6:44 PM Xianting Tian
 wrote:
>
>
> 在 2023/10/12 下午3:55, Jason Wang 写道:
> > On Thu, Oct 12, 2023 at 9:43 AM Xianting Tian
> >  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(,
> >>  vhost_attach_cgroups_work);
> >>   vhost_work_queue(dev, ); // 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(>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(, vhost_attach_cgroups_work);
> >>   vhost_work_queue(dev, );
> >>   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.
> >>
>

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: Report a possible vhost bug in stable branches

2023-10-12 Thread Jason Wang
On Thu, Oct 12, 2023 at 9:43 AM Xianting Tian
 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(,
> vhost_attach_cgroups_work);
>  vhost_work_queue(dev, ); // 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(>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?

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(, vhost_attach_cgroups_work);
>  vhost_work_queue(dev, );
>  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.
>

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization