[PATCH] mm/memory.c: avoid repeated set_page_dirty in fault_dirty_shared_page

[PATCH] mm/memory.c: avoid repeated set_page_dirty in fault_dirty_shared_page

[Li Xinhai]

When vm_ops->page_mkwrite is defined, and called from wp_page_shared and
do_shared_fault, the set_page_dirty must already called by page_mkwrite.
Then in fault_dirty_shared_page, avoid this repeated call.

Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
Cc: Jan Kara <jack@suse.cz>
---
 mm/memory.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 606da18..34a83d7 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2300,10 +2300,11 @@ static vm_fault_t fault_dirty_shared_page(struct vm_fault *vmf)
 	struct vm_area_struct *vma = vmf->vma;
 	struct address_space *mapping;
 	struct page *page = vmf->page;
-	bool dirtied;
+	bool dirtied = false;
 	bool page_mkwrite = vma->vm_ops && vma->vm_ops->page_mkwrite;
 
-	dirtied = set_page_dirty(page);
+	if(!page_mkwrite)
+		dirtied = set_page_dirty(page);
 	VM_BUG_ON_PAGE(PageAnon(page), page);
 	/*
 	 * Take a local copy of the address_space - page.mapping may be zeroed
@@ -3645,7 +3646,7 @@ static vm_fault_t do_shared_fault(struct vm_fault *vmf)
 	 * Check if the backing address space wants to know that the page is
 	 * about to become writable
 	 */
