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

[Longpeng (Mike, Cloud Infrastructure Service Product Dept.)]

在 2022/12/28 21:12, Philippe Mathieu-Daudé 写道:
> 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:

There's a transaction in vhost_dev_disable_notifiers() too, We must 
commit the outter transaction before invoking it, you can see the 
comment in it.

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