[PATCH v2 0/2] two optimizations to speed up the start time

[PATCH v2 0/2] two optimizations to speed up the start time

[Longpeng(Mike) via]

From: Longpeng <longpeng2@huawei.com>

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) (2):
  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 | 18 ++++++++++++++++++
 hw/virtio/vhost.c      | 40 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 56 insertions(+), 2 deletions(-)

-- 
2.23.0

[PATCH v2 1/2] 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[1] (64vq per device)

[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg921541.html

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

diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
index 7fb008bc9e..16f8391d86 100644
--- a/hw/virtio/vhost.c
+++ b/hw/virtio/vhost.c
@@ -1507,7 +1507,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, n, r, e;
 
     /* We will pass the notifiers to the kernel, make sure that QEMU
      * doesn't interfere.
@@ -1518,6 +1518,12 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
         goto fail;
     }
 
+    /*
+     * 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);
@@ -1527,8 +1533,12 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
         }
     }
 
+    memory_region_transaction_commit();
+
     return 0;
 fail_vq:
+    /* save i for a second iteration after transaction is committed. */
+    n = i;
     while (--i >= 0) {
         e = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i,
                                          false);
@@ -1536,8 +1546,18 @@ fail_vq:
             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);
     }
+
+    /*
+     * The transaction expects the ioeventfds to be open when it
+     * commits. Do it now, before the cleanup loop.
+     */
+    memory_region_transaction_commit();
+
+    while (--n >= 0) {
+        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + n);
+    }
+
     virtio_device_release_ioeventfd(vdev);
 fail:
     return r;
@@ -1553,6 +1573,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);
@@ -1560,8 +1586,18 @@ 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 v2 2/2] 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[1] (64vq per device).

[1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg921541.html

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

diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 7468e44b87..eb233cf08a 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -547,9 +547,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)
@@ -562,16 +571,25 @@ 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;
         }
     }
 
+    memory_region_transaction_commit();
     return;
 
 err:
     vhost_vdpa_host_notifiers_uninit(dev, i - dev->vq_index);
+    memory_region_transaction_commit();
     return;
 }
 
-- 
2.23.0

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

[Philippe Mathieu-Daudé]

On 6/12/22 09:18, Longpeng(Mike) via 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[1] (64vq per device).
> 
> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg921541.html
> 
> Signed-off-by: Longpeng <longpeng2@huawei.com>
> ---
>   hw/virtio/vhost-vdpa.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)


> @@ -562,16 +571,25 @@ 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;

Could we simplify by replacing this goto statement with:

                vhost_vdpa_host_notifiers_uninit(dev, i - dev->vq_index);
                break;

?

>           }
>       }
>   
> +    memory_region_transaction_commit();
>       return;
>   
>   err:
>       vhost_vdpa_host_notifiers_uninit(dev, i - dev->vq_index);
> +    memory_region_transaction_commit();
>       return;
>   }
>   

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

[longpeng2--- via]

在 2022/12/6 16:30, Philippe Mathieu-Daudé 写道:
> On 6/12/22 09:18, Longpeng(Mike) via 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[1] (64vq per device).
>>
>> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg921541.html
>>
>> Signed-off-by: Longpeng <longpeng2@huawei.com>
>> ---
>>   hw/virtio/vhost-vdpa.c | 18 ++++++++++++++++++
>>   1 file changed, 18 insertions(+)
> 
> 
>> @@ -562,16 +571,25 @@ 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;
> 
> Could we simplify by replacing this goto statement with:
> 
>                 vhost_vdpa_host_notifiers_uninit(dev, i - dev->vq_index);
>                 break;
> 
> ?
> 
Good suggestion! I'll do in v3, thanks.

>>           }
>>       }
>> +    memory_region_transaction_commit();
>>       return;
>>   err:
>>       vhost_vdpa_host_notifiers_uninit(dev, i - dev->vq_index);
>> +    memory_region_transaction_commit();
>>       return;
>>   }
> 
> .

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

[Philippe Mathieu-Daudé]

