Hi Akihiko, > > > >>>>> > >>>>>> Subject: Re: [PATCH v1 2/7] virtio-gpu: Don't rely on res->blob to > >> identify > >>>>>> blob resources > >>>>>> > >>>>>>>>>> > >>>>>>>>>>> Subject: Re: [PATCH v1 2/7] virtio-gpu: Don't rely on res->blob > to > >>>>>>>>>>> identify blob > >>>>>>>>>>> resources > >>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>>> Subject: Re: [PATCH v1 2/7] virtio-gpu: Don't rely on res- > >blob > >> to > >>>>>>>>>>>>> identify > >>>>>>>>>>> blob > >>>>>>>>>>>>> resources > >>>>>>>>>>>>> > >>>>>>>>>>>>> On 2025/10/04 8:35, Vivek Kasireddy wrote: > >>>>>>>>>>>>>> The res->blob pointer may not be valid (non-NULL) for > some > >>>> blobs > >>>>>>>>>>>>>> where the backing storage is not memfd based. Therefore, > >> we > >>>>>> cannot > >>>>>>>>>>>>>> use it to determine if a resource is a blob or not. Instead, > we > >>>>>>>>>>>>>> could use res->blob_size to make this determination as it > is > >>>>>>>>>>>>>> non-zero for blob resources regardless of where their > backing > >>>>>>>>>>>>>> storage is located. > >>>>>>>>>>>>> > >>>>>>>>>>>>> I think this patch is no longer necessary since now you add > >> code to > >>>>>>>>>>>>> mmap() VFIO storage with "[PATCH v1 7/7] virtio-gpu- > >> udmabuf: > >>>>>> Create > >>>>>>>>>>>>> dmabuf for blobs associated with VFIO devices". > >>>>>>>>>>>> Right, but given that mmap() can still fail for various reasons > >> and > >>>>>>>>>>>> this > >>>>>>>>>>>> use-case can work as long as dmabuf creation succeeds, I > think > >> it > >>>>>> makes > >>>>>>>>>>>> sense to not rely on res->blob to determine if a resource is > blob > >> or > >>>>>>>>>>>> not. > >>>>>>>>>>> > >>>>>>>>>>> I think the code will be simpler by making resource creation > fail > >> when > >>>>>>>>>>> mmap() fails, and I am concerned that the guest may > >> mulfunction > >>>>>> with > >>>>>>>>>>> such an incomplete resource. > >>>>>>>>>> AFAICT, mmap() is a slow, optional path except for the cursor > >> (which > >>>>>>>>>> needs > >>>>>>>>>> further improvement). So, failing resource creation when > mmap() > >> fails > >>>>>>>>>> does not seem like a good idea to me given the fact that > >> supporting > >>>>>>>>>> mmap() > >>>>>>>>>> is considered optional for dmabuf providers. And, even with > vfio, > >>>>>> mmap() > >>>>>>>>>> can be blocked for various reasons by the kernel driver IIUC. > >>>>>>>> > >>>>>>>> Reviewing the code again, I don't think mmap() can fail with the > >> current > >>>>>>>> version of this series. > >>>>>>>> > >>>>>>>> udmabuf obviously always supports mmap(). > >>>>>>>> > >>>>>>>> For VFIO, checking memory_region_is_ram_device() ensures that > >> VFIO > >>>>>>>> supports mmap(); memory_region_init_ram_device_ptr() is called > >> from > >>>>>>>> vfio_region_mmap(), which is only called when VFIO supports > >> mmap(). > >>>>>>> My point is not whether a dmabuf provider provides support for > >> mmap() > >>>>>>> or not but about the fact that mmap() can fail (for various reasons > >>>>>> because > >>>>>>> it is not a guarantee) making res->blob NULL. But we are incorrectly > >> using > >>>>>>> res->blob pointer to determine whether a resource is a blob (and > >> usable) > >>>>>>> or not which doesn't make sense because even if res->blob is NULL, > >> the > >>>>>>> resource is still valid and usable via the dmabuf fd, which is the > >> preferred, > >>>>>>> accelerated path. > >>>>>> > >>>>>> Failing to mmap something that is already mmap-ed to another > >> address is > >>>>>> very unrealistic and I can't really think of a possibility of such a > >>>>>> failure aside bugs. > >>>>> The fact that it is already mmap'd to another address would only be > >> true for > >>>>> VFIO devices but as I mentioned previously, we cannot make such > >>>> assumptions > >>>>> with other (future) dmabuf providers. > >>>> > >>>> It is true for udmabuf, though the memfds that back udmabuf are > >> directly > >>>> mapped instead; I don't think the indirection of udmabuf makes any > >>>> difference. > >>>> > >>>> If it's only for future DMA-BUF exporter, it is better to make the > >>>> change when the exporter is actually added, or we are adding code > that > >>>> cannot be tested right now and may or may not work when such an > >> exporter > >>>> is added. > >>>> > >>>>> > >>>>>> > >>>>>> If this condition (a valid resource with a NULL res->blob) could only > >>>>>> happen due to a bug, then, in my opinion, marking such a resource > as > >>>>>> invalid is actually a more defensive and desirable approach. If a core > >>>>>> operation like mmap fails unexpectedly on a resource that should > >> support > >>>>> But mmap is not considered as a core operation for dmabuf. It is > >> considered > >>>>> optional by dmabuf providers. For example, although very unlikely, it > >> might > >>>>> be possible that support for mmap() can be removed from udmabuf > >> driver > >>>>> driver for some reason. And, when this happens, the only adverse > effect > >>>> would > >>>>> be that gl=off would not work, which is not great but definitely not > >>>> catastrophic. > >>>> > >>>> We should be able to safely assume it never happens due to the "no > >>>> regressions" rule of Linux. If a userspace program breaks due to a UAPI > >>> I help maintain the udmabuf driver in the kernel and AFAICT, that rule > >> does > >>> not apply here because udmabuf driver providing support for mmap() > >> cannot > >>> be considered UAPI because it is not providing any direct (user visible) > >> interface > >>> to invoke mmap() as described here: > >>> https://docs.kernel.org/admin-guide/reporting-regressions.html#what- > is- > >> a-regression-and-what-is-the-no-regressions-rule > >> > >> I suppose that the result of open("/dev/udmabuf", O_RDWR) and the > >> UDMABUF_CREATE_LIST ioctl is always compatible with mmap(). Not sure > >> what you mean by "direct" interface, but they are all plain userspace > >> interfaces. > >> > >> I suggest asking people maintaining the udmabuf and the DMA-BUF > >> infrastructure if you know them. > > I am one of the two maintainers of udmabuf driver and I am also involved > > in the discussions with dmabuf maintainers regarding upstreaming of > > vfio-pci dmabuf feature in the kernel. And, based on these interactions, > > it is clear that mmap() is completely optional for dmabuf providers and > > userspace should never make any assumptions about whether mmap() > > (for dmabuf) works or not. Here is one reference: > > https://lore.kernel.org/dri-devel/[email protected]/ > > That adds nothing more than you have already told in the past discussion > in this thread. > > I started this thread because I found that the current implementation > only exercises udmabuf and VFIO devices that is known to implement > mmap(). On the other hand, the cited email discusses VFIO devices that > are not known to implement mmap() or future DMA-BUF exporters in > general. It says: > > Just wanted to confirm that it's entirely legit to not implement > > dma-buf mmap. Same for the in-kernel vmap functions. Especially for > > really funny buffers like these it's just not a good idea, and the > > dma-buf interfaces are intentionally "everything is optional". > > This doesn't apply to udmabuf and VFIO devices we are concerned with. > udmabuf is obviously not "funny buffers", and can be vmapped. Besides, > the udmabuf already implements mmap() and *removing* it can break > userspace unlike a DMA-BUF exporter that never implemented mmap(), > which > was discussed in the cited email. > > Speaking of VFIO devices, the code already checks it's mmapped by > calling memory_region_is_ram_device() and yet this patch tries to allow > blobs that cannot be mmapped, which is inconsistent. > > So, the discussion should be summarized as the following two questions: > 1. Can the kernel remove an existing mmap() implementation that is known > to be depended by the userspace (udmabuf)? > 2. Can the kernel let an mmap() implementation fail though it once > worked (VFIO)? IMO, the answer to both these questions is yes because mmap for dmabufs was optional to begin with. And, just because it works in most cases doesn't mean it would work with 100% certainty in all cases at all times. In other words, we should reasonably assume that mmap() can fail sometimes and when it does fail, it should not take down the fast, h/w accelerated path, which is what happens currently.
AFAICS, you are putting undue emphasis on mmap() capability for dmabufs. It is simply not important for GPU based use-cases because it is a very slow (and sometimes practically unusable) path (60 FPS vs 1 FPS for VFIO based buffers in my testing), which is why it was made optional for dmabuf providers. And, what this patch is essentially doing is just making sure that the fast path continues to work even when the slow path is not available (or fails) for some reason. But IIUC, your argument is that if the slow path cannot work, then the fast path should also remain unavailable, which is what happens without this patch. So, the status quo is not that great but I don't think I have any further arguments to add to this discussion at this time. > > The cited email doesn't answer either of them. I am not sure who can provide definitive answers to these questions but I'd like to move on by dropping this patch. I guess we could revisit this discussion when new use-cases (or potential failures) arise or more information becomes available. Thanks, Vivek > > Regards, > Akihiko Odaki
