[PATCH v3 0/3] two optimizations to speed up the start time

[PATCH v3 0/3] two optimizations to speed up the start time

[Longpeng(Mike) via]

From: Longpeng <longpeng2@huawei.com>

Changes v3->v2:
 - cleanup the code [Philippe]

Changes v2->v1:
 Patch-1:
  - remove vq_init_count [Jason]
 Patch-2:
  - new added. [Jason]

v1: https://www.mail-archive.com/qemu-devel@nongnu.org/msg922499.html

Longpeng (Mike) (3):
  vhost: simplify vhost_dev_enable_notifiers
  vhost: configure all host notifiers in a single MR transaction
  vdpa: commit all host notifier MRs in a single MR transaction

 hw/virtio/vhost-vdpa.c | 25 ++++++++++++++++++------
 hw/virtio/vhost.c      | 44 +++++++++++++++++++++++++++---------------
 2 files changed, 47 insertions(+), 22 deletions(-)

-- 
2.23.0

[PATCH v3 1/3] vhost: simplify vhost_dev_enable_notifiers

[Longpeng(Mike) via]

From: Longpeng <longpeng2@huawei.com>

Simplify the error path in vhost_dev_enable_notifiers by using
vhost_dev_disable_notifiers directly.

Signed-off-by: Longpeng <longpeng2@huawei.com>
---
 hw/virtio/vhost.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index fdcd1a8fdf..5994559da8 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1551,7 +1551,7 @@ void vhost_dev_cleanup(struct vhost_dev *hdev)
 int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
 {
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
-    int i, r, e;
+    int i, r;
 
     /* We will pass the notifiers to the kernel, make sure that QEMU
      * doesn't interfere.
@@ -1559,7 +1559,7 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
     r = virtio_device_grab_ioeventfd(vdev);
     if (r < 0) {
         error_report("binding does not support host notifiers");
-        goto fail;
+        return r;
     }
 
     for (i = 0; i < hdev->nvqs; ++i) {
@@ -1567,24 +1567,12 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
                                          true);
         if (r < 0) {
             error_report("vhost VQ %d notifier binding failed: %d", i, -r);
-            goto fail_vq;
+            vhost_dev_disable_notifiers(hdev, vdev);
+            return r;
         }
     }
 
     return 0;
-fail_vq:
-    while (--i >= 0) {
-        e = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i,
-                                         false);
-        if (e < 0) {
-            error_report("vhost VQ %d notifier cleanup error: %d", i, -r);
-        }
-        assert (e >= 0);
-        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i);
-    }
-    virtio_device_release_ioeventfd(vdev);
-fail:
-    return r;
 }
 
 /* Stop processing guest IO notifications in vhost.
-- 
2.23.0

[PATCH v3 2/3] vhost: configure all host notifiers in a single MR transaction

[Longpeng(Mike) via]

From: Longpeng <longpeng2@huawei.com>

This allows the vhost device to batch the setup of all its host notifiers.
This significantly reduces the device starting time, e.g. the time spend
on enabling notifiers reduce from 376ms to 9.1ms for a VM with 64 vCPUs
and 3 vhost-vDPA generic devices (vdpa_sim_blk, 64vq per device)

Signed-off-by: Longpeng <longpeng2@huawei.com>
---
 hw/virtio/vhost.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 5994559da8..064d4abe5c 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1562,16 +1562,25 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
         return r;
     }
 
+    /*
+     * Batch all the host notifiers in a single transaction to avoid
+     * quadratic time complexity in address_space_update_ioeventfds().
+     */
+    memory_region_transaction_begin();
+
     for (i = 0; i < hdev->nvqs; ++i) {
         r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i,
                                          true);
         if (r < 0) {
             error_report("vhost VQ %d notifier binding failed: %d", i, -r);
+            memory_region_transaction_commit();
             vhost_dev_disable_notifiers(hdev, vdev);
             return r;
         }
     }
 
+    memory_region_transaction_commit();
+
     return 0;
 }
 
@@ -1585,6 +1594,12 @@ void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
     int i, r;
 
+    /*
+     * Batch all the host notifiers in a single transaction to avoid
+     * quadratic time complexity in address_space_update_ioeventfds().
+     */
+    memory_region_transaction_begin();
+
     for (i = 0; i < hdev->nvqs; ++i) {
         r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i,
                                          false);
@@ -1592,6 +1607,15 @@ void vhost_dev_disable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
             error_report("vhost VQ %d notifier cleanup failed: %d", i, -r);
         }
         assert (r >= 0);
