Re: [dpdk-dev] [PATCH v1 3/4] vhost: improve device ready definition
From: Matan Azrad <hidden>
Date: 2020-06-22 10:06:24
Hi Maxime From: Maxime Coquelin <redacted>
quoted hunk ↗ jump to hunk
Sent: Monday, June 22, 2020 11:56 AM To: Matan Azrad <redacted>; Xiao Wang [off-list ref] 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:quoted
quoted
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 shouldhave been called.quoted
But in this version I checked twice, before message handler and aftermessage 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;
}