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

[longpeng2--- via <qemu-devel@nongnu.org>]

在 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.

> .