+    }
+
+    /*
+     * The transaction expects the ioeventfds to be open when it
+     * commits. Do it now, before the cleanup loop.
+     */
+    memory_region_transaction_commit();
+
+    for (i = 0; i < hdev->nvqs; ++i) {
         virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i);
     }
     virtio_device_release_ioeventfd(vdev);
-- 
2.23.0

[PATCH v3 3/3] vdpa: commit all host notifier MRs in a single MR transaction

[Longpeng(Mike) via]

From: Longpeng <longpeng2@huawei.com>

This allows the vhost-vdpa device to batch the setup of all its MRs of
host notifiers.

This significantly reduces the device starting time, e.g. the time spend
on setup the host notifier MRs reduce from 423ms to 32ms for a VM with
64 vCPUs and 3 vhost-vDPA generic devices (vdpa_sim_blk, 64vq per device).

Signed-off-by: Longpeng <longpeng2@huawei.com>
---
 hw/virtio/vhost-vdpa.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index fd0c33b0e1..870265188a 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -512,9 +512,18 @@ static void vhost_vdpa_host_notifiers_uninit(struct vhost_dev *dev, int n)
 {
     int i;
 
+    /*
+     * Pack all the changes to the memory regions in a single
+     * transaction to avoid a few updating of the address space
+     * topology.
+     */
+    memory_region_transaction_begin();
+
     for (i = dev->vq_index; i < dev->vq_index + n; i++) {
         vhost_vdpa_host_notifier_uninit(dev, i);
     }
+
+    memory_region_transaction_commit();
 }
 
 static void vhost_vdpa_host_notifiers_init(struct vhost_dev *dev)
@@ -527,17 +536,21 @@ static void vhost_vdpa_host_notifiers_init(struct vhost_dev *dev)
         return;
     }
 
+    /*
+     * Pack all the changes to the memory regions in a single
+     * transaction to avoid a few updating of the address space
+     * topology.
+     */
+    memory_region_transaction_begin();
+
     for (i = dev->vq_index; i < dev->vq_index + dev->nvqs; i++) {
         if (vhost_vdpa_host_notifier_init(dev, i)) {
-            goto err;
+            vhost_vdpa_host_notifiers_uninit(dev, i - dev->vq_index);
+            break;
         }
     }
 
-    return;
-
-err:
-    vhost_vdpa_host_notifiers_uninit(dev, i - dev->vq_index);
-    return;
+    memory_region_transaction_commit();
 }
 
 static void vhost_vdpa_svq_cleanup(struct vhost_dev *dev)
-- 
2.23.0

Re: [PATCH v3 2/3] vhost: configure all host notifiers in a single MR transaction

[Philippe Mathieu-Daudé]

On 27/12/22 08:20, Longpeng(Mike) wrote:
> From: Longpeng <longpeng2@huawei.com>
> 
> This allows the vhost device to batch the setup of all its host notifiers.
> This significantly reduces the device starting time, e.g. the time spend
> on enabling notifiers reduce from 376ms to 9.1ms for a VM with 64 vCPUs
> and 3 vhost-vDPA generic devices (vdpa_sim_blk, 64vq per device)
> 
> Signed-off-by: Longpeng <longpeng2@huawei.com>
> ---
>   hw/virtio/vhost.c | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 5994559da8..064d4abe5c 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1562,16 +1562,25 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
>           return r;
>       }
>   
> +    /*
> +     * Batch all the host notifiers in a single transaction to avoid
> +     * quadratic time complexity in address_space_update_ioeventfds().
> +     */
> +    memory_region_transaction_begin();
> +
>       for (i = 0; i < hdev->nvqs; ++i) {
>           r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i,
>                                            true);
>           if (r < 0) {
>               error_report("vhost VQ %d notifier binding failed: %d", i, -r);
> +            memory_region_transaction_commit();
>               vhost_dev_disable_notifiers(hdev, vdev);

Could we 'break' here, ...

>               return r;
>           }
>       }
>   
> +    memory_region_transaction_commit();
> +
>       return 0;

... and return 'r' here?

>   }

Otherwise LGTM.

Re: [PATCH v3 3/3] vdpa: commit all host notifier MRs in a single MR transaction

[Philippe Mathieu-Daudé]

