mbox series

[v3,0/3] expose mapping wrprotect, fix fb_defio use

Message ID cover.1739029358.git.lorenzo.stoakes@oracle.com
Headers show
Series expose mapping wrprotect, fix fb_defio use | expand

Message

Lorenzo Stoakes Feb. 8, 2025, 3:52 p.m. UTC 
Right now the only means by which we can write-protect a range using the
reverse mapping is via folio_mkclean().

However this is not always the appropriate means of doing so, specifically
in the case of the framebuffer deferred I/O logic (fb_defio enabled by
CONFIG_FB_DEFERRED_IO). There, kernel pages are mapped read-only and
write-protect faults used to batch up I/O operations.

Each time the deferred work is done, folio_mkclean() is used to mark the
framebuffer page as having had I/O performed on it. However doing so
requires the kernel page (perhaps allocated via vmalloc()) to have its
page->mapping, index fields set so the rmap can find everything that maps
it in order to write-protect.

This is problematic as firstly, these fields should not be set for
kernel-allocated memory, and secondly these are not folios (it's not user
memory) and page->index, mapping fields are now deprecated and soon to be
removed.

The removal of these fields is imminent, rendering this series more urgent
than it might first appear.

The implementers cannot be blamed for having used this however, as there is
simply no other way of performing this operation correctly.

This series fixes this - we provide the mapping_wrprotect_range() function
to allow the reverse mapping to be used to look up mappings from the page
cache object (i.e. its address_space pointer) at a specific offset.

The fb_defio logic already stores this offset, and can simply be expanded
to keep track of the page cache object, so the change then becomes
straight-forward.

This series should have no functional change.

v3:
* Fix fb_err() usage.
* Do not add missing CONFIG_MMU dependency on CONFIG_FB_DEFERRED_IO -
  unfortunately this is simply not practical in this series, as this option is
  selected by other options and this propagates quite significantly, and it is
  not obvious that a dependency on CONFIG_MMU has been correctly tested/assured
  to be valid in all instances.
  Instead, simply use #ifdef CONFIG_MMU to avoid calling
  mapping_wrprotect_range() for nommu, which is equivalent behaviour to before
  this change.
  Ideally somebody will work through the fb/drm code and ensure anything that
  requires deferred I/O explicitly depends on CONFIG_MMU, or is ok with the
  deferred work being a no-op.

v2:
* Make CONFIG_FB_DEFERRED_IO depend on CONFIG_MMU (as it strictly relies upon
  it) to fix randconfig builds.
* Replace mapping_wrprotect_page() with mapping_wrprotect_range() accepting a
  PFN, as per Simona. While it is not suited for use with DMA addresses (as
  pointed out by hch), it is still worth doing so to make the function more
  flexible.
* Also update the mapping_wrprotect_range() signature to be more logical -
  with the pfn argument appearing before nr_pages.
* Use fb_err() for error output as per Thomas.
https://lore.kernel.org/all/cover.1738855154.git.lorenzo.stoakes@oracle.com/

non-RFC:
* Kajtar kindly smoke-tested the defio side of this change and confirmed
  that it appears to work correctly. I am therefore stripping the RFC and
  putting forward as a non-RFC series.
https://lore.kernel.org/all/cover.1738347308.git.lorenzo.stoakes@oracle.com/

RFC v2:
* Updated Jaya Kumar's email on cc - the MAINTAINERS section is apparently
  incorrect.
* Corrected rmap_walk_file() comment to refer to folios as per Matthew.
* Reference folio->mapping rather than folio_mapping(folio) in
  rmap_walk_file() as per Matthew.
* Reference folio->index rather than folio_pgoff(folio) in rmap_walk_file()
  as per Matthew.
* Renamed rmap_wrprotect_file_page() to mapping_wrprotect_page() as per
  Matthew.
* Fixed kerneldoc and moved to implementation as per Matthew.
* Updated mapping_wrprotect_page() to take a struct page pointer as per
  David.
* Removed folio lock when invoking mapping_wrprotect_page() in
  fb_deferred_io_work() as per Matthew.
* Removed compound_nr() in fb_deferred_io_work() as per Matthew.

RFC v1:
https://lore.kernel.org/all/1e452b5b65f15a9a5d0c2ed3f5f812fdd1367603.1736352361.git.lorenzo.stoakes@oracle.com/


Lorenzo Stoakes (3):
  mm: refactor rmap_walk_file() to separate out traversal logic
  mm: provide mapping_wrprotect_range() function
  fb_defio: do not use deprecated page->mapping, index fields

 drivers/video/fbdev/core/fb_defio.c |  43 +++-----
 include/linux/fb.h                  |   1 +
 include/linux/rmap.h                |   3 +
 mm/rmap.c                           | 153 +++++++++++++++++++++++-----
 4 files changed, 146 insertions(+), 54 deletions(-)

--
2.48.1

Comments

Lorenzo Stoakes Feb. 9, 2025, 6:36 a.m. UTC | #1 
Andrew - apologies for this, I managed to miss some unusued variable warnings in
this nommu workaround, once you take the v3 could you apply this trivial
fix-patch on top to resolve that?

I tested this locally to confirm it resolves the problem. Thanks!

----8<----