| Message ID | 20221113163535.884299-8-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| 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 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.
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(-)