Skip to contentSkip to conversation

arm64: Workaround renaming of __GFP_WAIT

PATCH New Robin Murphy 5 participants 9 messages Oct 16, 2015 – Oct 28, 2015 arm64

lore
Share & tools
lore

Robin Murphy

author
#0 lore raw

PATCH 1/2 arm64: Workaround renaming of __GFP_WAIT

The imminent renaming of __GFP_WAIT in the mm tree conflicts with its use in the new IOMMU DMA ops; introduce a temporary local version of its replacement to smooth over the transition.

This patch should be reverted at 4.4-rc1.

CC Mel Gorman <[email protected]>
CC Andrew Morton <[email protected]>
Reported-by Sudeep Holla <[email protected]>
Signed-off-by Robin Murphy <[email protected]>

---

Sudeep points out that there are pending changes in -next touching arm64 which I hadn't spotted, which end up breaking the build when merged with my changes in the IOMMU tree. Catalin, would you mind acking these fixes so that Joerg can carry them? We should be able to send the revert through arm64 once the dust has settled.

Thanks,
Robin.

---
 arch/arm64/mm/dma-mapping.c | 4 ++++
 1 file changed, 4 insertions(+)
arch/arm64/mm/dma-mapping.c +4 -0
index 6320361..66444df 100644
@@ -546,6 +546,10 @@ static void flush_page(struct device *dev, const void *virt, phys_addr_t phys)
 	__dma_flush_range(virt, virt + PAGE_SIZE);
 }
 
+#ifdef __GFP_WAIT
+#define gfpflags_allow_blocking(gfp) ((gfp) & __GFP_WAIT)
+#endif
+
 static void *__iommu_alloc_attrs(struct device *dev, size_t size,
 				 dma_addr_t *handle, gfp_t gfp,
 				 struct dma_attrs *attrs)
Signature · 1 line
1.9.1
author
#1 lore raw

PATCH 2/2 arm64: Use gfpflags_allow_blocking()

__GFP_WAIT is going away to live its life under a new identity; convert __iommu_alloc_attrs() to the new helper function instead.

CC Mel Gorman <[email protected]>
CC Andrew Morton <[email protected]>
Reported-by Sudeep Holla <[email protected]>
Signed-off-by Robin Murphy <[email protected]>
---
 arch/arm64/mm/dma-mapping.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm64/mm/dma-mapping.c +1 -1
index 66444df..40cd4af 100644
@@ -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);
Signature · 1 line
1.9.1
#3 lore raw

PATCH 2/2 arm64: Use gfpflags_allow_blocking()

In reply to Robin Murphy
Quoted 8 lines · On Fri, 16 Oct 2015 16:33:42 +0100 Robin Murphy <[email protected]> wrote:

__GFP_WAIT is going away to live its life under a new identity; convert __iommu_alloc_attrs() to the new helper function instead.

...

arch/arm64/mm/dma-mapping.c static void *__iommu_alloc_attrs(struct device *dev, size_t size, · line 571

Quoted 7 lines
arch/arm64/mm/dma-mapping.c +1 -1
@@ -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

arch/arm64/mm/dma-mapping.c renamed from arch/arm64/mm/dma-mapping.c~mm-page_alloc-rename-__gfp_wait-to-__gfp_reclaim-arm-fix +1 -1
@@ -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);

?

author
#4 lore raw

PATCH 2/2 arm64: Use gfpflags_allow_blocking()

Hi Andrew,

Quoted 32 lines · Andrew Morton
Quoted 8 lines · On Fri, 16 Oct 2015 16:33:42 +0100 Robin Murphy <[email protected]> wrote:

__GFP_WAIT is going away to live its life under a new identity; convert __iommu_alloc_attrs() to the new helper function instead.

...

arch/arm64/mm/dma-mapping.c static void *__iommu_alloc_attrs(struct device *dev, size_t size, · line 571

Quoted 7 lines
arch/arm64/mm/dma-mapping.c +1 -1
@@ -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

arch/arm64/mm/dma-mapping.c renamed from arch/arm64/mm/dma-mapping.c~mm-page_alloc-rename-__gfp_wait-to-__gfp_reclaim-arm-fix +1 -1
@@ -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);
Quoted 1 line

?

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.

#5 lore raw

PATCH 2/2 arm64: Use gfpflags_allow_blocking()

Quoted 35 lines · Robin Murphy

Hi Andrew,

Quoted 32 lines · Andrew Morton
Quoted 8 lines · On Fri, 16 Oct 2015 16:33:42 +0100 Robin Murphy <[email protected]> wrote:

__GFP_WAIT is going away to live its life under a new identity; convert __iommu_alloc_attrs() to the new helper function instead.

...

arch/arm64/mm/dma-mapping.c static void *__iommu_alloc_attrs(struct device *dev, size_t size, · line 571

Quoted 7 lines
arch/arm64/mm/dma-mapping.c +1 -1
@@ -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

arch/arm64/mm/dma-mapping.c renamed from arch/arm64/mm/dma-mapping.c~mm-page_alloc-rename-__gfp_wait-to-__gfp_reclaim-arm-fix +1 -1
@@ -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);
Quoted 9 lines
Quoted 1 line

?

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.

Signature · 2 lines
Mel Gorman
SUSE Labs
maintainer
#6 lore raw

PATCH 1/2 arm64: Workaround renaming of __GFP_WAIT

In reply to Robin Murphy
Quoted 10 lines · Robin Murphy

The imminent renaming of __GFP_WAIT in the mm tree conflicts with its use in the new IOMMU DMA ops; introduce a temporary local version of its replacement to smooth over the transition.

This patch should be reverted at 4.4-rc1.

CC Mel Gorman <[email protected]>
CC Andrew Morton <[email protected]>
Reported-by Sudeep Holla <[email protected]>
Signed-off-by Robin Murphy <[email protected]>
Acked-by Catalin Marinas <[email protected]>
#7 lore raw

PATCH 1/2 arm64: Workaround renaming of __GFP_WAIT

In reply to Robin Murphy

Hi Robin,

Quoted 5 lines · Robin Murphy

The imminent renaming of __GFP_WAIT in the mm tree conflicts with its use in the new IOMMU DMA ops; introduce a temporary local version of its replacement to smooth over the transition.

This patch should be reverted at 4.4-rc1.

I lost track here, are these two patches required in the iommu tree for the next merge window?

Joerg

author
#8 lore raw

PATCH 1/2 arm64: Workaround renaming of __GFP_WAIT

Hi Joerg,

Quoted 11 lines · Joerg Roedel

Hi Robin,

Quoted 5 lines · Robin Murphy

The imminent renaming of __GFP_WAIT in the mm tree conflicts with its use in the new IOMMU DMA ops; introduce a temporary local version of its replacement to smooth over the transition.

This patch should be reverted at 4.4-rc1.

I lost track here, are these two patches required in the iommu tree for the next merge window?

Andrew has the equivalent of patch 2 as a fixup on top of Mel's series, which Stephen is currently carrying as a merge resolution in -next. I'm assuming this will go into the final merge as well, so as long as Linus takes the IOMMU tree first we should be OK as-is.

Robin.

Quoted 4 lines

Joerg