Regression after commit 19809c2da28a ("mm, vmalloc: use __GFP_HIGHMEM implicitly")

A user with i386 instead of AMD64 machine reports [1] that commit 19809c2da28a ("mm, vmalloc: use __GFP_HIGHMEM implicitlya??) causes a regression. BUG_ON(PageHighMem(pg)) in drivers/media/common/saa7146/saa7146_core.c always gets triggered after that commit.

Also there are other BUG_ON(PageHighMem()) in drivers/media, I think they will get hit by same regression in 32bit machine too.

Quoted 2 lines · Kai Heng Feng

A user with i386 instead of AMD64 machine reports [1] that commit 19809c2da28a ("mm, vmalloc: use __GFP_HIGHMEM implicitlya??) causes a regression. BUG_ON(PageHighMem(pg)) in drivers/media/common/saa7146/saa7146_core.c always gets triggered after that commit.

Well, the BUG_ON is wrong. You can absolutely have pages which are both HighMem and under the 4GB boundary. Only the first 896MB (iirc) are LowMem, and the next 3GB of pages are available to vmalloc_32().

Quoted 1 line

Also there are other BUG_ON(PageHighMem()) in drivers/media, I think they will get hit by same regression in 32bit machine too.

I fixed one of them. I think the other three are also bogus, but it's hard to say; the comments say "DMA to HighMem might not work", and they probably mean "above the 4GB boundary", but I really don't know.

(since two drivers now have this code, maybe it should be part of the core MM API? Or maybe there's already something better they should be using?)

drivers/media/common/saa7146/saa7146_core.c +1 -1

index 9f7c5b0a6b45..329fd43228ff 100644
@@ -160,7 +160,7 @@ static struct scatterlist* vmalloc_to_sg(unsigned char *virt, int nr_pages)
  		pg = vmalloc_to_page(virt);
  		if (NULL == pg)
  			goto err;
-		BUG_ON(PageHighMem(pg));
+		BUG_ON(page_to_pfn(pg) >= (1 << (32 - PAGE_SHIFT)));
  		sg_set_page(&sglist[i], pg, PAGE_SIZE, 0);
  	}
  	return sglist;

drivers/media/v4l2-core/videobuf-dma-sg.c +0 -0

index f412429cf5ba..b5ec74b9c867 100644

Quoted 6 lines

+++ b/drivers/media/v4l2-core/videobuf-dma-sg.c

drivers/media/common/saa7146/saa7146_core.c +0 -1

@@ -77,7 +77,7 @@ static struct scatterlist *videobuf_vmalloc_to_sg(unsigned char *virt,
  		pg = vmalloc_to_page(virt);
  		if (NULL == pg)
  			goto err;
-		BUG_ON(PageHighMem(pg));

drivers/media/common/saa7146/saa7146_core.c static struct scatterlist* vmalloc_to_sg(unsigned char *virt, int nr_pages) · line 166

Quoted 5 lines

drivers/media/common/saa7146/saa7146_core.c +1 -0

+		BUG_ON(page_to_pfn(pg) >= (1 << (32 - PAGE_SHIFT)));
  		sg_set_page(&sglist[i], pg, PAGE_SIZE, 0);
  	}
  	return sglist;

the vzalloc in this function needs to be switched to vmalloc32 if it actually wants to guarantee 32-bit memory.

Quoted 6 lines · Laura Abbott

Quoted 6 lines

+++ b/drivers/media/v4l2-core/videobuf-dma-sg.c

drivers/media/common/saa7146/saa7146_core.c +0 -1

@@ -77,7 +77,7 @@ static struct scatterlist *videobuf_vmalloc_to_sg(unsigned char *virt,
   		pg = vmalloc_to_page(virt);
   		if (NULL == pg)
   			goto err;
-		BUG_ON(PageHighMem(pg));

Quoted 8 lines

drivers/media/common/saa7146/saa7146_core.c static struct scatterlist* vmalloc_to_sg(unsigned char *virt, int nr_pages) · line 166

Quoted 5 lines

drivers/media/common/saa7146/saa7146_core.c +1 -0

+		BUG_ON(page_to_pfn(pg) >= (1 << (32 - PAGE_SHIFT)));
   		sg_set_page(&sglist[i], pg, PAGE_SIZE, 0);
   	}
   	return sglist;

the vzalloc in this function needs to be switched to vmalloc32 if it actually wants to guarantee 32-bit memory.

Whoops, you got confused between the sglist allocation and the allocation of the pages which will be mapped ...

Quoted 2 lines · Kai Heng Feng

A user with i386 instead of AMD64 machine reports [1] that commit 19809c2da28a ("mm, vmalloc: use __GFP_HIGHMEM implicitlya??) causes a regression. BUG_ON(PageHighMem(pg)) in drivers/media/common/saa7146/saa7146_core.c always gets triggered after that commit.

Well, the BUG_ON is wrong. You can absolutely have pages which are both HighMem and under the 4GB boundary. Only the first 896MB (iirc) are LowMem, and the next 3GB of pages are available to vmalloc_32().

So ... we could enable ZONE_DMA32 on 32-bit architectures. I don't know what side-effects that might have; it's clearly only been tested on 64-bit architectures so far.

It might be best to just revert 19809c2da28a and the follow-on 704b862f9efd.

Alternatively, try this. It passes in GFP_DMA32 from vmalloc_32, regardless of whether ZONE_DMA32 exists or not. If ZONE_DMA32 doesn't exist, then we clear it in __vmalloc_area_node(), after using it to determine that we shouldn't set __GFP_HIGHMEM.

mm/vmalloc.c +5 -4

index 673942094328..91e8a95123c4 100644
@@ -1669,10 +1669,11 @@ static void *__vmalloc_area_node(struct vm_struct  
 *area, gfp_t gfp_mask,
 	struct page **pages;
 	unsigned int nr_pages, array_size, i;
 	const gfp_t nested_gfp = (gfp_mask & GFP_RECLAIM_MASK) | __GFP_ZERO;
-	const gfp_t alloc_mask = gfp_mask | __GFP_NOWARN;
-	const gfp_t highmem_mask = (gfp_mask & (GFP_DMA | GFP_DMA32)) ?
-					0 :
-					__GFP_HIGHMEM;
+	gfp_t alloc_mask = gfp_mask | __GFP_NOWARN;
+	if (!(alloc_mask & GFP_ZONEMASK))
+		alloc_mask |= __GFP_HIGHMEM;
+	if (!IS_ENABLED(CONFIG_ZONE_DMA32) && (alloc_mask & __GFP_DMA32))
+		alloc_mask &= ~__GFP_DMA32;
 
 	nr_pages = get_vm_area_size(area) >> PAGE_SHIFT;

mm/vmalloc.c +1 -1

 	array_size = (nr_pages * sizeof(struct page *));

@@ -1680,7 +1681,7 @@ static void *__vmalloc_area_node(struct vm_struct  
 *area, gfp_t gfp_mask,
 	area->nr_pages = nr_pages;
 	/* Please note that the recursion is strictly bounded. */
 	if (array_size > PAGE_SIZE) {
-		pages = __vmalloc_node(array_size, 1, nested_gfp|highmem_mask,
+		pages = __vmalloc_node(array_size, 1, nested_gfp|__GFP_HIGHMEM,
 				PAGE_KERNEL, node, area->caller);
 	} else {

mm/vmalloc.c +2 -2

 		pages = kmalloc_node(array_size, nested_gfp, node);

@@ -1696,9 +1697,9 @@ static void *__vmalloc_area_node(struct vm_struct  
 *area, gfp_t gfp_mask,
 		struct page *page;
 
 		if (node == NUMA_NO_NODE)
-			page = alloc_page(alloc_mask|highmem_mask);
+			page = alloc_page(alloc_mask);
 		else
-			page = alloc_pages_node(node, alloc_mask|highmem_mask, 0);
+			page = alloc_pages_node(node, alloc_mask, 0);
 
 		if (unlikely(!page)) {

mm/vmalloc.c +1 -2

 			/* Successfully allocated i pages, free them in __vunmap() */

@@ -1706,7 +1707,7 @@ static void *__vmalloc_area_node(struct vm_struct  
 *area, gfp_t gfp_mask,
 			goto fail;
 		}
 		area->pages[i] = page;
-		if (gfpflags_allow_blocking(gfp_mask|highmem_mask))
+		if (gfpflags_allow_blocking(gfp_mask))
 			cond_resched();
 	}

@@ -1942,12 +1943,10 @@ void *vmalloc_exec(unsigned long size)
 			      NUMA_NO_NODE, __builtin_return_address(0));
 }
 
-#if defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA32)

Hi Matthew,

Quoted 5 lines · Matthew Wilcox

Alternatively, try this. It passes in GFP_DMA32 from vmalloc_32, regardless of whether ZONE_DMA32 exists or not. If ZONE_DMA32 doesn't exist, then we clear it in __vmalloc_area_node(), after using it to determine that we shouldn't set __GFP_HIGHMEM.

IIUC, I need to let drivers/media drivers start using vmalloc_32() with your patch, right?

Quoted 7 lines · Matthew Wilcox

Quoted 2 lines · Kai Heng Feng

A user with i386 instead of AMD64 machine reports [1] that commit 19809c2da28a ("mm, vmalloc: use __GFP_HIGHMEM implicitly”) causes a regression. BUG_ON(PageHighMem(pg)) in drivers/media/common/saa7146/saa7146_core.c always gets triggered after that commit.

Well, the BUG_ON is wrong. You can absolutely have pages which are both HighMem and under the 4GB boundary. Only the first 896MB (iirc) are LowMem, and the next 3GB of pages are available to vmalloc_32().

... nevertheless, 19809c2da28a does in fact break vmalloc_32 on 32-bit. Look:

#if defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA32) #define GFP_VMALLOC32 GFP_DMA32 | GFP_KERNEL #elif defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA) #define GFP_VMALLOC32 GFP_DMA | GFP_KERNEL #else
#define GFP_VMALLOC32 GFP_KERNEL
#endif

So we pass in GFP_KERNEL to __vmalloc_node, which calls __vmalloc_node_range which calls __vmalloc_area_node, which ORs in __GFP_HIGHMEM.

Quoted 10 lines · Matthew Wilcox

... nevertheless, 19809c2da28a does in fact break vmalloc_32 on 32-bit. Look:

#if defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA32) #define GFP_VMALLOC32 GFP_DMA32 | GFP_KERNEL #elif defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA) #define GFP_VMALLOC32 GFP_DMA | GFP_KERNEL #else
#define GFP_VMALLOC32 GFP_KERNEL
#endif

Quoted 2 lines

So we pass in GFP_KERNEL to __vmalloc_node, which calls __vmalloc_node_range which calls __vmalloc_area_node, which ORs in __GFP_HIGHMEM.

Dohh. I have missed this. I was convinced that we always add GFP_DMA32 when doing vmalloc_32. Sorry about that. The above definition looks quite weird to be honest. First of all do we have any 64b system without both DMA and DMA32 zones? If yes, what is the actual semantic of vmalloc_32? Or is there any magic forcing GFP_KERNEL into low 32b?

Also I would expect that __GFP_DMA32 should do the right thing on 32b systems. So something like the below should do the trick

mm/vmalloc.c +2 -1

index 673942094328..2eab5d1ef548 100644
@@ -1947,7 +1947,8 @@ void *vmalloc_exec(unsigned long size)
 #elif defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA)
 #define GFP_VMALLOC32 GFP_DMA | GFP_KERNEL
 #else
-#define GFP_VMALLOC32 GFP_KERNEL
+/* This should be only 32b systems */
+#define GFP_VMALLOC32 GFP_DMA32 | GFP_KERNEL
 #endif
 
 /**

Signature · 9 lines

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

Now, longer-term, perhaps we should do the following:

#ifdef CONFIG_ZONE_DMA32
#define OPT_ZONE_DMA32 ZONE_DMA32
#elif defined(CONFIG_64BIT)
#define OPT_ZONE_DMA OPT_ZONE_DMA
#else
#define OPT_ZONE_DMA32 ZONE_NORMAL
#endif

Then we wouldn't need the ifdef here and could always use GFP_DMA32 | GFP_KERNEL. Would need to audit current users and make sure they wouldn't be broken by such a change.

Quoted 9 lines · Matthew Wilcox

Now, longer-term, perhaps we should do the following:

#ifdef CONFIG_ZONE_DMA32
#define OPT_ZONE_DMA32 ZONE_DMA32
#elif defined(CONFIG_64BIT)
#define OPT_ZONE_DMA OPT_ZONE_DMA
#else
#define OPT_ZONE_DMA32 ZONE_NORMAL
#endif

For consistent / coherent memory, we have an allocation function. But we don't have an allocation function for streaming memory, which is what these drivers want. They also flush the DMA memory and then access the memory through a different virtual mapping, which I'm not sure is going to work well on virtually-indexed caches like SPARC and PA-RISC (maybe not MIPS either?)

Quoted 11 lines · Matthew Wilcox

Quoted 9 lines · Matthew Wilcox

Now, longer-term, perhaps we should do the following:

#ifdef CONFIG_ZONE_DMA32
#define OPT_ZONE_DMA32 ZONE_DMA32
#elif defined(CONFIG_64BIT)
#define OPT_ZONE_DMA OPT_ZONE_DMA
#else
#define OPT_ZONE_DMA32 ZONE_NORMAL
#endif

Quoted 6 lines

For consistent / coherent memory, we have an allocation function. But we don't have an allocation function for streaming memory, which is what these drivers want. They also flush the DMA memory and then access the memory through a different virtual mapping, which I'm not sure is going to work well on virtually-indexed caches like SPARC and PA-RISC (maybe not MIPS either?)

Perhaps I (and a number of other people ...) have misunderstood the semantics of GFP_DMA32. Perhaps GFP_DMA32 is not "allocate memory below 4GB", perhaps it's "allocate memory which can be mapped below 4GB".

Machines with an IOMMU can use ZONE_NORMAL. Machines with no IOMMU can choose to allocate memory with a physical address below 4GB.

After all, it has 'DMA' right there in the name.

Quoted 11 lines · Michal Hocko

Quoted 10 lines · Matthew Wilcox

... nevertheless, 19809c2da28a does in fact break vmalloc_32 on 32-bit. Look:

#if defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA32) #define GFP_VMALLOC32 GFP_DMA32 | GFP_KERNEL #elif defined(CONFIG_64BIT) && defined(CONFIG_ZONE_DMA) #define GFP_VMALLOC32 GFP_DMA | GFP_KERNEL #else
#define GFP_VMALLOC32 GFP_KERNEL
#endif

Quoted 8 lines

Quoted 2 lines

So we pass in GFP_KERNEL to __vmalloc_node, which calls __vmalloc_node_range which calls __vmalloc_area_node, which ORs in __GFP_HIGHMEM.

Dohh. I have missed this. I was convinced that we always add GFP_DMA32 when doing vmalloc_32. Sorry about that. The above definition looks quite weird to be honest. First of all do we have any 64b system without both DMA and DMA32 zones? If yes, what is the actual semantic of vmalloc_32? Or is there any magic forcing GFP_KERNEL into low 32b?

mmzone.h has the following, which may be inaccurate / out of date:

         * parisc, ia64, sparc  <4G
         * s390                 <2G
         * arm                  Various
         * alpha                Unlimited or 0-16MB.

*
* i386, x86_64 and multiple other arches
* <16M.

It claims ZONE_DMA32 is x86-64 only, which is incorrect; it's now used by arm64, ia64, mips, powerpc, tile.

Quoted 2 lines

Also I would expect that __GFP_DMA32 should do the right thing on 32b systems. So something like the below should do the trick

Oh, I see. Because we have:

#ifdef CONFIG_ZONE_DMA32
#define OPT_ZONE_DMA32 ZONE_DMA32
#else
#define OPT_ZONE_DMA32 ZONE_NORMAL
#endif

we'll end up allocating from ZONE_NORMAL if a non-DMA32 architecture asks for GFP_DMA32 memory. Thanks; I missed that.

I'd recommend this instead then:

#if defined(CONFIG_64BIT) && !defined(CONFIG_ZONE_DMA32) #define GFP_VMALLOC32 GFP_DMA | GFP_KERNEL #else
#define GFP_VMALLOC32 GFP_DMA32 | GFP_KERNEL #endif

I think it's clearer than the three-way #if.

Now, longer-term, perhaps we should do the following:

#ifdef CONFIG_ZONE_DMA32
#define OPT_ZONE_DMA32 ZONE_DMA32
#elif defined(CONFIG_64BIT)
#define OPT_ZONE_DMA OPT_ZONE_DMA
#else
#define OPT_ZONE_DMA32 ZONE_NORMAL
#endif

Then we wouldn't need the ifdef here and could always use GFP_DMA32 | GFP_KERNEL. Would need to audit current users and make sure they wouldn't be broken by such a change.

I noticed a mistake in 704b862f9efd;

(quoted diff) +1 -1

-               pages = __vmalloc_node(array_size, 1, nested_gfp|__GFP_HIGHMEM,
+               pages = __vmalloc_node(array_size, 1, nested_gfp|highmem_mask,

We should unconditionally use __GFP_HIGHMEM here instead of highmem_mask because this is where we allocate the array to hold the struct page pointers. This can be allocated from highmem, and does not need to be allocated from ZONE_NORMAL.

Similarly,

(quoted diff) +1 -1

-               if (gfpflags_allow_blocking(gfp_mask))
+               if (gfpflags_allow_blocking(gfp_mask|highmem_mask))

is not needed (it's not *wrong*, it was just an unnecessary change).