Paths

Table of Contentst

LinuxKPI: Add PAGE_IS_LKPI_PAGE for Linux struct page instead of vm_page
AcceptedPublic
Actions

Authored by bz on Sat, Jul 18, 11:45 AM.

Details

Reviewers

olce
dumbbell
markj
kib

Summary

Historically Linux 'page' was aliased to 'vm_page' which meant that all
"page accesses" went directly to our FreeBSD native system.
This was (any likely is) the most efficient way of doing this but it
means that we cannot (or do not want to) extend vm_page for LinuxKPI
specific fields leaving us in a limbo.

drm-kmod in the past has worked around this by changing code with
FreeBSD specific workarounds.
With the mt76 wireless drivers using page pools we are at a stage
that rather than re-writing the code we bite the bullet and rework
the entire LinuxKPI code to deal with a Linux struct page instead
building a 'shadow table' for mapping vm_page to page and back
in order to still have access to the 'backing store'.

In order to allow for a smoother transition until the initial code
can be reviewed, allow both the old alias and the new struct page
world to be compiled. That way the majority of people will not
be affected by the change at first, and people who want to try mt76
can define PAGE_IS_LKPI_PAGE in page.h (and manually re-compile drm-kmod
if needed).

The first cut of code is likely neither as efficient nor as memory
saving as it could be as we upfront allocate a full 'shadow table'
rather than having a (way) smaller pool and using a mapping or
allocating on demand. First try to make this work, then someone
with a lot more VM knowledge can try to make this work fast and
efficient.

Sponsored by: The FreeBSD Foundation
MFC after: 3 weeks

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 74910
Build 71793: arc lint + arc unit

Event Timeline

bz created this revision.Sat, Jul 18, 11:45 AM

Herald added a subscriber: imp. · View Herald TranscriptSat, Jul 18, 11:45 AM

bz requested review of this revision.Sat, Jul 18, 11:45 AM

Harbormaster completed remote builds in B74910: Diff 182170.Sat, Jul 18, 11:45 AM

bz mentioned this in D53440: LinuxKPI: Add work-in-pgroess PAGE_IS_LKPI_PAGE.Sat, Jul 18, 11:47 AM

bz added a parent revision: D58299: LinuxKPI: page.h: resort lines.Sat, Jul 18, 12:56 PM

The patch looks good to me.

I’m going to try it with DRM drivers from Linux 6.14 (and define PAGE_IS_LKPI_PAGE) and will report.

I made an inline comment in linux_free_page(): there is a refcounting issue vs. the call to lkpi_page_unset_vm_page().

Beside this issue, I also made the following change to the i915 DRM driver which manipulates vm_page_t directly:

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 2d94542f4d..58f64e9c0e 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -171,14 +171,21 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
 		if (vm_object_set_memattr(vmobj, VM_MEMATTR_WRITE_COMBINING) != KERN_SUCCESS) {
 #if __FreeBSD_version >= 1500038
 			struct pctrie_iter pages;
-			vm_page_t page;
+			struct vm_page *m;
 
 			vm_page_iter_init(&pages, vmobj);
-			VM_RADIX_FORALL(page, &pages)
+			VM_RADIX_FORALL(m, &pages)
 #else
-			for (vm_page_t page = vm_page_find_least(vmobj, 0); page != NULL; page = vm_page_next(page))
+			for (vm_page_t m = vm_page_find_least(vmobj, 0); m != NULL; m = vm_page_next(m))
 #endif
-				pmap_page_set_memattr(page, VM_MEMATTR_WRITE_COMBINING);
+			{
+#ifdef PAGE_IS_LKPI_PAGE
+				struct page *page;
+				page = lkpi_vm_page_to_page(m);
+				lkpi_page_set_vm_page(page, m);
+#endif
+				pmap_page_set_memattr(m, VM_MEMATTR_WRITE_COMBINING);
+			}
 		}
 		VM_OBJECT_WUNLOCK(vmobj);
 	}

Do you think it's fine?

sys/compat/linuxkpi/common/src/linux_page.c
251–254	Here, the page may still be referenced after the call to `vm_page_unwire()`. This triggers a panic in the both the admgpu and i915 DRM drivers. I "fixed" it locally by changing the code to: vm_page_unwire(pgo, PQ_ACTIVE); #ifdef PAGE_IS_LKPI_PAGE if (!vm_page_wired(pgo)) lkpi_page_unset_vm_page(page + x); #endif With this in place, the i915 driver works (I'm writing this using it). That said, the unwiring and the unset are not atomic. I'm not sure how to fix this, I will think about it.

Oops, I forgot to mark the review as needing changes.

Doing it now with this comment, written using the amdgpu DRM driver this time, so it works too.

This revision now requires changes to proceed.Sat, Jul 25, 4:43 PM

In D58320#1341094, @dumbbell wrote:

I made an inline comment in linux_free_page(): there is a refcounting issue vs. the call to lkpi_page_unset_vm_page().