On 27/12/22 08:20, Longpeng(Mike) wrote:
> From: Longpeng <longpeng2@huawei.com>
> 
> This allows the vhost-vdpa device to batch the setup of all its MRs of
> host notifiers.
> 
> This significantly reduces the device starting time, e.g. the time spend
> on setup the host notifier MRs reduce from 423ms to 32ms for a VM with
> 64 vCPUs and 3 vhost-vDPA generic devices (vdpa_sim_blk, 64vq per device).
> 
> Signed-off-by: Longpeng <longpeng2@huawei.com>
> ---
>   hw/virtio/vhost-vdpa.c | 25 +++++++++++++++++++------
>   1 file changed, 19 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index fd0c33b0e1..870265188a 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -512,9 +512,18 @@ static void vhost_vdpa_host_notifiers_uninit(struct vhost_dev *dev, int n)
>   {
>       int i;
>   
> +    /*
> +     * Pack all the changes to the memory regions in a single
> +     * transaction to avoid a few updating of the address space
> +     * topology.
> +     */
> +    memory_region_transaction_begin();
> +
>       for (i = dev->vq_index; i < dev->vq_index + n; i++) {
>           vhost_vdpa_host_notifier_uninit(dev, i);
>       }
> +
> +    memory_region_transaction_commit();
>   }

Instead of optimizing one frontend, I wonder if we shouldn't expose
a 'bool memory_region_transaction_in_progress()' helper in memory.c,
and in virtio_queue_set_host_notifier_mr() backend, assert we are
within a transaction. That way we'd optimize all virtio frontends.

Re: [PATCH v3 2/3] vhost: configure all host notifiers in a single MR transaction

[Michael S. Tsirkin]

On Tue, Dec 27, 2022 at 05:43:57PM +0100, Philippe Mathieu-Daudé wrote:
> On 27/12/22 08:20, Longpeng(Mike) wrote:
> > From: Longpeng <longpeng2@huawei.com>
> > 
> > This allows the vhost device to batch the setup of all its host notifiers.
> > This significantly reduces the device starting time, e.g. the time spend
> > on enabling notifiers reduce from 376ms to 9.1ms for a VM with 64 vCPUs
> > and 3 vhost-vDPA generic devices (vdpa_sim_blk, 64vq per device)
> > 
> > Signed-off-by: Longpeng <longpeng2@huawei.com>
> > ---
> >   hw/virtio/vhost.c | 24 ++++++++++++++++++++++++
> >   1 file changed, 24 insertions(+)
> > 
> > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> > index 5994559da8..064d4abe5c 100644
> > --- a/hw/virtio/vhost.c
> > +++ b/hw/virtio/vhost.c
> > @@ -1562,16 +1562,25 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
> >           return r;
> >       }
> > +    /*
> > +     * Batch all the host notifiers in a single transaction to avoid
> > +     * quadratic time complexity in address_space_update_ioeventfds().
> > +     */
> > +    memory_region_transaction_begin();
> > +
> >       for (i = 0; i < hdev->nvqs; ++i) {
> >           r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i,
> >                                            true);
> >           if (r < 0) {
> >               error_report("vhost VQ %d notifier binding failed: %d", i, -r);
> > +            memory_region_transaction_commit();
> >               vhost_dev_disable_notifiers(hdev, vdev);
> 
> Could we 'break' here, ...
> 
> >               return r;
> >           }
> >       }
> > +    memory_region_transaction_commit();
> > +
> >       return 0;
> 
> ... and return 'r' here?


won't this commit twice? seems ugly ...

> >   }
> 
> Otherwise LGTM.

Re: [PATCH v3 3/3] vdpa: commit all host notifier MRs in a single MR transaction

[Michael S. Tsirkin]

On Tue, Dec 27, 2022 at 05:51:47PM +0100, Philippe Mathieu-Daudé wrote:
> On 27/12/22 08:20, Longpeng(Mike) wrote:
> > From: Longpeng <longpeng2@huawei.com>
> > 
> > This allows the vhost-vdpa device to batch the setup of all its MRs of
> > host notifiers.
> > 
> > This significantly reduces the device starting time, e.g. the time spend
> > on setup the host notifier MRs reduce from 423ms to 32ms for a VM with
> > 64 vCPUs and 3 vhost-vDPA generic devices (vdpa_sim_blk, 64vq per device).
> > 
> > Signed-off-by: Longpeng <longpeng2@huawei.com>
> > ---
> >   hw/virtio/vhost-vdpa.c | 25 +++++++++++++++++++------
> >   1 file changed, 19 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index fd0c33b0e1..870265188a 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -512,9 +512,18 @@ static void vhost_vdpa_host_notifiers_uninit(struct vhost_dev *dev, int n)
> >   {
> >       int i;
> > +    /*
> > +     * Pack all the changes to the memory regions in a single
> > +     * transaction to avoid a few updating of the address space
> > +     * topology.
> > +     */
> > +    memory_region_transaction_begin();
> > +
> >       for (i = dev->vq_index; i < dev->vq_index + n; i++) {
> >           vhost_vdpa_host_notifier_uninit(dev, i);
> >       }
> > +
> > +    memory_region_transaction_commit();
> >   }
> 
> Instead of optimizing one frontend, I wonder if we shouldn't expose
> a 'bool memory_region_transaction_in_progress()' helper in memory.c,
> and in virtio_queue_set_host_notifier_mr() backend, assert we are
> within a transaction. That way we'd optimize all virtio frontends.


