On Mon, Jun 01, 2015 at 01:53:35PM +0800, Jason Wang wrote:
>
>
> On 06/01/2015 02:12 AM, Michael S. Tsirkin wrote:
> > On Tue, May 26, 2015 at 01:54:09AM -0400, Jason Wang wrote:
> >> We allocate one vhost log per vhost device. This is sub optimal when:
> >>
> >> - Guest has several device with vhost as backend
> >> - Guest has multiqueue devices
> >>
> >> Since each vhost device will allocate and use their private log, this
> >> could cause very huge unnecessary memory allocation. We can in fact
> >> avoid extra cost by sharing a single vhost log among all the vhost
> >> devices. This is feasible since:
> >>
> >> - All vhost devices for a vm should see a consistent memory layout
> >> (memory slots).
> >> - Vhost log were design to be shared, all access function were
> >> synchronized.
> >>
> >> So this patch tries to implement the sharing through
> >>
> >> - Introducing a new vhost_log structure and refcnt it.
> >> - Using a global pointer of vhost_log structure to keep track the
> >> current log used.
> >> - If there's no resize, next vhost device will just use this log and
> >> increase the refcnt.
> >> - If resize happens, a new vhost_log structure will be allocated and
> >> each vhost device will use the new log then drop the refcnt of old log.
> >> - The old log will be synced and freed when reference count drops to zero.
> >>
> >> Tested by doing scp during migration for a 2 queues virtio-net-pci.
> >>
> >> Cc: Michael S. Tsirkin <m...@redhat.com>
> >> Signed-off-by: Jason Wang <jasow...@redhat.com>
> > Almost there I think.
> >
> >> ---
> >> Changes from V3:
> >> - only sync the old log on put
> >> Changes from V2:
> >> - rebase to HEAD
> >> - drop unused node field from vhost_log structure
> >> Changes from V1:
> >> - Drop the list of vhost log, instead, using a global pointer instead
> >> ---
> >> hw/virtio/vhost.c | 78
> >> ++++++++++++++++++++++++++++++++++++-----------
> >> include/hw/virtio/vhost.h | 8 ++++-
> >> 2 files changed, 68 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >> index 54851b7..fef28d9 100644
> >> --- a/hw/virtio/vhost.c
> >> +++ b/hw/virtio/vhost.c
> >> @@ -22,15 +22,19 @@
> >> #include "hw/virtio/virtio-bus.h"
> >> #include "migration/migration.h"
> >>
> >> +static struct vhost_log *vhost_log;
> >> +
> >> static void vhost_dev_sync_region(struct vhost_dev *dev,
> >> MemoryRegionSection *section,
> >> uint64_t mfirst, uint64_t mlast,
> >> uint64_t rfirst, uint64_t rlast)
> >> {
> >> + vhost_log_chunk_t *log = dev->log->log;
> >> +
> >> uint64_t start = MAX(mfirst, rfirst);
> >> uint64_t end = MIN(mlast, rlast);
> >> - vhost_log_chunk_t *from = dev->log + start / VHOST_LOG_CHUNK;
> >> - vhost_log_chunk_t *to = dev->log + end / VHOST_LOG_CHUNK + 1;
> >> + vhost_log_chunk_t *from = log + start / VHOST_LOG_CHUNK;
> >> + vhost_log_chunk_t *to = log + end / VHOST_LOG_CHUNK + 1;
> >> uint64_t addr = (start / VHOST_LOG_CHUNK) * VHOST_LOG_CHUNK;
> >>
> >> if (end < start) {
> >> @@ -280,22 +284,57 @@ static uint64_t vhost_get_log_size(struct vhost_dev
> >> *dev)
> >> }
> >> return log_size;
> >> }
> >> +static struct vhost_log *vhost_log_alloc(uint64_t size)
> >> +{
> >> + struct vhost_log *log = g_malloc0(sizeof *log + size *
> >> sizeof(*(log->log)));
> >> +
> >> + log->size = size;
> >> + log->refcnt = 1;
> >> +
> >> + return log;
> >> +}
> >> +
> >> +static struct vhost_log *vhost_log_get(uint64_t size)
> >> +{
> >> + if (!vhost_log || vhost_log->size != size) {
> >> + vhost_log = vhost_log_alloc(size);
> >> + } else {
> >> + ++vhost_log->refcnt;
> >> + }
> >> +
> >> + return vhost_log;
> >> +}
> >> +
> >> +static void vhost_log_put(struct vhost_dev *dev, bool sync)
> >> +{
> >> + struct vhost_log *log = dev->log;
> >> +
> >> + if (!log) {
> >> + return;
> >> + }
> >> +
> >> + --log->refcnt;
> >> + if (log->refcnt == 0) {
> >> + /* Sync only the range covered by the old log */
> >> + if (dev->log_size && sync) {
> >> + vhost_log_sync_range(dev, 0, dev->log_size * VHOST_LOG_CHUNK
> >> - 1);
> >> + }
> >> + if (vhost_log == log) {
> >> + vhost_log = NULL;
> >> + }
> >> + g_free(log);
> >> + }
> >> +}
> >>
> >> static inline void vhost_dev_log_resize(struct vhost_dev* dev, uint64_t
> >> size)
> >> {
> >> - vhost_log_chunk_t *log;
> >> - uint64_t log_base;
> >> + struct vhost_log *log = vhost_log_get(size);
> >> + uint64_t log_base = (uintptr_t)log->log;
> >> int r;
> >>
> >> - log = g_malloc0(size * sizeof *log);
> >> - log_base = (uintptr_t)log;
> >> r = dev->vhost_ops->vhost_call(dev, VHOST_SET_LOG_BASE, &log_base);
> >> assert(r >= 0);
> >> - /* Sync only the range covered by the old log */
> >> - if (dev->log_size) {
> >> - vhost_log_sync_range(dev, 0, dev->log_size * VHOST_LOG_CHUNK - 1);
> >> - }
> >> - g_free(dev->log);
> >> + vhost_log_put(dev, true);
> >> dev->log = log;
> >> dev->log_size = size;
> >> }
> >> @@ -601,7 +640,7 @@ static int vhost_migration_log(MemoryListener
> >> *listener, int enable)
> >> if (r < 0) {
> >> return r;
> >> }
> >> - g_free(dev->log);
> >> + vhost_log_put(dev, false);
> >> dev->log = NULL;
> >> dev->log_size = 0;
> >> } else {
> >> @@ -1060,10 +1099,12 @@ int vhost_dev_start(struct vhost_dev *hdev,
> >> VirtIODevice *vdev)
> >> uint64_t log_base;
> >>
> >> hdev->log_size = vhost_get_log_size(hdev);
> >> - hdev->log = hdev->log_size ?
> >> - g_malloc0(hdev->log_size * sizeof *hdev->log) : NULL;
> >> - log_base = (uintptr_t)hdev->log;
> >> - r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE,
> >> &log_base);
> >> + if (hdev->log_size) {
> >> + hdev->log = vhost_log_get(hdev->log_size);
> >> + }
> > Why is this conditional on hdev->log_size?
> > What will the value be for log_size == 0?
>
> This is used to save an unnecessary allocation for a dummy vhost_log
> structure without any log.
Then you need to go over all uses and make sure they
are safe. A dummy vhost_log structure might be easier.
> >> + log_base = (uintptr_t)hdev->log->log;
> > especially since we de-reference it here.
>
> Yes, this seems unsafe, will change this to
>
> log_base = hdev->log_size ? (uintptr_t) hdev->log->log : NULL
> >> + r = hdev->vhost_ops->vhost_call(hdev, VHOST_SET_LOG_BASE,
> >> + hdev->log_size ? &log_base :
> >> NULL);
> >> if (r < 0) {
> >> r = -errno;
> >> goto fail_log;
> >> @@ -1072,6 +1113,9 @@ int vhost_dev_start(struct vhost_dev *hdev,
> >> VirtIODevice *vdev)
> >>
> >> return 0;
> >> fail_log:
> >> + if (hdev->log_size) {
> >> + vhost_log_put(hdev, false);
> >> + }
> >> fail_vq:
> >> while (--i >= 0) {
> >> vhost_virtqueue_stop(hdev,
> >> @@ -1101,7 +1145,7 @@ void vhost_dev_stop(struct vhost_dev *hdev,
> >> VirtIODevice *vdev)
> >> vhost_log_sync_range(hdev, 0, ~0x0ull);
> >>
> >> hdev->started = false;
> >> - g_free(hdev->log);
> >> + vhost_log_put(hdev, false);
> >> hdev->log = NULL;
> >> hdev->log_size = 0;
> >> }
> > Why false? We better sync the dirty bitmap since log is getting
> > cleared.
>
> We did a vhost_log_sync_range(hdev, 0, ~0x0ull) before. And we only sync
> 0 to dev->log_size * VHOST_LOG_CHUNK - 1 in vhost_log_put(). But looks
> like there's no difference, will remove vhost_log_sync_range() and use
> true for vhost_log_put() here.
> >
> > And if it's always true, we can just drop this flag.
>
> There's still other usage, e.g when fail to setting log base in
> vhost_dev_start() or migration end. In those cases, no need to sync.
We definitely must sync on migration end.
> >
> >> diff --git a/include/hw/virtio/vhost.h b/include/hw/virtio/vhost.h
> >> index 8f04888..816a2e8 100644
> >> --- a/include/hw/virtio/vhost.h
> >> +++ b/include/hw/virtio/vhost.h
> >> @@ -28,6 +28,12 @@ typedef unsigned long vhost_log_chunk_t;
> >> #define VHOST_LOG_CHUNK (VHOST_LOG_PAGE * VHOST_LOG_BITS)
> >> #define VHOST_INVALID_FEATURE_BIT (0xff)
> >>
> >> +struct vhost_log {
> >> + unsigned long long size;
> >> + int refcnt;
> >> + vhost_log_chunk_t log[0];
> >> +};
> >> +
> >> struct vhost_memory;
> >> struct vhost_dev {
> >> MemoryListener memory_listener;
> >> @@ -43,7 +49,6 @@ struct vhost_dev {
> >> unsigned long long backend_features;
> >> bool started;
> >> bool log_enabled;
> >> - vhost_log_chunk_t *log;
> >> unsigned long long log_size;
> >> Error *migration_blocker;
> >> bool force;
> >> @@ -52,6 +57,7 @@ struct vhost_dev {
> >> hwaddr mem_changed_end_addr;
> >> const VhostOps *vhost_ops;
> >> void *opaque;
> >> + struct vhost_log *log;
> >> };
> >>
> >> int vhost_dev_init(struct vhost_dev *hdev, void *opaque,
> >> --
> >> 1.8.3.1
> >>
