[Qemu-devel] [PATCH 0/2] vhost: two fixes

Jay Zhou posted 2 patches 6 years, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/1513269392-23224-1-git-send-email-jianjay.zhou@huawei.com
Test checkpatch passed
Test docker passed
Test ppc passed
Test s390x passed
There is a newer version of this series
hw/virtio/vhost-user.c            | 31 +++++++++++++++++++++++++
hw/virtio/vhost.c                 | 49 ++++++++++++++++++++++++++++++++++-----
include/hw/virtio/vhost-backend.h |  4 ++++
3 files changed, 78 insertions(+), 6 deletions(-)
[Qemu-devel] [PATCH 0/2] vhost: two fixes
Posted by Jay Zhou 6 years, 4 months ago
Jay Zhou (2):
  vhost: add used memslot number for vhost-user
  vhost: double check memslot number

 hw/virtio/vhost-user.c            | 31 +++++++++++++++++++++++++
 hw/virtio/vhost.c                 | 49 ++++++++++++++++++++++++++++++++++-----
 include/hw/virtio/vhost-backend.h |  4 ++++
 3 files changed, 78 insertions(+), 6 deletions(-)

-- 
1.8.3.1



Re: [Qemu-devel] [PATCH 0/2] vhost: two fixes
Posted by Michael S. Tsirkin 6 years, 4 months ago
On Fri, Dec 15, 2017 at 12:36:30AM +0800, Jay Zhou wrote:
> Jay Zhou (2):
>   vhost: add used memslot number for vhost-user
>   vhost: double check memslot number
> 
>  hw/virtio/vhost-user.c            | 31 +++++++++++++++++++++++++
>  hw/virtio/vhost.c                 | 49 ++++++++++++++++++++++++++++++++++-----
>  include/hw/virtio/vhost-backend.h |  4 ++++
>  3 files changed, 78 insertions(+), 6 deletions(-)

Cc two developers working on these files right now.

> -- 
> 1.8.3.1
> 

Re: [Qemu-devel] [PATCH 0/2] vhost: two fixes
Posted by Dr. David Alan Gilbert 6 years, 4 months ago
* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Fri, Dec 15, 2017 at 12:36:30AM +0800, Jay Zhou wrote:
> > Jay Zhou (2):
> >   vhost: add used memslot number for vhost-user
> >   vhost: double check memslot number
> > 
> >  hw/virtio/vhost-user.c            | 31 +++++++++++++++++++++++++
> >  hw/virtio/vhost.c                 | 49 ++++++++++++++++++++++++++++++++++-----
> >  include/hw/virtio/vhost-backend.h |  4 ++++
> >  3 files changed, 78 insertions(+), 6 deletions(-)
> 
> Cc two developers working on these files right now.

I have to admit to not understanding the 'used_memslots' variable.

* It's a global in vhost.c
* but set by vhost_set_memory that's called from the listener associated
  with each individual vhost
* While they're probably always the same, the merging code calls
  the vhost_backend_can_merge method for each device, so the number
  of regions can be different.

Dave

> > -- 
> > 1.8.3.1
> > 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 0/2] vhost: two fixes
Posted by Zhoujian (jay) 6 years, 4 months ago
Hi Dave,

> -----Original Message-----
> From: Dr. David Alan Gilbert [mailto:dgilbert@redhat.com]
> Sent: Friday, December 15, 2017 3:49 AM
> To: Michael S. Tsirkin <mst@redhat.com>
> Cc: Zhoujian (jay) <jianjay.zhou@huawei.com>; qemu-devel@nongnu.org;
> Huangweidong (C) <weidong.huang@huawei.com>; Gonglei (Arei)
> <arei.gonglei@huawei.com>; wangxin (U) <wangxinxin.wang@huawei.com>;
> Liuzhe (Cloud Open Labs, NFV) <gary.liuzhe@huawei.com>; Igor Mammedov
> <imammedo@redhat.com>
> Subject: Re: [PATCH 0/2] vhost: two fixes
> 
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > On Fri, Dec 15, 2017 at 12:36:30AM +0800, Jay Zhou wrote:
> > > Jay Zhou (2):
> > >   vhost: add used memslot number for vhost-user
> > >   vhost: double check memslot number
> > >
> > >  hw/virtio/vhost-user.c            | 31 +++++++++++++++++++++++++
> > >  hw/virtio/vhost.c                 | 49
> ++++++++++++++++++++++++++++++++++-----
> > >  include/hw/virtio/vhost-backend.h |  4 ++++
> > >  3 files changed, 78 insertions(+), 6 deletions(-)
> >
> > Cc two developers working on these files right now.
> 
> I have to admit to not understanding the 'used_memslots' variable.
> 
> * It's a global in vhost.c
> * but set by vhost_set_memory that's called from the listener associated
>   with each individual vhost
> * While they're probably always the same, the merging code calls
>   the vhost_backend_can_merge method for each device, so the number
>   of regions can be different.
> 

