Re: Re: [RFC v3 08/11] vduse: Introduce VDUSE - vDPA Device in Userspace

On Thu, Jan 28, 2021 at 2:14 PM Jason Wang [off-list ref] wrote:

On 2021/1/28 下午2:03, Yongji Xie wrote:

quoted

quoted

quoted

quoted

quoted

+
+static const struct file_operations vduse_domain_fops = {
+     .mmap = vduse_domain_mmap,
+     .release = vduse_domain_release,
+};

It's better to explain the reason for introducing a dedicated file for
mmap() here.

To make the implementation of iova_domain independent with vduse_dev.

My understanding is that, the only usage for this is to:

1) support different type of iova mappings
2) or switch between iova domain mappings

But I can't think of a need for this.

For example, share one iova_domain between several vduse devices.

Interesting.

quoted

And it will be helpful if we want to split this patch into iova domain
part and vduse device part. Because the page fault handler should be
paired with dma_map/dma_unmap.

Ok.

[...]

quoted

quoted

quoted

quoted

This looks not safe, let's use idr here.

Could you give more details? Looks like idr should not used in this
case which can not tolerate failure. And using a list to store the msg
is better than using idr when the msg needs to be re-inserted in some
cases.

My understanding is the "unique" (probably need a better name) is a
token that is used to uniquely identify a message. The reply from
userspace is required to write with exact the same token(unique). IDR
seems better but consider we can hardly hit 64bit overflow, atomic might
be OK as well.

Btw, under what case do we need to do "re-inserted"?

When userspace daemon receive the message but doesn't reply it before crash.

Do we have code to do this?

Yes, in patch 9.

quoted

quoted

quoted

quoted

So we had multiple types of requests/responses, is this better to
introduce a queue based admin interface other than ioctl?

Sorry, I didn't get your point. What do you mean by queue-based admin
interface? Virtqueue-based?

Yes, a queue(virtqueue). The commands could be passed through the queue.
(Just an idea, not sure it's worth)

I considered it before. But I found it still needs some extra works
(setup eventfd, set vring base and so on) to setup the admin virtqueue
before using it for communication. So I turn to use this simple way.

Yes. We might consider it in the future.

Agree.

quoted

quoted

quoted

quoted

Any reason for such IOTLB invalidation here?

As I mentioned before, this is used to notify userspace to update the
IOTLB. Mainly for virtio-vdpa case.

So the question is, usually, there could be several times of status
setting during driver initialization. Do we really need to update IOTLB
every time?

I think we can check whether there are some changes after the last
IOTLB updating here.

So the question still, except reset (write 0), any other status that can
affect IOTLB?

OK, I get your point. The status would not affect IOTLB. The reason
why we do IOTLB updating here is we can't do it in dma_map_ops which
might work in an atomic context. So I want to notify userspace to
update IOTLB before I/O is processed. Of course, it's not a must
because userspace can manually query it.

Thanks,
Yongji