Beside this issue, I also made the following change to the i915 DRM driver which manipulates vm_page_t directly:

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 2d94542f4d..58f64e9c0e 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -171,14 +171,21 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
 		if (vm_object_set_memattr(vmobj, VM_MEMATTR_WRITE_COMBINING) != KERN_SUCCESS) {
 #if __FreeBSD_version >= 1500038
 			struct pctrie_iter pages;
-			vm_page_t page;
+			struct vm_page *m;
 
 			vm_page_iter_init(&pages, vmobj);
-			VM_RADIX_FORALL(page, &pages)
+			VM_RADIX_FORALL(m, &pages)
 #else
-			for (vm_page_t page = vm_page_find_least(vmobj, 0); page != NULL; page = vm_page_next(page))
+			for (vm_page_t m = vm_page_find_least(vmobj, 0); m != NULL; m = vm_page_next(m))
 #endif
-				pmap_page_set_memattr(page, VM_MEMATTR_WRITE_COMBINING);
+			{
+#ifdef PAGE_IS_LKPI_PAGE
+				struct page *page;
+				page = lkpi_vm_page_to_page(m);
+				lkpi_page_set_vm_page(page, m);
+#endif
+				pmap_page_set_memattr(m, VM_MEMATTR_WRITE_COMBINING);
+			}
 		}
 		VM_OBJECT_WUNLOCK(vmobj);
 	}

Do you think it's fine?

I'll have a look; I am about to run out the door; this is 6.14? So there's a new case that we didn't know about during the time we fixed master (before/after 6.12?) or had I missed this before?

this is 6.14? So there's a new case that we didn't know about during the time we fixed master (before/after 6.12?) or had I missed this before?

This is my 6.15 work-in-progress, https://github.com/dumbbell/drm-kmod/tree/update-to-linux-6.15.

There are several changes it TTM in 6.15 that would be way simpler to adapt to FreeBSD if struct page was in :-)

In D58320#1341094, @dumbbell wrote:

I made an inline comment in linux_free_page(): there is a refcounting issue vs. the call to lkpi_page_unset_vm_page().

Beside this issue, I also made the following change to the i915 DRM driver which manipulates vm_page_t directly:

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
index 2d94542f4d..58f64e9c0e 100644
--- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
+++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
@@ -171,14 +171,21 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
 		if (vm_object_set_memattr(vmobj, VM_MEMATTR_WRITE_COMBINING) != KERN_SUCCESS) {
 #if __FreeBSD_version >= 1500038
 			struct pctrie_iter pages;
-			vm_page_t page;
+			struct vm_page *m;
 
 			vm_page_iter_init(&pages, vmobj);
-			VM_RADIX_FORALL(page, &pages)
+			VM_RADIX_FORALL(m, &pages)
 #else
-			for (vm_page_t page = vm_page_find_least(vmobj, 0); page != NULL; page = vm_page_next(page))
+			for (vm_page_t m = vm_page_find_least(vmobj, 0); m != NULL; m = vm_page_next(m))
 #endif
-				pmap_page_set_memattr(page, VM_MEMATTR_WRITE_COMBINING);
+			{
+#ifdef PAGE_IS_LKPI_PAGE
+				struct page *page;
+				page = lkpi_vm_page_to_page(m);
+				lkpi_page_set_vm_page(page, m);
+#endif
+				pmap_page_set_memattr(m, VM_MEMATTR_WRITE_COMBINING);
+			}
 		}
 		VM_OBJECT_WUNLOCK(vmobj);
 	}

Do you think it's fine?

Yes. I would leave a blank line after struct page *page; for style but that's it.

Update linux_free_pages() as suggested by @dumbbell.

Note: I am still confused as to why we do all this checking manually there instead of simply call vm_page_unwire() of which we seem to duplicate the logic?

Harbormaster completed remote builds in B75138: Diff 182714.Sat, Jul 25, 9:06 PM

bz marked an inline comment as done.Sat, Jul 25, 9:06 PM

In D58320#1341246, @bz wrote:

Note: I am still confused as to why we do all this checking manually there instead of simply call vm_page_unwire() of which we seem to duplicate the logic?

I don’t remember the details, but I think I get a panic if I don’t put this logic. You are right that similar thing happens in vm_page_unwire(). Perhaps this duplication is now irrelevant.

dumbbell accepted this revision.Sun, Jul 26, 10:45 AM

This revision is now accepted and ready to land.Sun, Jul 26, 10:45 AM

dsl added a subscriber: dsl.Mon, Jul 27, 5:31 AM

bz removed a parent revision: D58299: LinuxKPI: page.h: resort lines.Mon, Jul 27, 8:44 PM

bz added a child revision: D58479: mt76: mt7921: update man page.Mon, Jul 27, 10:50 PM

bz added a child revision: D58498: LinuxKPI: turn on PAGE_IS_LKPI_PAGE.Mon, Jul 27, 11:52 PM

bz removed a child revision: D58479: mt76: mt7921: update man page.

bz added reviewers: markj, kib.Tue, Jul 28, 12:00 AM

kib added inline comments.Tue, Jul 28, 1:42 AM

sys/compat/linuxkpi/common/src/linux_page.c
994	Our vm_page_array[]is not static (in some sense). There is vm_phys_fictitious_reg_range(). So I am not sure how this would work.

bz added inline comments.Wed, Jul 29, 12:49 AM

sys/compat/linuxkpi/common/src/linux_page.c
994	Is there a good description of those? What they are, what they are used for, when they are used?

kib added inline comments.Fri, Jul 31, 1:04 PM

sys/compat/linuxkpi/common/src/linux_page.c
994	The fictitious pages are used when we need to provide struct vm_page for some regions of the physical address space which are not covered by regular pages from vm_page_array[]. This is needed to make e.g. pagers work, or any other consumer that requires PHYS_TO_VM_PAGE() to work, for the physical address. Examples are the DRM apertures, this is where the idea to register fictitious ranges come from. Currently it is also used at least in Xen, by UEFI RT services driver, etc, where there are special regions of physical address space and we need PHYS_TO_VM_PAGE(). So if your 'proxy' linux struct page array does not get extended on the registration, I suspect it would at least break DRM.