Re: [dpdk-dev] [PATCH v1 3/4] vhost: improve device ready definition

[Matan Azrad <matan@mellanox.com>]

Hi Maxime

From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Monday, June 22, 2020 11:56 AM
> To: Matan Azrad <matan@mellanox.com>; Xiao Wang
> <xiao.w.wang@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;
}