On 6/12/22 09:18, Longpeng(Mike) via 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[1] (64vq per device)
> 
> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg921541.html
> 
> Signed-off-by: Longpeng <longpeng2@huawei.com>
> ---
>   hw/virtio/vhost.c | 40 ++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> index 7fb008bc9e..16f8391d86 100644
> --- a/hw/virtio/vhost.c
> +++ b/hw/virtio/vhost.c
> @@ -1507,7 +1507,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, n, r, e;
>   
>       /* We will pass the notifiers to the kernel, make sure that QEMU
>        * doesn't interfere.
> @@ -1518,6 +1518,12 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
>           goto fail;
>       }
>   
> +    /*
> +     * 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);
> @@ -1527,8 +1533,12 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, VirtIODevice *vdev)
>           }
>       }
>   
> +    memory_region_transaction_commit();
> +
>       return 0;
>   fail_vq:
> +    /* save i for a second iteration after transaction is committed. */
> +    n = i;
>       while (--i >= 0) {
>           e = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + i,
>                                            false);
> @@ -1536,8 +1546,18 @@ fail_vq:
>               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);
>       }
> +
> +    /*
> +     * The transaction expects the ioeventfds to be open when it
> +     * commits. Do it now, before the cleanup loop.
> +     */
> +    memory_region_transaction_commit();
> +
> +    while (--n >= 0) {
> +        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + n);
> +    }
> +
>       virtio_device_release_ioeventfd(vdev);
>   fail:
>       return r;

Similarly to patch #2, removing both goto statement in this function (as 
a preliminary patch) will 1/ simplify the code 2/ simplify reviewing 
your changes, resulting in something like:

int vhost_dev_enable_notifiers(struct vhost_dev *hdev,
                                VirtIODevice *vdev)
{
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
     int i, r, e;

     /* We will pass the notifiers to the kernel, make sure that QEMU
      * doesn't interfere.
      */
     r = virtio_device_grab_ioeventfd(vdev);
     if (r < 0) {
         error_report("binding does not support host notifiers");
         return r;
     }

     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);
             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);
             break;
         }
     }

     memory_region_transaction_commit();

     return r;
}

What do you think?

Regards,

Phil.

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

[longpeng2--- via]

在 2022/12/6 17:07, Philippe Mathieu-Daudé 写道:
> On 6/12/22 09:18, Longpeng(Mike) via 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[1] (64vq per device)
>>
>> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg921541.html
>>
>> Signed-off-by: Longpeng <longpeng2@huawei.com>
>> ---
>>   hw/virtio/vhost.c | 40 ++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 38 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index 7fb008bc9e..16f8391d86 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -1507,7 +1507,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, n, r, e;
>>       /* We will pass the notifiers to the kernel, make sure that QEMU
>>        * doesn't interfere.
>> @@ -1518,6 +1518,12 @@ int vhost_dev_enable_notifiers(struct vhost_dev 
>> *hdev, VirtIODevice *vdev)
>>           goto fail;
>>       }
>> +    /*
>> +     * 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);
>> @@ -1527,8 +1533,12 @@ int vhost_dev_enable_notifiers(struct vhost_dev 
>> *hdev, VirtIODevice *vdev)
>>           }
>>       }
>> +    memory_region_transaction_commit();
>> +
>>       return 0;
>>   fail_vq:
>> +    /* save i for a second iteration after transaction is committed. */
>> +    n = i;
>>       while (--i >= 0) {
>>           e = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), 
>> hdev->vq_index + i,
>>                                            false);
>> @@ -1536,8 +1546,18 @@ fail_vq:
>>               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);
>>       }
>> +
>> +    /*
>> +     * The transaction expects the ioeventfds to be open when it
>> +     * commits. Do it now, before the cleanup loop.
>> +     */
>> +    memory_region_transaction_commit();
>> +
>> +    while (--n >= 0) {
>> +        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), 
>> hdev->vq_index + n);
>> +    }
>> +
>>       virtio_device_release_ioeventfd(vdev);
>>   fail:
>>       return r;
> 
> Similarly to patch #2, removing both goto statement in this function (as 
> a preliminary patch) will 1/ simplify the code 2/ simplify reviewing 
> your changes, resulting in something like:
> 
> int vhost_dev_enable_notifiers(struct vhost_dev *hdev,
>                                 VirtIODevice *vdev)
> {
>      BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
>      int i, r, e;
> 
>      /* We will pass the notifiers to the kernel, make sure that QEMU
>       * doesn't interfere.
>       */
>      r = virtio_device_grab_ioeventfd(vdev);
>      if (r < 0) {
>          error_report("binding does not support host notifiers");
>          return r;
>      }
> 
>      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);
>              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);
>              break;
>          }
>      }
> 
>      memory_region_transaction_commit();
> 
>      return r;
> }
> 
> What do you think?
> 
Maybe we can use vhost_dev_disable_notifiers to further simplify the 
error path ?

