| Message ID | 20241216095429.210792-1-wguay@fb.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | cover-letter: Allow MMIO regions to be exported through dmabuf | expand |
Hi Christian, Thanks again for your prompt response/review. > On 17 Dec 2024, at 10:53, Christian König <christian.koenig@amd.com> wrote: > > > > Am 16.12.24 um 17:54 schrieb Keith Busch: >> On Mon, Dec 16, 2024 at 11:21:39AM +0100, Christian König wrote: >>> Am 16.12.24 um 10:54 schrieb Wei Lin Guay: >>>> From: Wei Lin Guay <wguay@meta.com> >>>> However, as a general mechanism, it can support many other scenarios with >>>> VFIO. I imagine this dmabuf approach to be usable by iommufd as well for >>>> generic and safe P2P mappings. >>>> >>>> This series goes after the "Break up ioctl dispatch functions to one >>>> function per ioctl" series. >>> Yeah that sounds like it should work. >>> >>> But where is the rest of the series, I only see the cover letter? >> Should be here: >> >> https://lore.kernel.org/linux-rdma/20241216095429.210792-2-wguay@fb.com/T/#u > > Please send that out once more with me on explicit CC. I will resend the patch series. I was experiencing issues with my email client, which inadvertently split the series into two separate emails. > > Apart from that I have to reject the adding of dma_buf_try_get(), that is clearly not something we should do. Understood. It appears that Vivek has confirmed that his v2 has resolved the issue. I will follow up with him to determine if he plans to resume his patch, and if so, I will apply my last patch on top of his updated patch series Thanks, Wei Lin > > Thanks, > Christian.
Am 17.12.24 um 12:06 schrieb Wei Lin Guay: > [SNIP] >> Please send that out once more with me on explicit CC. > I will resend the patch series. I was experiencing issues with my email client, which inadvertently split the series into two separate emails. Alternatively I can also copy the code from the list archive and explain why that doesn't work: +void vfio_pci_dma_buf_move(struct vfio_pci_core_device *vdev, bool revoked) +{ + struct vfio_pci_dma_buf *priv; + struct vfio_pci_dma_buf *tmp; + + lockdep_assert_held_write(&vdev->memory_lock); This makes sure that the caller is holding vdev->memory_lock. + + list_for_each_entry_safe(priv, tmp, &vdev->dmabufs, dmabufs_elm) { + if (!dma_buf_try_get(priv->dmabuf)) This here only works because vfio_pci_dma_buf_release() also grabs vdev->memory_lock and so we are protected against concurrent releases. + continue; + if (priv->revoked != revoked) { + dma_resv_lock(priv->dmabuf->resv, NULL); + priv->revoked = revoked; + dma_buf_move_notify(priv->dmabuf); + dma_resv_unlock(priv->dmabuf->resv); + } + dma_buf_put(priv->dmabuf); The problem is now that this here might drop the last reference which in turn calls vfio_pci_dma_buf_release() which also tries to grab vdev->memory_lock and so results in a deadlock. + } +} This pattern was suggested multiple times and I had to rejected it every time because the whole idea is just fundamentally broken. It's really astonishing how people always come up with the same broken pattern. Regards, Christian. > >> Apart from that I have to reject the adding of dma_buf_try_get(), that is clearly not something we should do. > > Understood. It appears that Vivek has confirmed that his v2 has resolved the issue. I will follow up with him to determine if he plans to resume his patch, and if so, I will apply my last patch on top of his updated patch series > > Thanks, > Wei Lin >> Thanks, >> Christian. >
Hi Christian, > Subject: Re: [PATCH 0/4] cover-letter: Allow MMIO regions to be exported > through dmabuf > > >> I will resend the patch series. I was experiencing issues with my email >> client, which inadvertently split the series into two separate emails. > > > Alternatively I can also copy the code from the list archive and explain why > that doesn't work: > > +void vfio_pci_dma_buf_move(struct vfio_pci_core_device *vdev, bool > +revoked) { > + struct vfio_pci_dma_buf *priv; > + struct vfio_pci_dma_buf *tmp; > + > + lockdep_assert_held_write(&vdev->memory_lock); > > This makes sure that the caller is holding vdev->memory_lock. > > + > + list_for_each_entry_safe(priv, tmp, &vdev->dmabufs, dmabufs_elm) { > + if (!dma_buf_try_get(priv->dmabuf)) > > This here only works because vfio_pci_dma_buf_release() also grabs vdev- > >memory_lock and so we are protected against concurrent releases. > > + continue; > + if (priv->revoked != revoked) { > + dma_resv_lock(priv->dmabuf->resv, NULL); > + priv->revoked = revoked; > + dma_buf_move_notify(priv->dmabuf); > + dma_resv_unlock(priv->dmabuf->resv); > + } > + dma_buf_put(priv->dmabuf); > > The problem is now that this here might drop the last reference which in turn > calls vfio_pci_dma_buf_release() which also tries to grab vdev- > >memory_lock and so results in a deadlock. AFAICS, vfio_pci_dma_buf_release() would not be called synchronously after the last reference is dropped by dma_buf_put(). This is because fput(), which is called by dma_buf_put() triggers f_op->release() asynchronously; therefore, a deadlock is unlikely to occur in this scenario, unless I am overlooking something. Thanks, Vivek > > + } > +} > > This pattern was suggested multiple times and I had to rejected it every time > because the whole idea is just fundamentally broken. > > It's really astonishing how people always come up with the same broken > pattern. > > Regards, > Christian. > > > > > > > > Apart from that I have to reject the adding of > dma_buf_try_get(), that is clearly not something we should do. > > > > Understood. It appears that Vivek has confirmed that his v2 has > resolved the issue. I will follow up with him to determine if he plans to > resume his patch, and if so, I will apply my last patch on top of his updated > patch series > > Thanks, > Wei Lin > > > Thanks, > Christian. > > > >
Am 18.12.24 um 07:16 schrieb Kasireddy, Vivek: > Hi Christian, > >> Subject: Re: [PATCH 0/4] cover-letter: Allow MMIO regions to be exported >> through dmabuf >> >> >>> I will resend the patch series. I was experiencing issues with my email >>> client, which inadvertently split the series into two separate emails. >> >> Alternatively I can also copy the code from the list archive and explain why >> that doesn't work: >> >> +void vfio_pci_dma_buf_move(struct vfio_pci_core_device *vdev, bool >> +revoked) { >> + struct vfio_pci_dma_buf *priv; >> + struct vfio_pci_dma_buf *tmp; >> + >> + lockdep_assert_held_write(&vdev->memory_lock); >> >> This makes sure that the caller is holding vdev->memory_lock. >> >> + >> + list_for_each_entry_safe(priv, tmp, &vdev->dmabufs, dmabufs_elm) { >> + if (!dma_buf_try_get(priv->dmabuf)) >> >> This here only works because vfio_pci_dma_buf_release() also grabs vdev- >>> memory_lock and so we are protected against concurrent releases. >> + continue; >> + if (priv->revoked != revoked) { >> + dma_resv_lock(priv->dmabuf->resv, NULL); >> + priv->revoked = revoked; >> + dma_buf_move_notify(priv->dmabuf); >> + dma_resv_unlock(priv->dmabuf->resv); >> + } >> + dma_buf_put(priv->dmabuf); >> >> The problem is now that this here might drop the last reference which in turn >> calls vfio_pci_dma_buf_release() which also tries to grab vdev- >>> memory_lock and so results in a deadlock. > AFAICS, vfio_pci_dma_buf_release() would not be called synchronously after the > last reference is dropped by dma_buf_put(). This is because fput(), which is called > by dma_buf_put() triggers f_op->release() asynchronously; therefore, a deadlock > is unlikely to occur in this scenario, unless I am overlooking something. My recollection is that the f_op->release handler is only called asynchronously if fput() was issued in interrupt context. But could be that this information is outdated. Regards, Christian. > > Thanks, > Vivek > >> + } >> +} >> >> This pattern was suggested multiple times and I had to rejected it every time >> because the whole idea is just fundamentally broken. >> >> It's really astonishing how people always come up with the same broken >> pattern. >> >> Regards, >> Christian. >> >> >> >> >> >> >> >> Apart from that I have to reject the adding of >> dma_buf_try_get(), that is clearly not something we should do. >> >> >> >> Understood. It appears that Vivek has confirmed that his v2 has >> resolved the issue. I will follow up with him to determine if he plans to >> resume his patch, and if so, I will apply my last patch on top of his updated >> patch series >> >> Thanks, >> Wei Lin >> >> >> Thanks, >> Christian. >> >> >> >>
On Tue, Dec 17, 2024 at 10:53:32AM +0100, Christian König wrote: > Am 16.12.24 um 17:54 schrieb Keith Busch: > > On Mon, Dec 16, 2024 at 11:21:39AM +0100, Christian König wrote: > > > Am 16.12.24 um 10:54 schrieb Wei Lin Guay: > > > > From: Wei Lin Guay <wguay@meta.com> > > > > However, as a general mechanism, it can support many other scenarios with > > > > VFIO. I imagine this dmabuf approach to be usable by iommufd as well for > > > > generic and safe P2P mappings. > > > > > > > > This series goes after the "Break up ioctl dispatch functions to one > > > > function per ioctl" series. > > > Yeah that sounds like it should work. > > > > > > But where is the rest of the series, I only see the cover letter? > > Should be here: > > > > https://lore.kernel.org/linux-rdma/20241216095429.210792-2-wguay@fb.com/T/#u > > Please send that out once more with me on explicit CC. > > Apart from that I have to reject the adding of dma_buf_try_get(), that is > clearly not something we should do. Yeah if we do try_get it would need to be at least on a specific dma_buf type (so checking for dma_buf->ops), since in general this does not work (unless we add the general code in dma_buf.c somehow to make it work). Aside from any other design concerns. -Sima