Your mean for some devices the new added MemoryRegionSection can be merged,
but for others it can not be merged? IIUC the vhost_mem for each vhost_dev
is the same.

Meanwhile, I think it is more reasonable to add globals in vhost-backend.c
and vhost-user.c respectively instead of 'used_memslots'. The reason
is explained in patch 1. What do you think?

Regards,
Jay

Re: [Qemu-devel] [PATCH 0/2] vhost: two fixes
Posted by Michael S. Tsirkin 6 years, 4 months ago
On Fri, Dec 15, 2017 at 02:38:35AM +0000, Zhoujian (jay) wrote:
> Hi Dave,
> 
> > -----Original Message-----
> > From: Dr. David Alan Gilbert [mailto:dgilbert@redhat.com]
> > Sent: Friday, December 15, 2017 3:49 AM
> > To: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Zhoujian (jay) <jianjay.zhou@huawei.com>; qemu-devel@nongnu.org;
> > Huangweidong (C) <weidong.huang@huawei.com>; Gonglei (Arei)
> > <arei.gonglei@huawei.com>; wangxin (U) <wangxinxin.wang@huawei.com>;
> > Liuzhe (Cloud Open Labs, NFV) <gary.liuzhe@huawei.com>; Igor Mammedov
> > <imammedo@redhat.com>
> > Subject: Re: [PATCH 0/2] vhost: two fixes
> > 
> > * Michael S. Tsirkin (mst@redhat.com) wrote:
> > > On Fri, Dec 15, 2017 at 12:36:30AM +0800, Jay Zhou wrote:
> > > > Jay Zhou (2):
> > > >   vhost: add used memslot number for vhost-user
> > > >   vhost: double check memslot number
> > > >
> > > >  hw/virtio/vhost-user.c            | 31 +++++++++++++++++++++++++
> > > >  hw/virtio/vhost.c                 | 49
> > ++++++++++++++++++++++++++++++++++-----
> > > >  include/hw/virtio/vhost-backend.h |  4 ++++
> > > >  3 files changed, 78 insertions(+), 6 deletions(-)
> > >
> > > Cc two developers working on these files right now.
> > 
> > I have to admit to not understanding the 'used_memslots' variable.
> > 
> > * It's a global in vhost.c
> > * but set by vhost_set_memory that's called from the listener associated
> >   with each individual vhost
> > * While they're probably always the same, the merging code calls
> >   the vhost_backend_can_merge method for each device, so the number
> >   of regions can be different.
> > 
> 
> Your mean for some devices the new added MemoryRegionSection can be merged,
> but for others it can not be merged? IIUC the vhost_mem for each vhost_dev
> is the same.
> 
> Meanwhile, I think it is more reasonable to add globals in vhost-backend.c
> and vhost-user.c respectively instead of 'used_memslots'. The reason
> is explained in patch 1. What do you think?
> 
> Regards,
> Jay

I'd rather avoid globals completely if possible.

-- 
MST