And we must commit before invoking virtio_bus_cleanup_host_notifier.

> Regards,
> 
> Phil.
> .

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

[Philippe Mathieu-Daudé]

On 6/12/22 11:28, Longpeng (Mike, Cloud Infrastructure Service Product 
Dept.) wrote:
> 
> 
> 在 2022/12/6 17:07, Philippe Mathieu-Daudé 写道:
>> On 6/12/22 09:18, Longpeng(Mike) via 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[1] (64vq per device)
>>>
>>> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg921541.html
>>>
>>> Signed-off-by: Longpeng <longpeng2@huawei.com>
>>> ---
>>>   hw/virtio/vhost.c | 40 ++++++++++++++++++++++++++++++++++++++--
>>>   1 file changed, 38 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>> index 7fb008bc9e..16f8391d86 100644
>>> --- a/hw/virtio/vhost.c
>>> +++ b/hw/virtio/vhost.c
>>> @@ -1507,7 +1507,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, n, r, e;
>>>       /* We will pass the notifiers to the kernel, make sure that QEMU
>>>        * doesn't interfere.
>>> @@ -1518,6 +1518,12 @@ int vhost_dev_enable_notifiers(struct 
>>> vhost_dev *hdev, VirtIODevice *vdev)
>>>           goto fail;
>>>       }
>>> +    /*
>>> +     * 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);
>>> @@ -1527,8 +1533,12 @@ int vhost_dev_enable_notifiers(struct 
>>> vhost_dev *hdev, VirtIODevice *vdev)
>>>           }
>>>       }
>>> +    memory_region_transaction_commit();
>>> +
>>>       return 0;
>>>   fail_vq:
>>> +    /* save i for a second iteration after transaction is committed. */
>>> +    n = i;
>>>       while (--i >= 0) {
>>>           e = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), 
>>> hdev->vq_index + i,
>>>                                            false);
>>> @@ -1536,8 +1546,18 @@ fail_vq:
>>>               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);
>>>       }
>>> +
>>> +    /*
>>> +     * The transaction expects the ioeventfds to be open when it
>>> +     * commits. Do it now, before the cleanup loop.
>>> +     */
>>> +    memory_region_transaction_commit();
>>> +
>>> +    while (--n >= 0) {
>>> +        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), 
>>> hdev->vq_index + n);
>>> +    }
>>> +
>>>       virtio_device_release_ioeventfd(vdev);
>>>   fail:
>>>       return r;
>>
>> Similarly to patch #2, removing both goto statement in this function 
>> (as a preliminary patch) will 1/ simplify the code 2/ simplify 
>> reviewing your changes, resulting in something like:
>>
>> int vhost_dev_enable_notifiers(struct vhost_dev *hdev,
>>                                 VirtIODevice *vdev)
>> {
>>      BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
>>      int i, r, e;
>>
>>      /* We will pass the notifiers to the kernel, make sure that QEMU
>>       * doesn't interfere.
>>       */
>>      r = virtio_device_grab_ioeventfd(vdev);
>>      if (r < 0) {
>>          error_report("binding does not support host notifiers");
>>          return r;
>>      }
>>
>>      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);
>>              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);
>>              break;
>>          }
>>      }
>>
>>      memory_region_transaction_commit();
>>
>>      return r;
>> }
>>
>> What do you think?
>>
> Maybe we can use vhost_dev_disable_notifiers to further simplify the 
> error path ?

Good idea, but having the BusState resolved on each call seems a waste.
Eventually factor it out and pass as argument ...

> And we must commit before invoking virtio_bus_cleanup_host_notifier.

... but with that info on top, finally your original patch is simpler.

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

[longpeng2--- via]

