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