On Tue, May 30, 2017 at 11:06:54PM +0200, Maxime Coquelin wrote: > Hi Michael, > > On 05/30/2017 08:12 PM, Michael S. Tsirkin wrote: > > On Fri, May 26, 2017 at 04:28:55PM +0200, Maxime Coquelin wrote: > > > Vhost-kernel backend need > > > > needs > > > > > to receive IOTLB entry for used ring > > > information early, which is done by triggering a miss event on > > > its address. > > > > > > This patch extends this behaviour to all rings information, to be > > > compatible with vhost-user backend design. > > > > Why does vhost-user need it though? > > For vhost-user, this simplifies the backend design because generally, > the backend sends IOTLB miss requests from processing threads through > the slave channel, and receives resulting IOTLB updates in vhost-user > protocol thread. > > The only exception is for these rings info, where IOTLB miss requests > are sent from vhost-user protocol thread (in the SET_VRING_ENABLE > request handler), so the resulting IOTLB update is only handled by > the backend when the rings are enabled, which is too late. > > It could be possible to overcome this issue, but I think it would > make the design more complex or less efficient. I see several options: > > 1. Change the IOTLB miss request so that the master sends the IOTLB > update as reply, instead of the reply-ack. It would mean that IOTLB > updates/invalidations would be sent either via the master channel or > the slave channel. On QEMU side, it means diverging from kernel backend > implementation. On backend side, it means having possibly multiple > threads writing to the IOTLB cache. > > 2. In vhost-user protocol thread, when handling SET_VRING_ENABLE, send > IOTLB miss request without setting the reply-ack flag, and poll the > vhost-user socket to read the resulting IOTLB update. The problem is > that other requests could be pending in the socket's buffer, and so it > would end-up nesting multiple requests handling. > > 3. Don't interpret rings info in the vhost-user protocol thread, but > only in the processing threads. The advantage is that it would address > the remark you made on patch 6 that invalidates are not affecting ring > info. The downside being the overhead induced by checking whether the > ring info are valid every time it is being used. I haven't prototyped > this solution, but I expected the performance regression to be a > blocker. > > 4. In SET_VRING_ENABLE, don't enable the ring if needed entries are not in > IOTLB cache. Just send the IOTLB misses without reply-ack flag and postpone > enable when handling IOTLB updates. It will be a little more complex > solution than current one, but it may be the less impacting compared to the > other 3 options. > > > Thinking again, maybe trying solution would be worth the effort, and could > be extended also to disable the rings when receiving invalidates > that affect rings info. > > What do you think?
I'm fine with 3 or 4 generally. But pls note that if the ring crosses a page boundary (e.g. ring size > page size) and when not using hugetlbfs, there is no guarantee a single DMA address covers the whole ring. > > > > > > > > Signed-off-by: Maxime Coquelin <maxime.coque...@redhat.com> > > > --- > > > v2: > > > - Revert back to existing behaviour, i.e. only send IOTLB updates > > > at ring enablement time, not at ring address setting time (mst). > > > - Extend IOTLB misses to all ring addresses, not only used ring. > > > > > > hw/virtio/vhost.c | 8 ++++++-- > > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > > index 6eddb09..7867034 100644 > > > --- a/hw/virtio/vhost.c > > > +++ b/hw/virtio/vhost.c > > > @@ -1552,11 +1552,15 @@ int vhost_dev_start(struct vhost_dev *hdev, > > > VirtIODevice *vdev) > > > if (vhost_dev_has_iommu(hdev)) { > > > hdev->vhost_ops->vhost_set_iotlb_callback(hdev, true); > > > - /* Update used ring information for IOTLB to work correctly, > > > - * vhost-kernel code requires for this.*/ > > > + /* > > > + * Update rings information for IOTLB to work correctly, > > > + * vhost-kernel and vhost-user codes require for this. > > > > Better just say "Update ring info for vhost iotlb." > > > > The rest isn't really informative. > > Ok. > > > > > > > > > > + */ > > > for (i = 0; i < hdev->nvqs; ++i) { > > > struct vhost_virtqueue *vq = hdev->vqs + i; > > > vhost_device_iotlb_miss(hdev, vq->used_phys, true); > > > + vhost_device_iotlb_miss(hdev, vq->desc_phys, true); > > > + vhost_device_iotlb_miss(hdev, vq->avail_phys, true); > > > > So I don't remember why does vhost in kernel want miss on used > > at start time. > > > > Jason, could you comment on this please? > > > > > > > > > } > > > } > > > return 0; > > > -- > > > 2.9.4 > > Thanks, > Maxime