-	if (vma->vm_ops->page_mkwrite) {
+	if (vma->vm_ops && vma->vm_ops->page_mkwrite) {
 		unlock_page(vmf->page);
 		tmp = do_page_mkwrite(vmf);
 		if (unlikely(!tmp ||
-- 
1.8.3.1

Re: [PATCH] mm/memory.c: avoid repeated set_page_dirty in fault_dirty_shared_page

[David Hildenbrand]

On 12.12.19 16:21, Li Xinhai wrote:
> When vm_ops->page_mkwrite is defined, and called from wp_page_shared and
> do_shared_fault, the set_page_dirty must already called by page_mkwrite.
> Then in fault_dirty_shared_page, avoid this repeated call.
> 
> Signed-off-by: Li Xinhai <lixinhai.lxh@gmail.com>
> Cc: Jan Kara <jack@suse.cz>
> ---
>  mm/memory.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 606da18..34a83d7 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -2300,10 +2300,11 @@ static vm_fault_t fault_dirty_shared_page(struct vm_fault *vmf)
>  	struct vm_area_struct *vma = vmf->vma;
>  	struct address_space *mapping;
>  	struct page *page = vmf->page;
> -	bool dirtied;
> +	bool dirtied = false;
>  	bool page_mkwrite = vma->vm_ops && vma->vm_ops->page_mkwrite;
>  
> -	dirtied = set_page_dirty(page);
> +	if(!page_mkwrite)
> +		dirtied = set_page_dirty(page);
>  	VM_BUG_ON_PAGE(PageAnon(page), page);
>  	/*
>  	 * Take a local copy of the address_space - page.mapping may be zeroed
> @@ -3645,7 +3646,7 @@ static vm_fault_t do_shared_fault(struct vm_fault *vmf)
>  	 * Check if the backing address space wants to know that the page is
>  	 * about to become writable
>  	 */
> -	if (vma->vm_ops->page_mkwrite) {
> +	if (vma->vm_ops && vma->vm_ops->page_mkwrite) {
>  		unlock_page(vmf->page);
>  		tmp = do_page_mkwrite(vmf);
>  		if (unlikely(!tmp ||
> 

This hunk looks like an unrelated change to me.

-- 
Thanks,

David / dhildenb

Re: [PATCH] mm/memory.c: avoid repeated set_page_dirty in fault_dirty_shared_page

[Kirill A. Shutemov]

On Thu, Dec 12, 2019 at 11:21:18PM +0800, Li Xinhai wrote:
> When vm_ops->page_mkwrite is defined, and called from wp_page_shared and
> do_shared_fault, the set_page_dirty must already called by page_mkwrite.

Must? Do all ->page_mkwrite implementation do this?

> @@ -3645,7 +3646,7 @@ static vm_fault_t do_shared_fault(struct vm_fault *vmf)
>  	 * Check if the backing address space wants to know that the page is
>  	 * about to become writable
>  	 */
> -	if (vma->vm_ops->page_mkwrite) {
> +	if (vma->vm_ops && vma->vm_ops->page_mkwrite) {

vma->vm_ops is always non-NULL here.


-- 
 Kirill A. Shutemov

Re: [PATCH] mm/memory.c: avoid repeated set_page_dirty in fault_dirty_shared_page

[lixinhai.lxh]

On 2019-12-13 at 00:55 Kirill A. Shutemov wrote:
>On Thu, Dec 12, 2019 at 11:21:18PM +0800, Li Xinhai wrote:
>> When vm_ops->page_mkwrite is defined, and called from wp_page_shared and
>> do_shared_fault, the set_page_dirty must already called by page_mkwrite.
>
>Must? Do all ->page_mkwrite implementation do this?

My understanding is that set_page_dirty need be called before PTE is set to allow
writing. If not in this sequence, other thread will see a writable PTE and dirty the page
before current thread set_page_dirty. 
In ->page_mkwrite, FS can decide if set_page_dirty should be called or not. I checked
a few FS, ext4/xfs/btrsfs/ceph and generic filemap_page_mkwrite, they called it. 
If FS provide ->page_mkwrite and decide don't call set_page_dirty, why
fault_dirty_shared_page call this function unconditionally? or, I missed something?

In case no ->page_mkwrite provided, call set_page_dirty looks reasonable for default
action.

>> @@ -3645,7 +3646,7 @@ static vm_fault_t do_shared_fault(struct vm_fault *vmf)
>>  * Check if the backing address space wants to know that the page is
>>  * about to become writable
>>  */
>> -	if (vma->vm_ops->page_mkwrite) {
>> +	if (vma->vm_ops && vma->vm_ops->page_mkwrite) {
>
>vma->vm_ops is always non-NULL here.
yes, thanks point out.

>
>--
> Kirill A. Shutemov

Re: [PATCH] mm/memory.c: avoid repeated set_page_dirty in fault_dirty_shared_page

[Jan Kara]

On Fri 13-12-19 15:28:32, lixinhai.lxh@gmail.com wrote:
> On 2019-12-13 at 00:55 Kirill A. Shutemov wrote:
> >On Thu, Dec 12, 2019 at 11:21:18PM +0800, Li Xinhai wrote:
> >> When vm_ops->page_mkwrite is defined, and called from wp_page_shared and
> >> do_shared_fault, the set_page_dirty must already called by page_mkwrite.
> >
> >Must? Do all ->page_mkwrite implementation do this?
> 
> My understanding is that set_page_dirty need be called before PTE is set
> to allow writing. If not in this sequence, other thread will see a
> writable PTE and dirty the page before current thread set_page_dirty. 

Yes, filesystems effectively do rely on this.

> In ->page_mkwrite, FS can decide if set_page_dirty should be called or
> not. I checked a few FS, ext4/xfs/btrsfs/ceph and generic
> filemap_page_mkwrite, they called it.  If FS provide ->page_mkwrite and
> decide don't call set_page_dirty, why fault_dirty_shared_page call this
> function unconditionally? or, I missed something?

Well, generally the responsibility for dirtying the page has been on the
generic MM code (i.e., fault_dirty_shared_page()). Now you're right that
lots of filesystems will end up dirtying the page because they are reusing
generic helpers for handling ->page_mkwrite() and that happens to dirty the
page. But that is mostly a coincidence and not guarantee. So to safely
remove page dirtying from fault_dirty_shared_page(), you'd need to audit
*all* ->page_mkwrite() implementations, make sure they all dirty the page,
and then document this requirement somewhere. Overall I don't think the
effort is really worth it since redirtying already dirty page is very
cheap.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR