: Better checking in range_straddles_page_boundary

PATCH In review Chris Lalancette 2 participants 3 messages Oct 22, 2008

Share & tools

Chris Lalancette

author

2008-10-22 09:05 UTC #0 lore raw

PATCH 1/1 : Better checking in range_straddles_page_boundary

All,
Attached is a simple patch to slightly rework the logic in
range_straddles_page_boundary(). The reason for this is to avoid a crash we are seeing on 32-bit dom0. Basically, the contiguous_bitmap is allocated based on max_low_pfn. However, the swiotlb code can be passed a request (in swiotlb_map_sg) that is > 1 page (I've seen 2 pages), and is also a page (sg->page) > max_low_pfn. If this combination happens, then you get a fatal page fault when doing the test_bit(pfn, contiguous_bitmap). For that reason, rework the logic in range_straddles_page_boundary so that if it gets a request > 1 page, and it's above max_low_pfn, then we force the splitting of the request.
In our testing, this seems to fix the issue.

Note that the patch is against the RHEL-5 2.6.18 code, but the patch should apply with a little fuzz to the linux-2.6.18-xen.hg tree.

Signed-off-by Chris Lalancette <[email protected]>

arch/i386/kernel/pci-dma-xen.c renamed from arch/i386/kernel/pci-dma-xen.c.orig +24 -2

@@ -13,6 +13,7 @@
 #include <linux/pci.h>
 #include <linux/module.h>
 #include <linux/version.h>
+#include <linux/bootmem.h>
 #include <asm/io.h>
 #include <xen/balloon.h>
 #include <asm/swiotlb.h>

@@ -134,12 +135,33 @@ int range_straddles_page_boundary(paddr_
 	unsigned long pfn = p >> PAGE_SHIFT;
 	unsigned int offset = p & ~PAGE_MASK;
 
+	/*
+	 * 1.  If this request is going to stay on this page, it won't
+	 * cross a page boundary
+	 */
 	if (offset + size <= PAGE_SIZE)
 		return 0;
-	if (test_bit(pfn, contiguous_bitmap))
-		return 0;
+	/*
+	 * 2.  If all of the pages here are physically contiguous, we
+	 * technically cross a page boundary, but that's OK
+	 */
 	if (check_pages_physically_contiguous(pfn, offset, size))
 		return 0;
+	/*
+	 * 3.  If the above two tests failed, *and* we are a page > max_low_pfn,
+	 * then we have to straddle a page boundary (we won't be in the
+	 * contiguous_bitmap, and attempting to access it will cause a
+	 * page fault
+	 */
+	if (pfn > max_low_pfn)
+		return 1;
+	/*
+	 * 4.  Finally, check the contiguous bitmap for this pfn; if it's in
+	 * there, we are OK.
+	 */
+	  
+	if (test_bit(pfn, contiguous_bitmap))
+		return 0;
 	return 1;
 }

_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel

Keir Fraser

2008-10-22 10:47 UTC #1 lore raw

Quoted 10 lines · Chris Lalancette

Attached is a simple patch to slightly rework the logic in
range_straddles_page_boundary(). The reason for this is to avoid a crash we are
seeing on 32-bit dom0. Basically, the contiguous_bitmap is allocated based on max_low_pfn. However, the swiotlb code can be passed a request (in swiotlb_map_sg) that is > 1 page (I've seen 2 pages), and is also a page (sg->page) > max_low_pfn. If this combination happens, then you get a fatal page fault when doing the test_bit(pfn, contiguous_bitmap). For that reason, rework the logic in range_straddles_page_boundary so that if it gets a request
Quoted 1 line

Quoted 3 lines

1 page, and it's above max_low_pfn, then we force the splitting of the request.
In our testing, this seems to fix the issue.

How about we just get rid of the contiguous_bitmap? We might as well if you are going to push that check after calling check_pages_physically_contiguous(), since no extent which is acceptable to contiguous_bitmap should fail on check_pages_physically_contiguous(). I think I kept contiguous_bitmap only as a fast check before calling that slower function.

-- Keir

Chris Lalancette

author

2008-10-22 10:57 UTC #2 lore raw

Quoted 15 lines · Keir Fraser

Quoted 4 lines · Chris Lalancette
Attached is a simple patch to slightly rework the logic in
range_straddles_page_boundary(). The reason for this is to avoid a crash we are
seeing on 32-bit dom0. Basically, the contiguous_bitmap is allocated based on
Quoted 8 lines
max_low_pfn. However, the swiotlb code can be passed a request (in swiotlb_map_sg) that is > 1 page (I've seen 2 pages), and is also a page (sg->page) > max_low_pfn. If this combination happens, then you get a fatal page fault when doing the test_bit(pfn, contiguous_bitmap). For that reason, rework the logic in range_straddles_page_boundary so that if it gets a request 1 page, and it's above max_low_pfn, then we force the splitting of the request.
In our testing, this seems to fix the issue.

Quoted 6 lines

How about we just get rid of the contiguous_bitmap? We might as well if you are going to push that check after calling check_pages_physically_contiguous(), since no extent which is acceptable to contiguous_bitmap should fail on check_pages_physically_contiguous(). I think I kept contiguous_bitmap only as a fast check before calling that slower function.

Oh, hm, good point. If we think that the fast check is still good to have, the other option is to leave range_straddles_page_boundary as-is, and just allocate contiguous_bitmap with max_pfn (or end_pfn; I can never remember which is which i386 vs. x86_64).

Either way works for me, though; I don't think the check_pages_physically_contiguous is a huge performance bottleneck, especially since *most* requests are for 1 page; it's just the odd request that comes in with 2 pages (or more, but I've never seen more than 2 pages).

Signature · 1 line

Chris Lalancette