在 2022/12/6 18:45, Philippe Mathieu-Daudé 写道:
> On 6/12/22 11:28, Longpeng (Mike, Cloud Infrastructure Service Product 
> Dept.) wrote:
>>
>>
>> 在 2022/12/6 17:07, Philippe Mathieu-Daudé 写道:
>>> On 6/12/22 09:18, Longpeng(Mike) via 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[1] (64vq per device)
>>>>
>>>> [1] https://www.mail-archive.com/qemu-devel@nongnu.org/msg921541.html
>>>>
>>>> Signed-off-by: Longpeng <longpeng2@huawei.com>
>>>> ---
>>>>   hw/virtio/vhost.c | 40 ++++++++++++++++++++++++++++++++++++++--
>>>>   1 file changed, 38 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>>>> index 7fb008bc9e..16f8391d86 100644
>>>> --- a/hw/virtio/vhost.c
>>>> +++ b/hw/virtio/vhost.c
>>>> @@ -1507,7 +1507,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, n, r, e;
>>>>       /* We will pass the notifiers to the kernel, make sure that QEMU
>>>>        * doesn't interfere.
>>>> @@ -1518,6 +1518,12 @@ int vhost_dev_enable_notifiers(struct 
>>>> vhost_dev *hdev, VirtIODevice *vdev)
>>>>           goto fail;
>>>>       }
>>>> +    /*
>>>> +     * 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);
>>>> @@ -1527,8 +1533,12 @@ int vhost_dev_enable_notifiers(struct 
>>>> vhost_dev *hdev, VirtIODevice *vdev)
>>>>           }
>>>>       }
>>>> +    memory_region_transaction_commit();
>>>> +
>>>>       return 0;
>>>>   fail_vq:
>>>> +    /* save i for a second iteration after transaction is 
>>>> committed. */
>>>> +    n = i;
>>>>       while (--i >= 0) {
>>>>           e = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), 
>>>> hdev->vq_index + i,
>>>>                                            false);
>>>> @@ -1536,8 +1546,18 @@ fail_vq:
>>>>               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);
>>>>       }
>>>> +
>>>> +    /*
>>>> +     * The transaction expects the ioeventfds to be open when it
>>>> +     * commits. Do it now, before the cleanup loop.
>>>> +     */
>>>> +    memory_region_transaction_commit();
>>>> +
>>>> +    while (--n >= 0) {
>>>> +        virtio_bus_cleanup_host_notifier(VIRTIO_BUS(qbus), 
>>>> hdev->vq_index + n);
>>>> +    }
>>>> +
>>>>       virtio_device_release_ioeventfd(vdev);
>>>>   fail:
>>>>       return r;
>>>
>>> Similarly to patch #2, removing both goto statement in this function 
>>> (as a preliminary patch) will 1/ simplify the code 2/ simplify 
>>> reviewing your changes, resulting in something like:
>>>
>>> int vhost_dev_enable_notifiers(struct vhost_dev *hdev,
>>>                                 VirtIODevice *vdev)
>>> {
>>>      BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
>>>      int i, r, e;
>>>
>>>      /* We will pass the notifiers to the kernel, make sure that QEMU
>>>       * doesn't interfere.
>>>       */
>>>      r = virtio_device_grab_ioeventfd(vdev);
>>>      if (r < 0) {
>>>          error_report("binding does not support host notifiers");
>>>          return r;
>>>      }
>>>
>>>      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);
>>>              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);
>>>              break;
>>>          }
>>>      }
>>>
>>>      memory_region_transaction_commit();
>>>
>>>      return r;
>>> }
>>>
>>> What do you think?
>>>
>> Maybe we can use vhost_dev_disable_notifiers to further simplify the 
>> error path ?
> 
> Good idea, but having the BusState resolved on each call seems a waste.
> Eventually factor it out and pass as argument ...
> 
>> And we must commit before invoking virtio_bus_cleanup_host_notifier.
> 
> ... but with that info on top, finally your original patch is simpler.

Yes, I'll try in next version, thanks.

> .

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

[Michael S. Tsirkin]

On Wed, Dec 07, 2022 at 08:22:18AM +0800, Longpeng (Mike, Cloud Infrastructure Service Product Dept.) wrote:
> > > And we must commit before invoking virtio_bus_cleanup_host_notifier.
> > 
> > ... but with that info on top, finally your original patch is simpler.
> 
> Yes, I'll try in next version, thanks.

OK so I'm waiting for v3.