diff mbox series

[1/4] mm/memory: convert do_page_mkwrite() to use folios

Message ID 20230703055850.227169-1-sidhartha.kumar@oracle.com
State New
Headers
Series [1/4] mm/memory: convert do_page_mkwrite() to use folios |

Commit Message

Sidhartha Kumar July 3, 2023, 5:58 a.m. UTC 
  Saves one implicit call to compound_head();

Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
---
 mm/memory.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

zhangpeng (AS) July 3, 2023, 9:29 a.m. UTC | #1 
On 2023/7/3 13:58, Sidhartha Kumar wrote:

> Saves one implicit call to compound_head();
>
> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
> ---
>   mm/memory.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 21fab27272092..098fac2f5efc0 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -2932,7 +2932,7 @@ static gfp_t __get_fault_gfp_mask(struct vm_area_struct *vma)
>   static vm_fault_t do_page_mkwrite(struct vm_fault *vmf)
>   {
>   	vm_fault_t ret;
> -	struct page *page = vmf->page;
> +	struct folio *folio = page_folio(vmf->page);
>   	unsigned int old_flags = vmf->flags;
>   
>   	vmf->flags = FAULT_FLAG_WRITE|FAULT_FLAG_MKWRITE;
> @@ -2947,14 +2947,14 @@ static vm_fault_t do_page_mkwrite(struct vm_fault *vmf)
>   	if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))
>   		return ret;
>   	if (unlikely(!(ret & VM_FAULT_LOCKED))) {
> -		lock_page(page);
> -		if (!page->mapping) {
> -			unlock_page(page);
> +		folio_lock(folio);
> +		if (!folio_mapping(folio)) {

Could page->mapping be directly converted to folio->mapping?

Thanks,
Peng

> +			folio_unlock(folio);
>   			return 0; /* retry */
>   		}
>   		ret |= VM_FAULT_LOCKED;
>   	} else
> -		VM_BUG_ON_PAGE(!PageLocked(page), page);
> +		VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
>   	return ret;
>   }
>
Matthew Wilcox July 3, 2023, 11:25 p.m. UTC | #2 
On Sun, Jul 02, 2023 at 10:58:47PM -0700, Sidhartha Kumar wrote:
> @@ -2947,14 +2947,14 @@ static vm_fault_t do_page_mkwrite(struct vm_fault *vmf)
>  	if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))
>  		return ret;
>  	if (unlikely(!(ret & VM_FAULT_LOCKED))) {
> -		lock_page(page);
> -		if (!page->mapping) {
> -			unlock_page(page);
> +		folio_lock(folio);
> +		if (!folio_mapping(folio)) {

You don't need to call folio_mapping() here.  folio->mapping works
absolutely fine in this circumstance.  In fact, you may have broken a
driver with this change.  I can elaborate more, but I'm not quite in the
mood to do that right now.
Matthew Wilcox July 4, 2023, 7:35 p.m. UTC | #3 
On Sun, Jul 02, 2023 at 10:58:47PM -0700, Sidhartha Kumar wrote:
> @@ -2947,14 +2947,14 @@ static vm_fault_t do_page_mkwrite(struct vm_fault *vmf)
>  	if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))
>  		return ret;
>  	if (unlikely(!(ret & VM_FAULT_LOCKED))) {
> -		lock_page(page);
> -		if (!page->mapping) {
> -			unlock_page(page);
> +		folio_lock(folio);
> +		if (!folio_mapping(folio)) {
> +			folio_unlock(folio);

I promised to explain this better once I had time, and I have time now.

folio->mapping is used for a multitude of purposes, unfortunately.
Maybe some future work will reduce that, but for now, These Are The Rules.

If the folio is marked as being Slab, it's used for something else.
The folio does not belong to an address space (nor can it be mapped,
so we're not going to see it here, but sometimes we see it in other
contexts where we call folio_mapping()).

The bottom two bits are used as PAGE_MAPPING_FLAGS.  If they're both
0, this folio belongs to a file, and the rest of folio->mapping is a
pointer to a struct address_space.  Or they're both 0 because the whole
thing is NULL.  More on that below.  If the bottom two bits are 01b,
this is an anonymous folio, and folio->mapping is actually a pointer
to an anon_vma (which is not the same thing as an anon vma).  If the
bottom two bits are 10b, this is a Movable page (anon & file memory is
also movable, but this is different).  The folio->mapping points to a
struct movable_operations.  If the bottom two bits are 11b, this is a
KSM allocation, and folio->mapping points to a struct ksm_stable_node.

When we remove a folio from the page cache, we reset folio->mapping
to NULL.  We often remove folios from the page cache before their
refcount drops to zero (the common case is to look up the folio in the
page cache, which grabs a reference, remove the folio from the page
cache which decrements the refcount, then put the folio which might be
the last refcount).  So it's entirely possible to see a folio in this
function with a NULL mapping; that means it's been removed from the
file through a truncate or siimlar, and we need to fail the mkwrite.
Userspace is about to get a segfault.

If you find all of that confusing, well, I agree, and I'm trying to
simplify it.

So, with all that background, what's going on here?  Part of the
"modern" protocol for handling page faults is to lock the folio
in vm_ops->page_mkwrite.  But we still support (... why?) drivers
that haven't been updated.  They return 0 on success instead of
VM_FAULT_LOCKED.  So we take the lock for them, then check that the
folio wasn't truncated, and bail out if it looks like it was.

If we have a really old-school driver that has allocated a page,
mapped it to userspace, and set page->mapping to be, eg, Movable, by
calling folio_mapping() instead of folio->mapping, we'll end up seeing
NULL instead of a non-NULL value, mistakenly believe it to have been
truncated and enter an endless loop.

Am I being paranoid here?  Maybe!  Drivers should have been updated by
now.  The "modern" way was introduced in 2007 (commit d0217ac04ca6), so
it'd be nice to turn this into a WARN_ON_ONCE so drivers fix their code.
There are only ~30 implementations of page_mkwrite in the kernel, so it
might not take too long to check everything's OK.
diff mbox series

Patch

diff --git a/mm/memory.c b/mm/memory.c
index 21fab27272092..098fac2f5efc0 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2932,7 +2932,7 @@  static gfp_t __get_fault_gfp_mask(struct vm_area_struct *vma)
 static vm_fault_t do_page_mkwrite(struct vm_fault *vmf)
 {
 	vm_fault_t ret;
-	struct page *page = vmf->page;
+	struct folio *folio = page_folio(vmf->page);
 	unsigned int old_flags = vmf->flags;
 
 	vmf->flags = FAULT_FLAG_WRITE|FAULT_FLAG_MKWRITE;
@@ -2947,14 +2947,14 @@  static vm_fault_t do_page_mkwrite(struct vm_fault *vmf)
 	if (unlikely(ret & (VM_FAULT_ERROR | VM_FAULT_NOPAGE)))
 		return ret;
 	if (unlikely(!(ret & VM_FAULT_LOCKED))) {
-		lock_page(page);
-		if (!page->mapping) {
-			unlock_page(page);
+		folio_lock(folio);
+		if (!folio_mapping(folio)) {
+			folio_unlock(folio);
 			return 0; /* retry */
 		}
 		ret |= VM_FAULT_LOCKED;
 	} else
-		VM_BUG_ON_PAGE(!PageLocked(page), page);
+		VM_BUG_ON_FOLIO(!folio_test_locked(folio), folio);
 	return ret;
 }