| Message ID | 20190218210715.1066-1-krisman@collabora.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Improve handling of GFP flags in the CMA allocator | expand |
On 2/18/19 10:07 PM, Gabriel Krisman Bertazi wrote: > Hi, > > The main goal of this patchset is to solve a deadlock in the CMA > allocator, which happens because cma_alloc tries to sleep waiting for an > IO in the GFP_NOIO path. This issue, which was reported by Gael Portay > was discussed here: > > https://groups.google.com/a/lists.one-eyed-alien.net/forum/#!topic/usb-storage/BXpAsg-G1us > > My proposed requires reverting the patches that removed the gfp flags > information from cma_alloc() (patches 1 to 3). According to the author, > that parameter was removed because it misleads developers about what > cma_alloc actually supports. In his specific case he had problems with > GFP_ZERO. With that in mind I gave a try at implementing GFP_ZERO in a > quite trivial way in patch 4. Finally, patches 5 and 6 attempt to fix > the issue by avoiding the unecessary serialization done around > alloc_contig_range. I haven't checked in detail yet, but for GFP_NOIO, we have memalloc_noio_save() / memalloc_noio_restore() which adds implicit GFP_NOIO for the whole call stack. So that could be perhaps used to avoid adding the gfp flags back to function signatures. Since you are adding a new test for __GFP_IO in cma_alloc() in patch 6, you would use e.g. current_gfp_context(GFP_KERNEL) first to add __GFP_NOIO based on the implicit context. As for the arm64 caller, maybe it already is in noio context (ideal world), or would add it based on test before calling dma_alloc_from_contiguous(). There's also some documentation in Documentation/core-api/gfp_mask-from-fs-io.rst CCing Michal for opinion since he authored this > This is my first adventure in the mm subsystem, so I hope I didn't screw > up something very obvious. I tested this on the workload that was > deadlocking (arm board, with CMA intensive operations from the GPU and > USB), as well as some scripting on top of debugfs. Is there any > regression test I should be running, which specially applies to the CMA > code? > > > Gabriel Krisman Bertazi (6): > Revert "kernel/dma: remove unsupported gfp_mask parameter from > dma_alloc_from_contiguous()" > Revert "mm/cma: remove unsupported gfp_mask parameter from > cma_alloc()" > cma: Warn about callers requesting unsupported flags > cma: Add support for GFP_ZERO > page_isolation: Propagate temporary pageblock isolation error > cma: Isolate pageblocks speculatively during allocation > > arch/arm/mm/dma-mapping.c | 5 +-- > arch/arm64/mm/dma-mapping.c | 2 +- > arch/powerpc/kvm/book3s_hv_builtin.c | 2 +- > arch/xtensa/kernel/pci-dma.c | 2 +- > drivers/iommu/amd_iommu.c | 2 +- > drivers/iommu/intel-iommu.c | 3 +- > drivers/s390/char/vmcp.c | 2 +- > drivers/staging/android/ion/ion_cma_heap.c | 2 +- > include/linux/cma.h | 2 +- > include/linux/dma-contiguous.h | 4 +- > kernel/dma/contiguous.c | 6 +-- > kernel/dma/direct.c | 3 +- > kernel/dma/remap.c | 2 +- > mm/cma.c | 51 ++++++++++++++++++---- > mm/cma_debug.c | 2 +- > mm/page_isolation.c | 20 ++++++--- > 16 files changed, 74 insertions(+), 36 deletions(-) >
I don't think this is a good idea. The whole concept of just passing random GFP_ flags to dma_alloc_attrs / dma_alloc_coherent can't work, given that on many architectures we need to set up new page tables to remap the allocated memory, and we can't use arbitrary gfp flags for pte allocations. So instead of trying to pass them further down again we need to instead work to fix all callers of dma_alloc_attrs / dma_alloc_coherent that don't just pass GFP_KERNEL.
On 2/26/19 6:29 AM, Christoph Hellwig wrote: > I don't think this is a good idea. The whole concept of just passing > random GFP_ flags to dma_alloc_attrs / dma_alloc_coherent can't work, > given that on many architectures we need to set up new page tables > to remap the allocated memory, and we can't use arbitrary gfp flags > for pte allocations. > > So instead of trying to pass them further down again we need to instead > work to fix all callers of dma_alloc_attrs / dma_alloc_coherent > that don't just pass GFP_KERNEL. > What's the expected approach to fix callers? It's not clear how you would fix the callers for the case that prompted this series (context correctly used GFP_NOIO but it was not passed to dma_alloc_coherent) Thanks, Laura
On Wed 27-02-19 16:12:30, Laura Abbott wrote: > On 2/26/19 6:29 AM, Christoph Hellwig wrote: > > I don't think this is a good idea. The whole concept of just passing > > random GFP_ flags to dma_alloc_attrs / dma_alloc_coherent can't work, > > given that on many architectures we need to set up new page tables > > to remap the allocated memory, and we can't use arbitrary gfp flags > > for pte allocations. > > > > So instead of trying to pass them further down again we need to instead > > work to fix all callers of dma_alloc_attrs / dma_alloc_coherent > > that don't just pass GFP_KERNEL. > > > > What's the expected approach to fix callers? It's not clear how > you would fix the callers for the case that prompted this series > (context correctly used GFP_NOIO but it was not passed to > dma_alloc_coherent) Use the scope API (memalloc_noio_{save,restore}) at the scope boundary (lock or other restriction that prohibids IO path recursion) and you do not have to care about the specific allocation because it will inherit GFP_IO automagically.
Hi, The main goal of this patchset is to solve a deadlock in the CMA allocator, which happens because cma_alloc tries to sleep waiting for an IO in the GFP_NOIO path. This issue, which was reported by Gael Portay was discussed here: https://groups.google.com/a/lists.one-eyed-alien.net/forum/#!topic/usb-storage/BXpAsg-G1us My proposed requires reverting the patches that removed the gfp flags information from cma_alloc() (patches 1 to 3). According to the author, that parameter was removed because it misleads developers about what cma_alloc actually supports. In his specific case he had problems with GFP_ZERO. With that in mind I gave a try at implementing GFP_ZERO in a quite trivial way in patch 4. Finally, patches 5 and 6 attempt to fix the issue by avoiding the unecessary serialization done around alloc_contig_range. This is my first adventure in the mm subsystem, so I hope I didn't screw up something very obvious. I tested this on the workload that was deadlocking (arm board, with CMA intensive operations from the GPU and USB), as well as some scripting on top of debugfs. Is there any regression test I should be running, which specially applies to the CMA code? Gabriel Krisman Bertazi (6): Revert "kernel/dma: remove unsupported gfp_mask parameter from dma_alloc_from_contiguous()" Revert "mm/cma: remove unsupported gfp_mask parameter from cma_alloc()" cma: Warn about callers requesting unsupported flags cma: Add support for GFP_ZERO page_isolation: Propagate temporary pageblock isolation error cma: Isolate pageblocks speculatively during allocation arch/arm/mm/dma-mapping.c | 5 +-- arch/arm64/mm/dma-mapping.c | 2 +- arch/powerpc/kvm/book3s_hv_builtin.c | 2 +- arch/xtensa/kernel/pci-dma.c | 2 +- drivers/iommu/amd_iommu.c | 2 +- drivers/iommu/intel-iommu.c | 3 +- drivers/s390/char/vmcp.c | 2 +- drivers/staging/android/ion/ion_cma_heap.c | 2 +- include/linux/cma.h | 2 +- include/linux/dma-contiguous.h | 4 +- kernel/dma/contiguous.c | 6 +-- kernel/dma/direct.c | 3 +- kernel/dma/remap.c | 2 +- mm/cma.c | 51 ++++++++++++++++++---- mm/cma_debug.c | 2 +- mm/page_isolation.c | 20 ++++++--- 16 files changed, 74 insertions(+), 36 deletions(-)