diff mbox series

[v2,3/6] vhost-net: vhost-user: update vhost_net_virtqueue_reset()

Message ID a14f5ebdefb82d7679841c1d5ddab54ec9406ea1.1662949366.git.kangjie.xu@linux.alibaba.com
State New
Headers show
Series Support VIRTIO_F_RING_RESET for vhost-user in virtio pci-modern | expand

Commit Message

Kangjie Xu Sept. 12, 2022, 3:10 a.m. UTC 
Update vhost_net_virtqueue_reset() for vhost-user scenario.

In order to reuse some functions, we process the idx for
vhost-user scenario because vhost_get_vq_index behave
differently for vhost-user.

Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
---
 hw/net/vhost_net.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Jason Wang Sept. 14, 2022, 3:13 a.m. UTC | #1 
在 2022/9/12 11:10, Kangjie Xu 写道:
> Update vhost_net_virtqueue_reset() for vhost-user scenario.
>
> In order to reuse some functions, we process the idx for
> vhost-user scenario because vhost_get_vq_index behave
> differently for vhost-user.
>
> Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> ---
>   hw/net/vhost_net.c | 3 +++
>   1 file changed, 3 insertions(+)
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index ea896ea75b..25e5665489 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -545,6 +545,9 @@ void vhost_net_virtqueue_reset(VirtIODevice *vdev, NetClientState *nc,
>       assert(vhost_ops);
>   
>       idx = vhost_ops->vhost_get_vq_index(&net->dev, vq_index);
> +    if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
> +        idx -= net->dev.vq_index;
> +    }


This looks tricky. Any reason we can't simply use vq_index for both 
vhost-user and vhost-net?

Thanks


>   
>       if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
>           file.index = idx;
Xuan Zhuo Sept. 14, 2022, 6:18 a.m. UTC | #2 
On Wed, 14 Sep 2022 11:13:29 +0800, Jason Wang <jasowang@redhat.com> wrote:
>
> 在 2022/9/12 11:10, Kangjie Xu 写道:
> > Update vhost_net_virtqueue_reset() for vhost-user scenario.
> >
> > In order to reuse some functions, we process the idx for
> > vhost-user scenario because vhost_get_vq_index behave
> > differently for vhost-user.
> >
> > Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > ---
> >   hw/net/vhost_net.c | 3 +++
> >   1 file changed, 3 insertions(+)
> >
> > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > index ea896ea75b..25e5665489 100644
> > --- a/hw/net/vhost_net.c
> > +++ b/hw/net/vhost_net.c
> > @@ -545,6 +545,9 @@ void vhost_net_virtqueue_reset(VirtIODevice *vdev, NetClientState *nc,
> >       assert(vhost_ops);
> >
> >       idx = vhost_ops->vhost_get_vq_index(&net->dev, vq_index);
> > +    if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
> > +        idx -= net->dev.vq_index;
> > +    }
>
>
> This looks tricky. Any reason we can't simply use vq_index for both
> vhost-user and vhost-net?


static int vhost_user_get_vq_index(struct vhost_dev *dev, int idx)
{
    assert(idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs);

    return idx;
}


static int vhost_kernel_get_vq_index(struct vhost_dev *dev, int idx)
{
    assert(idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs);

    return idx - dev->vq_index;
}

The implementation of these two callbacks is different. The structure of the two
scenarios is different. We may need to do some optimizations in the future.

Thanks.


>
> Thanks
>
>
> >
> >       if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
> >           file.index = idx;
>
Jason Wang Sept. 15, 2022, 2:12 a.m. UTC | #3 
On Wed, Sep 14, 2022 at 2:21 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
>
> On Wed, 14 Sep 2022 11:13:29 +0800, Jason Wang <jasowang@redhat.com> wrote:
> >
> > 在 2022/9/12 11:10, Kangjie Xu 写道:
> > > Update vhost_net_virtqueue_reset() for vhost-user scenario.
> > >
> > > In order to reuse some functions, we process the idx for
> > > vhost-user scenario because vhost_get_vq_index behave
> > > differently for vhost-user.
> > >
> > > Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > ---
> > >   hw/net/vhost_net.c | 3 +++
> > >   1 file changed, 3 insertions(+)
> > >
> > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > index ea896ea75b..25e5665489 100644
> > > --- a/hw/net/vhost_net.c
> > > +++ b/hw/net/vhost_net.c
> > > @@ -545,6 +545,9 @@ void vhost_net_virtqueue_reset(VirtIODevice *vdev, NetClientState *nc,
> > >       assert(vhost_ops);
> > >
> > >       idx = vhost_ops->vhost_get_vq_index(&net->dev, vq_index);
> > > +    if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
> > > +        idx -= net->dev.vq_index;
> > > +    }
> >
> >
> > This looks tricky. Any reason we can't simply use vq_index for both
> > vhost-user and vhost-net?
>
>
> static int vhost_user_get_vq_index(struct vhost_dev *dev, int idx)
> {
>     assert(idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs);
>
>     return idx;
> }
>
>
> static int vhost_kernel_get_vq_index(struct vhost_dev *dev, int idx)
> {
>     assert(idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs);
>
>     return idx - dev->vq_index;
> }
>
> The implementation of these two callbacks is different. The structure of the two
> scenarios is different. We may need to do some optimizations in the future.

Yes, but what I meant is, you do

idx -= net->dev.vq_index;

and then

net->dev.vq_index + idx

This is a hint that we should have a better organization of the code.

Thanks

