| Message ID | 20151016135900.bc1e10115a866a301dbb0cd8@linux-foundation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andrew, On 16/10/15 21:59, Andrew Morton wrote: > On Fri, 16 Oct 2015 16:33:42 +0100 Robin Murphy <robin.murphy@arm.com> wrote: > >> __GFP_WAIT is going away to live its life under a new identity; convert >> __iommu_alloc_attrs() to the new helper function instead. >> >> ... >> >> --- a/arch/arm64/mm/dma-mapping.c >> +++ b/arch/arm64/mm/dma-mapping.c >> @@ -566,7 +566,7 @@ static void *__iommu_alloc_attrs(struct device *dev, size_t size, >> */ >> gfp |= __GFP_ZERO; >> >> - if (gfp & __GFP_WAIT) { >> + if (gfpflags_allow_blocking(gfp)) { >> struct page **pages; >> pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, coherent); > > Seems unnecessarily elaborate. What's wrong with > > --- a/arch/arm64/mm/dma-mapping.c~mm-page_alloc-rename-__gfp_wait-to-__gfp_reclaim-arm-fix > +++ a/arch/arm64/mm/dma-mapping.c > @@ -562,7 +562,7 @@ static void *__iommu_alloc_attrs(struct > */ > gfp |= __GFP_ZERO; > > - if (gfp & __GFP_WAIT) { > + if (gfp & __GFP_RECLAIM) { > struct page **pages; > pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, coherent); > > > ? Well, in that case the charge of "unnecessarily elaborate" should have been directed at the original patch, and the 53 other locations where (flags & __GFP_WAIT) was changed as per the commit message: "Callers that are checking if they are non-blocking should use the helper gfpflags_allow_blocking() where possible." More importantly, it's also now apparently inconsistent with all the other dma_alloc_coherent() implementations, which thanks to the helper function are testing against __GFP_DIRECT_RECLAIM instead. As I have no clear understanding of what the difference between the two flags is, and how they relate to whether it's safe to call map_vm_area() or not (which is what principally matters here), I'm very uncomfortable with a change introducing that inconsistency. If instead of my two patches you'd prefer to carry a fix through -mm and coordinate with Joerg and Linus to ensure everything gets merged in the right order, that's fine by me, but either way the change needs to guarantee the same behaviour as all the other instances in arch/arm and arch/arm64 which return a remapped buffer (and I have to say personally I much prefer having the rather inscrutable flag logic hidden behind a clearly-named helper function). Thanks, Robin.
On Mon, Oct 19, 2015 at 01:43:13PM +0100, Robin Murphy wrote: > Hi Andrew, > > On 16/10/15 21:59, Andrew Morton wrote: > >On Fri, 16 Oct 2015 16:33:42 +0100 Robin Murphy <robin.murphy@arm.com> wrote: > > > >>__GFP_WAIT is going away to live its life under a new identity; convert > >>__iommu_alloc_attrs() to the new helper function instead. > >> > >>... > >> > >>--- a/arch/arm64/mm/dma-mapping.c > >>+++ b/arch/arm64/mm/dma-mapping.c > >>@@ -566,7 +566,7 @@ static void *__iommu_alloc_attrs(struct device *dev, size_t size, > >> */ > >> gfp |= __GFP_ZERO; > >> > >>- if (gfp & __GFP_WAIT) { > >>+ if (gfpflags_allow_blocking(gfp)) { > >> struct page **pages; > >> pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, coherent); > > > >Seems unnecessarily elaborate. What's wrong with > > > >--- a/arch/arm64/mm/dma-mapping.c~mm-page_alloc-rename-__gfp_wait-to-__gfp_reclaim-arm-fix > >+++ a/arch/arm64/mm/dma-mapping.c > >@@ -562,7 +562,7 @@ static void *__iommu_alloc_attrs(struct > > */ > > gfp |= __GFP_ZERO; > > > >- if (gfp & __GFP_WAIT) { > >+ if (gfp & __GFP_RECLAIM) { > > struct page **pages; > > pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, coherent); > > > > > >? > > Well, in that case the charge of "unnecessarily elaborate" should have been > directed at the original patch, and the 53 other locations where (flags & > __GFP_WAIT) was changed as per the commit message: > > "Callers that are checking if they are non-blocking should use the > helper gfpflags_allow_blocking() where possible." > The use of gfpflags_allows_blocking() like you originally had is actually preferred by me. __GFP_RECLAIM can return true when the caller only allows kswapd to wake which has nothing to do with blocking (currently). The helper was added to avoid this type of confusion.
--- a/arch/arm64/mm/dma-mapping.c~mm-page_alloc-rename-__gfp_wait-to-__gfp_reclaim-arm-fix +++ a/arch/arm64/mm/dma-mapping.c @@ -562,7 +562,7 @@ static void *__iommu_alloc_attrs(struct */ gfp |= __GFP_ZERO; - if (gfp & __GFP_WAIT) { + if (gfp & __GFP_RECLAIM) { struct page **pages; pgprot_t prot = __get_dma_pgprot(attrs, PAGE_KERNEL, coherent);