On Wed, Feb 17, 2021 at 6:24 PM Stefan Hajnoczi <stefa...@redhat.com> wrote:
>
> On Tue, Feb 09, 2021 at 04:37:57PM +0100, Eugenio Pérez wrote:
> > @@ -40,6 +42,26 @@ static void vhost_handle_guest_kick(EventNotifier *n)
> > }
> > }
> >
> > +/* Forward vhost notifications */
> > +static void vhost_handle_call(EventNotifier *n)
>
> The name vhost_shadow_vq_handle_call() expresses the purpose of the
> function more clearly.
>
I will rename it.
> > @@ -75,8 +102,19 @@ bool vhost_shadow_vq_start_rcu(struct vhost_dev *dev,
> > /* Check for pending notifications from the guest */
> > vhost_handle_guest_kick(&svq->host_notifier);
> >
> > + r = dev->vhost_ops->vhost_set_vring_call(dev, &call_file);
> > + if (r != 0) {
> > + error_report("Couldn't set call fd: %s", strerror(errno));
> > + goto err_set_vring_call;
> > + }
>
> This ignores notifier_is_masked and always unmasks.
Right, I will check for it too.
>
> > @@ -1608,6 +1607,10 @@ void vhost_virtqueue_mask(struct vhost_dev *hdev,
> > VirtIODevice *vdev, int n,
> > if (mask) {
> > assert(vdev->use_guest_notifier_mask);
> > file.fd = event_notifier_get_fd(&hdev->vqs[index].masked_notifier);
> > + } else if (hdev->sw_lm_enabled) {
> > + VhostShadowVirtqueue *svq = hdev->shadow_vqs[n];
> > + EventNotifier *e = vhost_shadow_vq_get_call_notifier(svq);
> > + file.fd = event_notifier_get_fd(e);
> > } else {
> > file.fd =
> > event_notifier_get_fd(virtio_queue_get_guest_notifier(vvq));
> > }
>
> Maybe you can extend this function so it can be called unconditionally
> from both vhost_shadow_vq_start_rcu() and vhost_shadow_vq_stop_rcu(). It
> would be a single place that invokes vhost_set_vring_call().
Your proposal is better, but I'm not sure if something depends on
calling mask(..., false) repeatedly. For example, if guest_notifier
changes.
However, from SVQ point of view we are only interested in avoiding to
set masked notifiers over a vhost already masked: unmasked state will
always incur on a file descriptor change. So I think it should be fine
if we allow unmasking, In other words, allow calling
vhost_set_vring_call(guest_notifier()) repeatedly, so old behavior is
preserved, and only omit the mask(..., true) if the notifier is
already masked, so we can call it unconditionally on
vhost_shadow_vq_start/stop.
Thanks!
