linuxkpi: Add `folio` and `folio_batch` APIs

[dumbbell]

Summary

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

[emaste]

(For next time please upload with context, e.g. -U999999)

[dumbbell]

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.

[bz]

sys/compat/linuxkpi/common/include/linux/gfp.h

137

Can we please move the alloc (and free) functions into C files so we can change them in the future without breaking the KBI?

sys/compat/linuxkpi/common/include/linux/mm.h

169

return ()

173

This indicates there is some kind of reference counting on struct folio? Are you sure you can always just free here? Hmm given there is no folio_get yet you may indeed still be able to get away with it... but more sophisticated use will likely require this one day.

451

return ()

457

return ()

463

return ()

475

return ()

sys/compat/linuxkpi/common/include/linux/mm_types.h

83

/*
 * text
 * more text
 */

84

Also __folio_batch_release() relies on this currently.

sys/compat/linuxkpi/common/include/linux/pagevec.h

93

return ()

99

return ()

106

return ()

113

This also has the assumption that struct folio is struct page.

sys/compat/linuxkpi/common/include/linux/shmem_fs.h

64

return ()

sys/compat/linuxkpi/common/include/linux/swap.h

63

XXX-BZ look this up as we should not do that anymore; there should be a page_* KPI for this? It'll break when we will have a real struct page and not just a typedef.

Can we try calling mark_page_accessed() for the page? That way we contain the problem to the one function for now?

[dumbbell]

Address @bz comments.

[dumbbell]

sys/compat/linuxkpi/common/include/linux/mm.h

173

You are right. I will rework the patch to fix it.

[dumbbell]

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.

What do you think, @bz and @kib?

[dumbbell]

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.

[bz]

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.

sys/compat/linuxkpi/common/include/linux/mm.h

292

I would put this before folio_get() so get_page and put_page are together.

296

Are we sure that that is the case in both instances in linux_free_pages(), especially the else path?

Wouldn't the original vm_page_unwire() do the right thing as well if I read the comment above vm_page_unwire_managed() correctly? @markj?

308

can be cast to struct page. But a native English speaker may know better.

sys/compat/linuxkpi/common/src/linux_page.c

167

This really ignores the else case I believe; but see comment in mm_types.h for put_page(). Someone like @markj who knows the VM bits more needs to have a look at this. get_page and put_page need to keep working for both the direct map case and the else case here.

[markj]

sys/compat/linuxkpi/common/src/linux_page.c

167

Why do we need to handle managed pages here? I'd assume that this function is a complement of linux_alloc_pages(), which returns unmanaged pages when PMAP_HAS_DMAP is true.

[emaste]

sys/compat/linuxkpi/common/include/linux/mm.h

308

yes, "can be cast to"

[kib]

sys/compat/linuxkpi/common/include/linux/mm_types.h

89

Then write a static_assert that checks that offsetof is zero?
And more, since struct page and struct folio seems to be mutually equivalent, assert that sizes are same?

sys/compat/linuxkpi/common/include/linux/pagevec.h

69

We usually do not add such delimiters. At best you could write a multi-line herald like

/*
 * folio-batch stuff
 */

[bz]

sys/compat/linuxkpi/common/include/linux/mm_types.h

89

I do not think we can assert that sizes are the same. It's just one is embedded in the other. It's the other way round on Linux by the way if I understand it correctly. But I like the CTASSERT idea for the offset!

sys/compat/linuxkpi/common/include/linux/pagevec.h

69

I do. Very old school. Sorry. Massively help if you need to delimit entire sections. No multi-line comment will ever do that. But I was wondering too how that appeared here ;-)

[dumbbell]

sys/compat/linuxkpi/common/include/linux/mm.h

296

I can read again the code in Linux because I don’t remember the details. I can write something about what I understand from the Linux code.

sys/compat/linuxkpi/common/src/linux_page.c

167

I will test again tomorrow without this but IIRC, I got a panic. Once I can reproduce, I will share details here and add a comment to the code.

On Linux, the "free page" code path is not the complement of alloc_pages() because the page may come from another "source".

[emaste]

We should probably have a brief description of the folio api. It was introduced in Linux in:

commit 7b230db3b8d373219f88a3d25c8fbbf12cc7f233
Author: Matthew Wilcox (Oracle) <[e-mail]>
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) <[e-mail]>
    Acked-by: Jeff Layton <[e-mail]>
    Acked-by: Kirill A. Shutemov <[e-mail]>
    Acked-by: Vlastimil Babka <[e-mail]>
    Reviewed-by: William Kucharski <[e-mail]>
    Reviewed-by: Christoph Hellwig <[e-mail]>
    Reviewed-by: David Howells <[e-mail]>
    Acked-by: Mike Rapoport <[e-mail]>

sys/compat/linuxkpi/common/include/linux/mm.h

292

Do we pair both get and both put functions, or both page and both folio functions? I don't think it matters too much.

[dumbbell]

I split this patch info multiple reviews:

• D49810
• D49811
• D49812
• D49813
• And this one for the final addition of struct folio.

[dumbbell]

Split patch info multiple reviews

[dumbbell]

sys/compat/linuxkpi/common/src/linux_page.c

167

I confirm that this if/else is required because pages come from other sources than alloc_pages(). Without it, the kernel panics with:

panic: vm_page_assert_xbusied: page 0xfffffe0000d8a338 not exclusive busy @ /home/dumbbell/Documents/Dev/freebsd/src/sys/vm/vm_page.c:1696

[markj]

sys/compat/linuxkpi/common/src/linux_page.c

167

Could you please show the page (e.g., p/x *(vm_page_t)0xfffffe0000d8a338 in kgdb) and the stack trace?

[dumbbell]

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:195

Here 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:
https://github.com/dumbbell/drm-kmod/blob/update-to-linux-6.8/drivers/gpu/drm/i915/gem/i915_gem_shmem.c#L765

The call to put_page() a few lines after is the one causing the panic:
https://github.com/dumbbell/drm-kmod/blob/update-to-linux-6.8/drivers/gpu/drm/i915/gem/i915_gem_shmem.c#L778

[dumbbell]

Refresh patch after a change to D49810 which this patch depends on; no changes to the patch itself.

[dumbbell]

Refresh again after a small mistake…

[dumbbell]

@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.

sys/compat/linuxkpi/common/src/linux_page.c

167

The rest of this discussion happened in D49813.

[kib]

Don't you need to update the linuxkpi.ko Makefile for the new source file?

sys/compat/linuxkpi/common/include/linux/pagevec.h

119

Is it useful to kassert that we do not overflow the folios[]?

[dumbbell]

Address feedback from @kib:

• Add KASSERT to make sure a folio batch doesn't grow beyond PAGEVEC_SIZE
• Add linux_folio.c to linuxkpi kernel module Makefile

[dumbbell]

Thank you @kib, both good points. I updated the patch accordingly.

[bz]

Ignoring the placing of the CTASSERT I am fine. You can move it or not before committing. Will be up to you.

sys/compat/linuxkpi/common/src/linux_page.c

183

i would have put that next to struct folio in the header given it's a compile time assert documenting the offset there.

[emaste]

placing of the CTASSERT

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.

[markj]

sys/compat/linuxkpi/common/include/linux/page-flags.h

37