Differential D48743
linuxkpi: Add `folio` and `folio_batch` APIs Authored by dumbbell on Jan 31 2025, 2:08 PM. Tags None Referenced Files
Details
They are used by the i915 DRM driver in Linux 6.6 (although this change was only backported with Linux 6.7 DRM drivers...). struct folio simply wraps struct page for now. struct folio_batch is the same as struct pagevec but it works with struct folio instead of struct page directly. This is part of the update of DRM drivers to Linux 6.7. Sponsored by: The FreeBSD Foundation
Diff Detail
Event TimelineComment Actions Add diff context. No other changes. Manu told me the same, so while I’m uploading patches to address bz feedback, I’m updating existing patch with their context.
Comment Actions I spent some time to review the folio code and the existing page allocation code (alloc_pages(), __free_pages(), put_page(), release_pages(), ...). Pages from alloc_pages() are not attached to a VM object (unmanaged). Our code assumes that a page allocated this way will be freed using __free_pages() which takes care of the unwiring and if 0, caliing vm_page_free(). put_page() and release_pages() assume that the page was allocated by something else and pages are managed and that something else will take care of the page free. They don’t do anything special when the wiring count reaches zero. Now from folio_batch_release(), I wonder if I should check the VPO_MANAGED flag to determine if the page should simply be unwired (same as put_page()) or if it should call __free_pages(). In other words, I wonder if this flag can be used to unify the put_page(), release_pages() and folio_batch_release() functions. FYI, on Linux, put_page() calls folio_put() (which I will add) and folio_batch_release() calls release_pages() (which accept a struct page ** or a struct folio ** through an anonymous union and assumes that a page can be casted to a folio). When the refcount reaches zero, both code paths call the same code to free pages. Comment Actions I reworked the patch to unify the put_page() and __free_pages() code paths. Now, all put/freed pages are handled by __free_pages(). release_pages() accepts either a list of struct page or a list of struct folio, like Linux. Comment Actions I would be tempted to break out the move of release_pages() w/o modifications into an earlier commit. Likewise depending on outcome of the review the adjustments for put_page etc. (but that's partially beyond me; I hope markj or kib can help). Then the remaining bits a pure folio.
Comment Actions We should probably have a brief description of the folio api. It was introduced in Linux in: commit 7b230db3b8d373219f88a3d25c8fbbf12cc7f233
Author: Matthew Wilcox (Oracle) <willy@infradead.org>
Date: Sun Dec 6 22:22:48 2020 -0500
mm: Introduce struct folio
A struct folio is a new abstraction to replace the venerable struct page.
A function which takes a struct folio argument declares that it will
operate on the entire (possibly compound) page, not just PAGE_SIZE bytes.
In return, the caller guarantees that the pointer it is passing does
not point to a tail page. No change to generated code.
Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Acked-by: Jeff Layton <jlayton@kernel.org>
Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
Reviewed-by: William Kucharski <william.kucharski@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: David Howells <dhowells@redhat.com>
Acked-by: Mike Rapoport <rppt@linux.ibm.com>
Comment Actions I got that same panic from a different location: panic: vm_page_assert_xbusied: page 0xfffffe0000135f80 not exclusive busy @ /home/dumbbell/Documents/Dev/freebsd/src/sys/vm/vm_page.c:1696 Here is the stacktrace: #3 0xffffffff80b586e7 in vpanic (fmt=0xffffffff811c8a46 "vm_page_assert_xbusied: page %p not exclusive busy @ %s:%d", ap=ap@entry=0xfffffe0109b266a0) at /home/dumbbell/Documents/Dev/freebsd/src/sys/kern/kern_shutdown.c:979
#4 0xffffffff80b58513 in panic (fmt=<unavailable>) at /home/dumbbell/Documents/Dev/freebsd/src/sys/kern/kern_shutdown.c:892
#5 0xffffffff80f2edc2 in vm_page_free_object_prep (m=m@entry=0xfffffe0000135f80) at /home/dumbbell/Documents/Dev/freebsd/src/sys/vm/vm_page.c:1696
#6 0xffffffff80f3763f in vm_page_free_prep (m=m@entry=0xfffffe0000135f80) at /home/dumbbell/Documents/Dev/freebsd/src/sys/vm/vm_page.c:4188
#7 0xffffffff80f2e5a2 in vm_page_free_toq (m=<unavailable>, m@entry=0xfffffe0000135f80) at /home/dumbbell/Documents/Dev/freebsd/src/sys/vm/vm_page.c:4254
#8 0xffffffff80f2e58b in vm_page_free (m=<unavailable>, m@entry=0xfffffe0000135f80) at /home/dumbbell/Documents/Dev/freebsd/src/sys/vm/vm_page.c:1383
#9 0xffffffff80e06e9d in linux_free_pages (vm_page=vm_page@entry=0xfffffe0000135f80, order=order@entry=0) at /home/dumbbell/Documents/Dev/freebsd/src/sys/compat/linuxkpi/common/src/linux_page.c:172
#10 0xffffffff85390b57 in __free_page (vm_page=0xfffffe0000135f80) at /home/dumbbell/Documents/Dev/freebsd/drm-kmod/../src/sys/compat/linuxkpi/common/include/linux/gfp.h:128
#11 put_page (vm_page=0xfffffe0000135f80) at /home/dumbbell/Documents/Dev/freebsd/drm-kmod/../src/sys/compat/linuxkpi/common/include/linux/mm.h:290
#12 i915_gem_object_create_shmem_from_data (dev_priv=<optimized out>, data=0xffffffff85182000 <_binary_i915_adlp_guc_70_bin_start>, size=337472)
at /home/dumbbell/Documents/Dev/freebsd/drm-kmod/drivers/gpu/drm/i915/gem/i915_gem_shmem.c:778
#13 0xffffffff8537c067 in intel_uc_fw_fetch (uc_fw=0xfffffe01939fe2e0) at /home/dumbbell/Documents/Dev/freebsd/drm-kmod/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c:975
#14 0xffffffff8537ac6a in __uc_fetch_firmwares (uc=0xfffffe01939fe028, uc@entry=<error reading variable: value is not available>) at /home/dumbbell/Documents/Dev/freebsd/drm-kmod/drivers/gpu/drm/i915/gt/uc/intel_uc.c:291
#15 0xffffffff85207d8e in intel_uc_fetch_firmwares (uc=<unavailable>) at /home/dumbbell/Documents/Dev/freebsd/drm-kmod/drivers/gpu/drm/i915/gt/uc/intel_uc.h:112
#16 i915_gem_init (dev_priv=dev_priv@entry=0xfffffe01939fa000) at /home/dumbbell/Documents/Dev/freebsd/drm-kmod/drivers/gpu/drm/i915/i915_gem.c:1173
#17 0xffffffff85203c0c in i915_driver_probe (pdev=0xfffff8000bcacc00, ent=<optimized out>) at /home/dumbbell/Documents/Dev/freebsd/drm-kmod/drivers/gpu/drm/i915/i915_driver.c:801
#18 0xffffffff80e08a10 in linux_pci_attach_device (dev=<unavailable>, dev@entry=<error reading variable: value is not available>, pdrv=0xffffffff8541b160 <i915_pci_driver>, pdrv@entry=<error reading variable: value is not available>,
id=0xffffffff853f0a70 <pciidlist+9120>, id@entry=<error reading variable: value is not available>, pdev=0xfffff8000bcacc00, pdev@entry=<error reading variable: value is not available>)
at /home/dumbbell/Documents/Dev/freebsd/src/sys/compat/linuxkpi/common/src/linux_pci.c:553
#19 0xffffffff80b9603b in DEVICE_ATTACH (dev=0xfffff80001c86100) at ./device_if.h:195Here is the struct vm_page that triggered the panic: (kgdb) f 10
#10 0xffffffff85390b57 in __free_page (vm_page=0xfffffe0000135f80) at /home/dumbbell/Documents/Dev/freebsd/drm-kmod/../src/sys/compat/linuxkpi/common/include/linux/gfp.h:128
128 linux_free_pages(page, 0);
(kgdb) p *vm_page
$3 = {plinks = {q = {tqe_next = 0xffffffffffffffff, tqe_prev = 0xffffffffffffffff}, s = {ss = {sle_next = 0xffffffffffffffff}}, memguard = {p = 18446744073709551615, v = 18446744073709551615}, uma = {slab = 0xffffffffffffffff,
zone = 0xffffffffffffffff}}, listq = {tqe_next = 0x0, tqe_prev = 0xfffff80016680db0}, object = 0xfffff80016680d68, pindex = 0, phys_addr = 50069504, md = {pv_list = {tqh_first = 0x0, tqh_last = 0xfffffe0000135fb8},
pv_gen = -1331646228, pat_mode = 6}, ref_count = 2147483648, busy_lock = 1, a = {{flags = 0, queue = 255 '\377', act_count = 0 '\000'}, _bits = 16711680}, order = 13 '\r', pool = 1 '\001', flags = 1 '\001', oflags = 0 '\000',
psind = 0 '\000', segind = 2 '\002', valid = 255 '\377', dirty = 0 '\000'}Compared to my description of the code path used to allocate the page in D49813, this one was allocated at: The call to put_page() a few lines after is the one causing the panic: Comment Actions Refresh patch after a change to D49810 which this patch depends on; no changes to the patch itself. Comment Actions @bz, @markj or @kib: Could you please give this patch a another review? Other reviews created from the original patch were accepted separately. @bz and @emaste: Both of you have slightly different opinion on the grouping of get/put page/folio. I don’t have a strong opinion on this. I went with @bz proposal before you posted your comment, @emaste.
Comment Actions Don't you need to update the linuxkpi.ko Makefile for the new source file?
Comment Actions Address feedback from @kib:
Comment Actions Ignoring the placing of the CTASSERT I am fine. You can move it or not before committing. Will be up to you.
Comment Actions
I can see arguments for having it in either place; the header feels more natural to me, but it's really the call to __free_page that requires them to be equivalent and it saves a little bit of CPU time to avoid evaluating the CTASSERT in each consumer of that header. If this is violated the build is going to stop either way.
| |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||