Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 14 Apr 2023 19:11:43 +0100
From:   Matthew Wilcox <willy@infradead.org>
To:     Suren Baghdasaryan <surenb@google.com>
Cc:     akpm@linux-foundation.org, hannes@cmpxchg.org, mhocko@suse.com,
        josef@toxicpanda.com, jack@suse.cz, ldufour@linux.ibm.com,
        laurent.dufour@fr.ibm.com, michel@lespinasse.org,
        liam.howlett@oracle.com, jglisse@google.com, vbabka@suse.cz,
        minchan@google.com, dave@stgolabs.net, punit.agrawal@bytedance.com,
        lstoakes@gmail.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, kernel-team@android.com
Subject: Re: [PATCH 1/1] mm: do not increment pgfault stats when page fault
 handler retries
Message-ID: <ZDmXX82ly2OhfJ1D@casper.infradead.org>
References: <20230414175444.1837474-1-surenb@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230414175444.1837474-1-surenb@google.com>
Precedence: bulk

On Fri, Apr 14, 2023 at 10:54:44AM -0700, Suren Baghdasaryan wrote:
> If the page fault handler requests a retry, we will count the fault
> multiple times.  This is a relatively harmless problem as the retry paths
> are not often requested, and the only user-visible problem is that the
> fault counter will be slightly higher than it should be.  Nevertheless,
> userspace only took one fault, and should not see the fact that the
> kernel had to retry the fault multiple times.
> 
> Fixes: 6b4c9f446981 ("filemap: drop the mmap_sem for all blocking operations")

I know I suggested this fixes line, but I think it's actually been
here much longer, perhaps since

Fixes: d065bd810b6d ("mm: retry page fault when blocking on disk transfer")

Michel, what do you think?

> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
> Patch applies cleanly over linux-next and mm-unstable
> 
>  mm/memory.c | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 1c5b231fe6e3..d88f370eacd1 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -5212,17 +5212,16 @@ vm_fault_t handle_mm_fault(struct vm_area_struct *vma, unsigned long address,
>  
>  	__set_current_state(TASK_RUNNING);
>  
> -	count_vm_event(PGFAULT);
> -	count_memcg_event_mm(vma->vm_mm, PGFAULT);
> -
>  	ret = sanitize_fault_flags(vma, &flags);
>  	if (ret)
> -		return ret;
> +		goto out;
>  
>  	if (!arch_vma_access_permitted(vma, flags & FAULT_FLAG_WRITE,
>  					    flags & FAULT_FLAG_INSTRUCTION,
> -					    flags & FAULT_FLAG_REMOTE))
> -		return VM_FAULT_SIGSEGV;
> +					    flags & FAULT_FLAG_REMOTE)) {
> +		ret = VM_FAULT_SIGSEGV;
> +		goto out;
> +	}
>  
>  	/*
>  	 * Enable the memcg OOM handling for faults triggered in user
> @@ -5253,6 +5252,11 @@ vm_fault_t handle_mm_fault(struct vm_area_struct *vma, unsigned long address,
>  	}
>  
>  	mm_account_fault(regs, address, flags, ret);
> +out:
> +	if (!(ret & VM_FAULT_RETRY)) {
> +		count_vm_event(PGFAULT);
> +		count_memcg_event_mm(vma->vm_mm, PGFAULT);
> +	}
>  
>  	return ret;
>  }
> -- 
> 2.40.0.634.g4ca3ef3211-goog
>