Re: [Qemu-devel] [PATCH 0/2] vhost: two fixes
Posted by Zhoujian (jay) 6 years, 4 months ago
Hi Michael,

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Friday, December 15, 2017 12:36 PM
> To: Zhoujian (jay) <jianjay.zhou@huawei.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>; qemu-devel@nongnu.org;
> Huangweidong (C) <weidong.huang@huawei.com>; Gonglei (Arei)
> <arei.gonglei@huawei.com>; wangxin (U) <wangxinxin.wang@huawei.com>;
> Liuzhe (Cloud Open Labs, NFV) <gary.liuzhe@huawei.com>; Igor Mammedov
> <imammedo@redhat.com>
> Subject: Re: [PATCH 0/2] vhost: two fixes
> 
> On Fri, Dec 15, 2017 at 02:38:35AM +0000, Zhoujian (jay) wrote:
> > Hi Dave,
> >
> > > -----Original Message-----
> > > From: Dr. David Alan Gilbert [mailto:dgilbert@redhat.com]
> > > Sent: Friday, December 15, 2017 3:49 AM
> > > To: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Zhoujian (jay) <jianjay.zhou@huawei.com>; qemu-devel@nongnu.org;
> > > Huangweidong (C) <weidong.huang@huawei.com>; Gonglei (Arei)
> > > <arei.gonglei@huawei.com>; wangxin (U) <wangxinxin.wang@huawei.com>;
> > > Liuzhe (Cloud Open Labs, NFV) <gary.liuzhe@huawei.com>; Igor
> > > Mammedov <imammedo@redhat.com>
> > > Subject: Re: [PATCH 0/2] vhost: two fixes
> > >
> > > * Michael S. Tsirkin (mst@redhat.com) wrote:
> > > > On Fri, Dec 15, 2017 at 12:36:30AM +0800, Jay Zhou wrote:
> > > > > Jay Zhou (2):
> > > > >   vhost: add used memslot number for vhost-user
> > > > >   vhost: double check memslot number
> > > > >
> > > > >  hw/virtio/vhost-user.c            | 31 +++++++++++++++++++++++++
> > > > >  hw/virtio/vhost.c                 | 49
> > > ++++++++++++++++++++++++++++++++++-----
> > > > >  include/hw/virtio/vhost-backend.h |  4 ++++
> > > > >  3 files changed, 78 insertions(+), 6 deletions(-)
> > > >
> > > > Cc two developers working on these files right now.
> > >
> > > I have to admit to not understanding the 'used_memslots' variable.
> > >
> > > * It's a global in vhost.c
> > > * but set by vhost_set_memory that's called from the listener
> associated
> > >   with each individual vhost
> > > * While they're probably always the same, the merging code calls
> > >   the vhost_backend_can_merge method for each device, so the number
> > >   of regions can be different.
> > >
> >
> > Your mean for some devices the new added MemoryRegionSection can be
> > merged, but for others it can not be merged? IIUC the vhost_mem for
> > each vhost_dev is the same.
> >
> > Meanwhile, I think it is more reasonable to add globals in
> > vhost-backend.c and vhost-user.c respectively instead of
> > 'used_memslots'. The reason is explained in patch 1. What do you think?
> >
> > Regards,
> > Jay
> 
> I'd rather avoid globals completely if possible.
> 

It is possible, we could add a 'used_memslots' variable in struct vhost_dev
for per device. I will try to do it in v2.

Regards,
Jay

Re: [Qemu-devel] [PATCH 0/2] vhost: two fixes
Posted by Dr. David Alan Gilbert 6 years, 4 months ago
* Zhoujian (jay) (jianjay.zhou@huawei.com) wrote:
> Hi Dave,
> 
> > -----Original Message-----
> > From: Dr. David Alan Gilbert [mailto:dgilbert@redhat.com]
> > Sent: Friday, December 15, 2017 3:49 AM
> > To: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Zhoujian (jay) <jianjay.zhou@huawei.com>; qemu-devel@nongnu.org;
> > Huangweidong (C) <weidong.huang@huawei.com>; Gonglei (Arei)
> > <arei.gonglei@huawei.com>; wangxin (U) <wangxinxin.wang@huawei.com>;
> > Liuzhe (Cloud Open Labs, NFV) <gary.liuzhe@huawei.com>; Igor Mammedov
> > <imammedo@redhat.com>
> > Subject: Re: [PATCH 0/2] vhost: two fixes
> > 
> > * Michael S. Tsirkin (mst@redhat.com) wrote:
> > > On Fri, Dec 15, 2017 at 12:36:30AM +0800, Jay Zhou wrote:
> > > > Jay Zhou (2):
> > > >   vhost: add used memslot number for vhost-user
> > > >   vhost: double check memslot number
> > > >
> > > >  hw/virtio/vhost-user.c            | 31 +++++++++++++++++++++++++
> > > >  hw/virtio/vhost.c                 | 49
> > ++++++++++++++++++++++++++++++++++-----
> > > >  include/hw/virtio/vhost-backend.h |  4 ++++
> > > >  3 files changed, 78 insertions(+), 6 deletions(-)
> > >
> > > Cc two developers working on these files right now.
> > 
> > I have to admit to not understanding the 'used_memslots' variable.
> > 
> > * It's a global in vhost.c
> > * but set by vhost_set_memory that's called from the listener associated
> >   with each individual vhost
> > * While they're probably always the same, the merging code calls
> >   the vhost_backend_can_merge method for each device, so the number
> >   of regions can be different.
> > 
> 
> Your mean for some devices the new added MemoryRegionSection can be merged,
> but for others it can not be merged?

That's my understanding, because of the call to vhost_backend_can_merge

> IIUC the vhost_mem for each vhost_dev
> is the same.
> 
> Meanwhile, I think it is more reasonable to add globals in vhost-backend.c
> and vhost-user.c respectively instead of 'used_memslots'. The reason
> is explained in patch 1. What do you think?

If we need globals I'm not sure it matters that much where they live.

Dave

> Regards,
> Jay
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK