| Message ID | 20221113163535.884299-8-hch@lst.de |
|---|---|
| State | Accepted |
| Commit | ffcb754584603adf7039d7972564fbf6febdc542 |
| Headers | show |
| Series | [1/7] media: videobuf-dma-contig: use dma_mmap_coherent | expand |
On Sun, Nov 13, 2022 at 05:35:35PM +0100, Christoph Hellwig wrote: > DMA allocations can never be turned back into a page pointer, so > requesting compound pages doesn't make sense and it can't even be > supported at all by various backends. > > Reject __GFP_COMP with a warning in dma_alloc_attrs, and stop clearing > the flag in the arm dma ops and dma-iommu. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > arch/arm/mm/dma-mapping.c | 17 ----------------- > drivers/iommu/dma-iommu.c | 3 --- > kernel/dma/mapping.c | 8 ++++++++ > 3 files changed, 8 insertions(+), 20 deletions(-) > > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c > index d7909091cf977..c135f6e37a00c 100644 > --- a/arch/arm/mm/dma-mapping.c > +++ b/arch/arm/mm/dma-mapping.c > @@ -564,14 +564,6 @@ static void *__dma_alloc(struct device *dev, size_t size, dma_addr_t *handle, > if (mask < 0xffffffffULL) > gfp |= GFP_DMA; > > - /* > - * Following is a work-around (a.k.a. hack) to prevent pages > - * with __GFP_COMP being passed to split_page() which cannot > - * handle them. The real problem is that this flag probably > - * should be 0 on ARM as it is not supported on this > - * platform; see CONFIG_HUGETLBFS. > - */ > - gfp &= ~(__GFP_COMP); > args.gfp = gfp; > > *handle = DMA_MAPPING_ERROR; > @@ -1093,15 +1085,6 @@ static void *arm_iommu_alloc_attrs(struct device *dev, size_t size, > return __iommu_alloc_simple(dev, size, gfp, handle, > coherent_flag, attrs); > > - /* > - * Following is a work-around (a.k.a. hack) to prevent pages > - * with __GFP_COMP being passed to split_page() which cannot > - * handle them. The real problem is that this flag probably > - * should be 0 on ARM as it is not supported on this > - * platform; see CONFIG_HUGETLBFS. > - */ > - gfp &= ~(__GFP_COMP); > - > pages = __iommu_alloc_buffer(dev, size, gfp, attrs, coherent_flag); > if (!pages) > return NULL; > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c > index 9297b741f5e80..f798c44e09033 100644 > --- a/drivers/iommu/dma-iommu.c > +++ b/drivers/iommu/dma-iommu.c > @@ -744,9 +744,6 @@ static struct page **__iommu_dma_alloc_pages(struct device *dev, > /* IOMMU can map any pages, so himem can also be used here */ > gfp |= __GFP_NOWARN | __GFP_HIGHMEM; > > - /* It makes no sense to muck about with huge pages */ > - gfp &= ~__GFP_COMP; > - > while (count) { > struct page *page = NULL; > unsigned int order_size; > diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c > index 33437d6206445..c026a5a5e0466 100644 > --- a/kernel/dma/mapping.c > +++ b/kernel/dma/mapping.c > @@ -498,6 +498,14 @@ void *dma_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle, > > WARN_ON_ONCE(!dev->coherent_dma_mask); > > + /* > + * DMA allocations can never be turned back into a page pointer, so > + * requesting compound pages doesn't make sense (and can't even be > + * supported at all by various backends). > + */ > + if (WARN_ON_ONCE(flag & __GFP_COMP)) > + return NULL; In RDMA patches, you wrote that GFP_USER is not legal flag either. So it is better to WARN here for everything that is not allowed. > + > if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr)) > return cpu_addr; > > -- > 2.30.2 >
On 13.11.2022 17:35, Christoph Hellwig wrote: > DMA allocations can never be turned back into a page pointer, so > requesting compound pages doesn't make sense and it can't even be > supported at all by various backends. > > Reject __GFP_COMP with a warning in dma_alloc_attrs, and stop clearing > the flag in the arm dma ops and dma-iommu. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > arch/arm/mm/dma-mapping.c | 17 ----------------- > drivers/iommu/dma-iommu.c | 3 --- > kernel/dma/mapping.c | 8 ++++++++ > 3 files changed, 8 insertions(+), 20 deletions(-) > > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c > index d7909091cf977..c135f6e37a00c 100644 > --- a/arch/arm/mm/dma-mapping.c > +++ b/arch/arm/mm/dma-mapping.c > @@ -564,14 +564,6 @@ static void *__dma_alloc(struct device *dev, size_t size, dma_addr_t *handle, > if (mask < 0xffffffffULL) > gfp |= GFP_DMA; > > - /* > - * Following is a work-around (a.k.a. hack) to prevent pages > - * with __GFP_COMP being passed to split_page() which cannot > - * handle them. The real problem is that this flag probably > - * should be 0 on ARM as it is not supported on this > - * platform; see CONFIG_HUGETLBFS. > - */ > - gfp &= ~(__GFP_COMP); > args.gfp = gfp; > > *handle = DMA_MAPPING_ERROR; > @@ -1093,15 +1085,6 @@ static void *arm_iommu_alloc_attrs(struct device *dev, size_t size, > return __iommu_alloc_simple(dev, size, gfp, handle, > coherent_flag, attrs); > > - /* > - * Following is a work-around (a.k.a. hack) to prevent pages > - * with __GFP_COMP being passed to split_page() which cannot > - * handle them. The real problem is that this flag probably > - * should be 0 on ARM as it is not supported on this > - * platform; see CONFIG_HUGETLBFS. > - */ > - gfp &= ~(__GFP_COMP); > - > pages = __iommu_alloc_buffer(dev, size, gfp, attrs, coherent_flag); > if (!pages) > return NULL; > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c > index 9297b741f5e80..f798c44e09033 100644 > --- a/drivers/iommu/dma-iommu.c > +++ b/drivers/iommu/dma-iommu.c > @@ -744,9 +744,6 @@ static struct page **__iommu_dma_alloc_pages(struct device *dev, > /* IOMMU can map any pages, so himem can also be used here */ > gfp |= __GFP_NOWARN | __GFP_HIGHMEM; > > - /* It makes no sense to muck about with huge pages */ > - gfp &= ~__GFP_COMP; > - > while (count) { > struct page *page = NULL; > unsigned int order_size; > diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c > index 33437d6206445..c026a5a5e0466 100644 > --- a/kernel/dma/mapping.c > +++ b/kernel/dma/mapping.c > @@ -498,6 +498,14 @@ void *dma_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle, > > WARN_ON_ONCE(!dev->coherent_dma_mask); > > + /* > + * DMA allocations can never be turned back into a page pointer, so > + * requesting compound pages doesn't make sense (and can't even be > + * supported at all by various backends). > + */ > + if (WARN_ON_ONCE(flag & __GFP_COMP)) > + return NULL; > + > if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr)) > return cpu_addr; > Best regards
On Wed, Nov 16, 2022 at 07:11:06AM +0100, Christoph Hellwig wrote: > On Mon, Nov 14, 2022 at 10:11:50AM +0200, Leon Romanovsky wrote: > > In RDMA patches, you wrote that GFP_USER is not legal flag either. So it > > is better to WARN here for everything that is not allowed. > > So __GFP_COMP is actually problematic and changes behavior, and I plan > to lift an optimization from the arm code to the generic one that > only rounds up allocations to the next page size instead of the next > power of two, so I need this check now. Other flags including > GFP_USER are pretty bogus to, but I actually need to do a full audit > before rejecting them, which I've only done for GFP_COMP so far. ok, let's do it later. Thanks
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c index d7909091cf977..c135f6e37a00c 100644 --- a/arch/arm/mm/dma-mapping.c +++ b/arch/arm/mm/dma-mapping.c @@ -564,14 +564,6 @@ static void *__dma_alloc(struct device *dev, size_t size, dma_addr_t *handle, if (mask < 0xffffffffULL) gfp |= GFP_DMA; - /* - * Following is a work-around (a.k.a. hack) to prevent pages - * with __GFP_COMP being passed to split_page() which cannot - * handle them. The real problem is that this flag probably - * should be 0 on ARM as it is not supported on this - * platform; see CONFIG_HUGETLBFS. - */ - gfp &= ~(__GFP_COMP); args.gfp = gfp; *handle = DMA_MAPPING_ERROR; @@ -1093,15 +1085,6 @@ static void *arm_iommu_alloc_attrs(struct device *dev, size_t size, return __iommu_alloc_simple(dev, size, gfp, handle, coherent_flag, attrs); - /* - * Following is a work-around (a.k.a. hack) to prevent pages - * with __GFP_COMP being passed to split_page() which cannot - * handle them. The real problem is that this flag probably - * should be 0 on ARM as it is not supported on this - * platform; see CONFIG_HUGETLBFS. - */ - gfp &= ~(__GFP_COMP); - pages = __iommu_alloc_buffer(dev, size, gfp, attrs, coherent_flag); if (!pages) return NULL; diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c index 9297b741f5e80..f798c44e09033 100644 --- a/drivers/iommu/dma-iommu.c +++ b/drivers/iommu/dma-iommu.c @@ -744,9 +744,6 @@ static struct page **__iommu_dma_alloc_pages(struct device *dev, /* IOMMU can map any pages, so himem can also be used here */ gfp |= __GFP_NOWARN | __GFP_HIGHMEM; - /* It makes no sense to muck about with huge pages */ - gfp &= ~__GFP_COMP; - while (count) { struct page *page = NULL; unsigned int order_size; diff --git a/kernel/dma/mapping.c b/kernel/dma/mapping.c index 33437d6206445..c026a5a5e0466 100644 --- a/kernel/dma/mapping.c +++ b/kernel/dma/mapping.c @@ -498,6 +498,14 @@ void *dma_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle, WARN_ON_ONCE(!dev->coherent_dma_mask); + /* + * DMA allocations can never be turned back into a page pointer, so + * requesting compound pages doesn't make sense (and can't even be + * supported at all by various backends). + */ + if (WARN_ON_ONCE(flag & __GFP_COMP)) + return NULL; + if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr)) return cpu_addr;
DMA allocations can never be turned back into a page pointer, so requesting compound pages doesn't make sense and it can't even be supported at all by various backends. Reject __GFP_COMP with a warning in dma_alloc_attrs, and stop clearing the flag in the arm dma ops and dma-iommu. Signed-off-by: Christoph Hellwig <hch@lst.de> --- arch/arm/mm/dma-mapping.c | 17 ----------------- drivers/iommu/dma-iommu.c | 3 --- kernel/dma/mapping.c | 8 ++++++++ 3 files changed, 8 insertions(+), 20 deletions(-)