Hi Maxime
From: Maxime Coquelin <maxime.coque...@redhat.com>
> Sent: Monday, June 22, 2020 11:56 AM
> To: Matan Azrad <ma...@mellanox.com>; Xiao Wang
> <xiao.w.w...@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [PATCH v1 3/4] vhost: improve device ready definition
>
>
>
> On 6/22/20 10:41 AM, Matan Azrad wrote:
> >> The issue is if you only check ready state only before and after the
> >> message affecting the ring is handled, it can be ready at both
> >> stages, while the rings have changed and state change callback should
> have been called.
> > But in this version I checked twice, before message handler and after
> message handler, so it should catch any update.
>
> No, this is not enough, we have to check also during some handlers, so that
> the ready state is invalidated because sometimes it will be ready before and
> after the message handler but with different values.
>
> That's what I did in my example patch:
> @@ -1847,15 +1892,16 @@ vhost_user_set_vring_kick(struct virtio_net
> **pdev, struct VhostUserMsg *msg,
>
> ...
>
> if (vq->kickfd >= 0)
> close(vq->kickfd);
> +
> + vq->kickfd = VIRTIO_UNINITIALIZED_EVENTFD;
> +
> + vhost_user_update_vring_state(dev, file.index);
> +
> vq->kickfd = file.fd;
>
>
> Without that, the ready check will return ready before and after the kickfd
> changed and the driver won't be notified.
The driver will be notified in the next VHOST_USER_SET_VRING_ENABLE message
according to v1.
One of our assumption we agreed on in the design mail is that it doesn't make
sense that QEMU will change queue configuration without enabling the queue
again.
Because of that we decided to force calling state callback again when QEMU send
VHOST_USER_SET_VRING_ENABLE(1) message even if the queue is already ready.
So when driver/app see state enable->enable, it should take into account that
the queue configuration was probably changed.
I think that this assumption is correct according to the QEMU code.
That's why I prefer to collect all the ready checks callbacks (queue state and
device new\conf) to one function that will be called after the message handler:
Pseudo:
vhost_user_update_ready_statuses() {
switch (msg):
case enable:
if(enable is 1)
force queue state =1.
case callfd
case kickfd
.....
Check queue and device ready + call callbacks if needed..
Default
Return;
}
