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

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 6/22/20 12:06 PM, Matan Azrad wrote:
> 
> 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.

Yes, this was our initial assumption.
But now looking into the details of the implementation, I find it is
even cleaner & clearer not to do this assumption.

> 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;
> }

I find it more natural to "invalidate" ready state where it is handled
(after vring_invalidate(), before setting new FD for call & kick, ...)