If we are doing something like this, I'd rather pass around
a "transaction" structure so this can be checked statically.
Looks like something that can be done on top though.

-- 
MST

Re: [PATCH v3 2/3] vhost: configure all host notifiers in a single MR transaction

[Philippe Mathieu-Daudé]

On 27/12/22 18:54, Michael S. Tsirkin wrote:
> On Tue, Dec 27, 2022 at 05:43:57PM +0100, Philippe Mathieu-Daudé wrote:
>> On 27/12/22 08:20, Longpeng(Mike) wrote:
>>> From: Longpeng <longpeng2@huawei.com>
>>>
>>> This allows the vhost device to batch the setup of all its host notifiers.
>>> This significantly reduces the device starting time, e.g. the time spend
>>> on enabling notifiers reduce from 376ms to 9.1ms for a VM with 64 vCPUs
>>> and 3 vhost-vDPA generic devices (vdpa_sim_blk, 64vq per device)
>>>
>>> Signed-off-by: Longpeng <longpeng2@huawei.com>
>>> ---
>>>    hw/virtio/vhost.c | 24 ++++++++++++++++++++++++
>>>    1 file changed, 24 insertions(+)
>>>
>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>> index 5994559da8..064d4abe5c 100644
>>> --- a/hw/virtio/vhost.c
>>> +++ b/hw/virtio/vhost.c
>>> @@ -1562,16 +1562,25 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
>>>            return r;
>>>        }
>>> +    /*
>>> +     * Batch all the host notifiers in a single transaction to avoid
>>> +     * quadratic time complexity in address_space_update_ioeventfds().
>>> +     */
>>> +    memory_region_transaction_begin();
>>> +
>>>        for (i = 0; i < hdev->nvqs; ++i) {
>>>            r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i,
>>>                                             true);
>>>            if (r < 0) {
>>>                error_report("vhost VQ %d notifier binding failed: %d", i, -r);
>>> +            memory_region_transaction_commit();
>>>                vhost_dev_disable_notifiers(hdev, vdev);
>>
>> Could we 'break' here, ...
>>
>>>                return r;
>>>            }
>>>        }
>>> +    memory_region_transaction_commit();
>>> +
>>>        return 0;
>>
>> ... and return 'r' here?
> 
> 
> won't this commit twice? seems ugly ...

Twice? I meant keep the begin/commit() around the for() to have
only *one* commit() call instead of 2:

-- >8 --
+    memory_region_transaction_begin();
+
      for (i = 0; i < hdev->nvqs; ++i) {
          r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), 
hdev->vq_index + i,
                                           true);
          if (r < 0) {
              error_report("vhost VQ %d notifier binding failed: %d", i, 
-r);
              vhost_dev_disable_notifiers(hdev, vdev);
-            return r;
+            break;
          }
      }

+    memory_region_transaction_commit();
+
-    return 0;
+    return r;
  }
---

Anyhow,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 3/3] vdpa: commit all host notifier MRs in a single MR transaction

[Philippe Mathieu-Daudé]

