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

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

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>