| Message ID | 1513269392-23224-1-git-send-email-jianjay.zhou@huawei.com |
|---|---|
| Headers | show |
| Series | vhost: two fixes | expand |
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 >
* 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
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
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.
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
Hi Michael, > -----Original Message----- > From: Zhoujian (jay) > Sent: Friday, December 15, 2017 12:52 PM > To: 'Michael S. Tsirkin' <mst@redhat.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 > > 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. > If the globals don't exist, the disadvantage I found is that the check "if memslots number exceeds" will be moved from the beginning to the end in vhost_dev_init, does it acceptable? Or are there other ideas to avoid globals? To be honest, I prefer to add globals in vhost-backend.c and vhost-user.c respectively, the value of used_memslots for the same type of backend is the same. Regards, Jay
* 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