On 27/12/22 18:56, Michael S. Tsirkin wrote:
> On Tue, Dec 27, 2022 at 05:51:47PM +0100, Philippe Mathieu-Daudé wrote:
>> On 27/12/22 08:20, Longpeng(Mike) wrote:
>>> From: Longpeng <longpeng2@huawei.com>
>>>
>>> This allows the vhost-vdpa device to batch the setup of all its MRs of
>>> host notifiers.
>>>
>>> This significantly reduces the device starting time, e.g. the time spend
>>> on setup the host notifier MRs reduce from 423ms to 32ms for a VM with
>>> 64 vCPUs and 3 vhost-vDPA generic devices (vdpa_sim_blk, 64vq per device).
>>>
>>> Signed-off-by: Longpeng <longpeng2@huawei.com>
>>> ---
>>>    hw/virtio/vhost-vdpa.c | 25 +++++++++++++++++++------
>>>    1 file changed, 19 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>>> index fd0c33b0e1..870265188a 100644
>>> --- a/hw/virtio/vhost-vdpa.c
>>> +++ b/hw/virtio/vhost-vdpa.c
>>> @@ -512,9 +512,18 @@ static void vhost_vdpa_host_notifiers_uninit(struct vhost_dev *dev, int n)
>>>    {
>>>        int i;
>>> +    /*
>>> +     * Pack all the changes to the memory regions in a single
>>> +     * transaction to avoid a few updating of the address space
>>> +     * topology.
>>> +     */
>>> +    memory_region_transaction_begin();
>>> +
>>>        for (i = dev->vq_index; i < dev->vq_index + n; i++) {
>>>            vhost_vdpa_host_notifier_uninit(dev, i);
>>>        }
>>> +
>>> +    memory_region_transaction_commit();
>>>    }
>>
>> Instead of optimizing one frontend, I wonder if we shouldn't expose
>> a 'bool memory_region_transaction_in_progress()' helper in memory.c,
>> and in virtio_queue_set_host_notifier_mr() backend, assert we are
>> within a transaction. That way we'd optimize all virtio frontends.
> 
> 
> If we are doing something like this, I'd rather pass around
> a "transaction" structure so this can be checked statically.

Ah, clever.

> Looks like something that can be done on top though.

Sure.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH v3 2/3] vhost: configure all host notifiers in a single MR transaction

[longpeng2--- via]

在 2022/12/28 21:12, Philippe Mathieu-Daudé 写道:
> On 27/12/22 18:54, Michael S. Tsirkin wrote:
>> On Tue, Dec 27, 2022 at 05:43:57PM +0100, Philippe Mathieu-Daudé wrote:
>>> On 27/12/22 08:20, Longpeng(Mike) wrote:
>>>> From: Longpeng <longpeng2@huawei.com>
>>>>
>>>> This allows the vhost device to batch the setup of all its host 
>>>> notifiers.
>>>> This significantly reduces the device starting time, e.g. the time 
>>>> spend
>>>> on enabling notifiers reduce from 376ms to 9.1ms for a VM with 64 vCPUs
>>>> and 3 vhost-vDPA generic devices (vdpa_sim_blk, 64vq per device)
>>>>
>>>> Signed-off-by: Longpeng <longpeng2@huawei.com>
>>>> ---
>>>>    hw/virtio/vhost.c | 24 ++++++++++++++++++++++++
>>>>    1 file changed, 24 insertions(+)
>>>>
>>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>>> index 5994559da8..064d4abe5c 100644
>>>> --- a/hw/virtio/vhost.c
>>>> +++ b/hw/virtio/vhost.c
>>>> @@ -1562,16 +1562,25 @@ int vhost_dev_enable_notifiers(struct 
>>>> vhost_dev *hdev, VirtIODevice *vdev)
>>>>            return r;
>>>>        }
>>>> +    /*
>>>> +     * Batch all the host notifiers in a single transaction to avoid
>>>> +     * quadratic time complexity in address_space_update_ioeventfds().
>>>> +     */
>>>> +    memory_region_transaction_begin();
>>>> +
>>>>        for (i = 0; i < hdev->nvqs; ++i) {
>>>>            r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), 
>>>> hdev->vq_index + i,
>>>>                                             true);
>>>>            if (r < 0) {
>>>>                error_report("vhost VQ %d notifier binding failed: 
>>>> %d", i, -r);
>>>> +            memory_region_transaction_commit();
>>>>                vhost_dev_disable_notifiers(hdev, vdev);
>>>
>>> Could we 'break' here, ...
>>>
>>>>                return r;
>>>>            }
>>>>        }
>>>> +    memory_region_transaction_commit();
>>>> +
>>>>        return 0;
>>>
>>> ... and return 'r' here?
>>
>>
>> won't this commit twice? seems ugly ...
> 
> Twice? I meant keep the begin/commit() around the for() to have
> only *one* commit() call instead of 2:
> 

There's a transaction in vhost_dev_disable_notifiers() too, We must 
commit the outter transaction before invoking it, you can see the 
comment in it.

> -- >8 --
> +    memory_region_transaction_begin();
> +
>       for (i = 0; i < hdev->nvqs; ++i) {
>           r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), 
> hdev->vq_index + i,
>                                            true);
>           if (r < 0) {
>               error_report("vhost VQ %d notifier binding failed: %d", i, 
> -r);
>               vhost_dev_disable_notifiers(hdev, vdev);
> -            return r;
> +            break;
>           }
>       }
> 
> +    memory_region_transaction_commit();
> +
> -    return 0;
> +    return r;
>   }
> ---
> 
> Anyhow,
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> .