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.