On Tue, Jun 4, 2019 at 11:11 AM Robin Murphy <robin.murphy@xxxxxxx> wrote: > > On 06/05/2019 19:52, Tom Murphy wrote: > > Add a gfp_t parameter to the iommu_ops::map function. > > Remove the needless locking in the AMD iommu driver. > > > > The iommu_ops::map function (or the iommu_map function which calls it) > > was always supposed to be sleepable (according to Joerg's comment in > > this thread: https://lore.kernel.org/patchwork/patch/977520/ ) and so > > should probably have had a "might_sleep()" since it was written. However > > currently the dma-iommu api can call iommu_map in an atomic context, > > which it shouldn't do. This doesn't cause any problems because any iommu > > driver which uses the dma-iommu api uses gfp_atomic in it's > > iommu_ops::map function. But doing this wastes the memory allocators > > atomic pools. > > Hmm, in some cases iommu_ops::unmap may need to allocate as well, > primarily if it needs to split a hugepage mapping. Should we pass flags > through there as well, or are we prepared to assume that that case will > happen rarely enough that it's fair to just assume GFP_ATOMIC? It won't > happen for DMA ops, but it's conceivable that other users such as GPU > drivers might make partial unmaps, and I doubt we could totally rule out > the wackiest ones doing so from non-sleeping contexts. > jfyi, today we (well, drm/msm) only unmap entire buffers, so assuming there isn't any coelescense of adjacent buffers that happen to form a hugepage on ::map(), we are probably ok on ::unmap().. we do always only call ::map or ::unmap under sleepable conditions. btw, would it be simpler to just make gfp_t a domain a attribute? That seems like it would be less churn, but maybe I'm overlooking something. BR, -R
