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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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.