>
> Thanks.
>
>
> >
> > Thanks
> >
> >
> > >
> > >       if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
> > >           file.index = idx;
> >
>
Xuan Zhuo Sept. 15, 2022, 11:17 a.m. UTC | #4 
On Thu, 15 Sep 2022 10:12:11 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Wed, Sep 14, 2022 at 2:21 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > On Wed, 14 Sep 2022 11:13:29 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > 在 2022/9/12 11:10, Kangjie Xu 写道:
> > > > Update vhost_net_virtqueue_reset() for vhost-user scenario.
> > > >
> > > > In order to reuse some functions, we process the idx for
> > > > vhost-user scenario because vhost_get_vq_index behave
> > > > differently for vhost-user.
> > > >
> > > > Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > ---
> > > >   hw/net/vhost_net.c | 3 +++
> > > >   1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > > index ea896ea75b..25e5665489 100644
> > > > --- a/hw/net/vhost_net.c
> > > > +++ b/hw/net/vhost_net.c
> > > > @@ -545,6 +545,9 @@ void vhost_net_virtqueue_reset(VirtIODevice *vdev, NetClientState *nc,
> > > >       assert(vhost_ops);
> > > >
> > > >       idx = vhost_ops->vhost_get_vq_index(&net->dev, vq_index);
> > > > +    if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
> > > > +        idx -= net->dev.vq_index;
> > > > +    }
> > >
> > >
> > > This looks tricky. Any reason we can't simply use vq_index for both
> > > vhost-user and vhost-net?
> >
> >
> > static int vhost_user_get_vq_index(struct vhost_dev *dev, int idx)
> > {
> >     assert(idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs);
> >
> >     return idx;
> > }
> >
> >
> > static int vhost_kernel_get_vq_index(struct vhost_dev *dev, int idx)
> > {
> >     assert(idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs);
> >
> >     return idx - dev->vq_index;
> > }
> >
> > The implementation of these two callbacks is different. The structure of the two
> > scenarios is different. We may need to do some optimizations in the future.
>
> Yes, but what I meant is, you do
>
> idx -= net->dev.vq_index;
>
> and then
>
> net->dev.vq_index + idx
>
> This is a hint that we should have a better organization of the code.

I see.

Will fix this.

Thanks.


>
> Thanks
>
> >
> > Thanks.
> >
> >
> > >
> > > Thanks
> > >
> > >
> > > >
> > > >       if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
> > > >           file.index = idx;
> > >
> >
>
Xuan Zhuo Sept. 26, 2022, 7:51 a.m. UTC | #5 
On Thu, 15 Sep 2022 10:12:11 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Wed, Sep 14, 2022 at 2:21 PM Xuan Zhuo <xuanzhuo@linux.alibaba.com> wrote:
> >
> > On Wed, 14 Sep 2022 11:13:29 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > 在 2022/9/12 11:10, Kangjie Xu 写道:
> > > > Update vhost_net_virtqueue_reset() for vhost-user scenario.
> > > >
> > > > In order to reuse some functions, we process the idx for
> > > > vhost-user scenario because vhost_get_vq_index behave
> > > > differently for vhost-user.
> > > >
> > > > Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > ---
> > > >   hw/net/vhost_net.c | 3 +++
> > > >   1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> > > > index ea896ea75b..25e5665489 100644
> > > > --- a/hw/net/vhost_net.c
> > > > +++ b/hw/net/vhost_net.c
> > > > @@ -545,6 +545,9 @@ void vhost_net_virtqueue_reset(VirtIODevice *vdev, NetClientState *nc,
> > > >       assert(vhost_ops);
> > > >
> > > >       idx = vhost_ops->vhost_get_vq_index(&net->dev, vq_index);
> > > > +    if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
> > > > +        idx -= net->dev.vq_index;
> > > > +    }
> > >
> > >
> > > This looks tricky. Any reason we can't simply use vq_index for both
> > > vhost-user and vhost-net?
> >
> >
> > static int vhost_user_get_vq_index(struct vhost_dev *dev, int idx)
> > {
> >     assert(idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs);
> >
> >     return idx;
> > }
> >
> >
> > static int vhost_kernel_get_vq_index(struct vhost_dev *dev, int idx)
> > {
> >     assert(idx >= dev->vq_index && idx < dev->vq_index + dev->nvqs);
> >
> >     return idx - dev->vq_index;
> > }
> >
> > The implementation of these two callbacks is different. The structure of the two
> > scenarios is different. We may need to do some optimizations in the future.
>
> Yes, but what I meant is, you do
>
> idx -= net->dev.vq_index;
>
> and then
>
> net->dev.vq_index + idx
>
> This is a hint that we should have a better organization of the code.

Rethink about this.

If I don't do this "idx -= net->dev.vq_index".

Then, it is necessary to call vhost_virtqueue_stop() separately and once again
"net->dev.vqs + idx - net->dev.vq_index"

    vhost_virtqueue_stop(&net->dev,
                         vdev,
                         net->dev.vqs + idx - net->dev.vq_index,
                         idx);

Thanks.


>
> Thanks
>
> >
> > Thanks.
> >
> >
> > >
> > > Thanks
> > >
> > >
> > > >
> > > >       if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
> > > >           file.index = idx;
> > >
> >
>
diff mbox series

Patch

diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
index ea896ea75b..25e5665489 100644
--- a/hw/net/vhost_net.c
+++ b/hw/net/vhost_net.c
@@ -545,6 +545,9 @@  void vhost_net_virtqueue_reset(VirtIODevice *vdev, NetClientState *nc,
     assert(vhost_ops);
 
     idx = vhost_ops->vhost_get_vq_index(&net->dev, vq_index);
+    if (net->nc->info->type == NET_CLIENT_DRIVER_VHOST_USER) {
+        idx -= net->dev.vq_index;
+    }
 
     if (net->nc->info->type == NET_CLIENT_DRIVER_TAP) {
         file.index = idx;