Re: [PATCH v2.1 04/26] mm: allow VM_FAULT_RETRY for multiple times

From: Peter Xu <peterx@redhat.com>
To: Jerome Glisse <jglisse@redhat.com>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	David Hildenbrand <david@redhat.com>,
	Hugh Dickins <hughd@google.com>, Maya Gokhale <gokhale2@llnl.gov>,
	Pavel Emelyanov <xemul@virtuozzo.com>,
	Martin Cracauer <cracauer@cons.org>, Shaohua Li <shli@fb.com>,
	Marty McFadden <mcfadden8@llnl.gov>,
	Andrea Arcangeli <aarcange@redhat.com>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	Denis Plotnikov <dplotnikov@virtuozzo.com>,
	Mike Rapoport <rppt@linux.vnet.ibm.com>,
	Mel Gorman <mgorman@suse.de>,
	"Kirill A . Shutemov" <kirill@shutemov.name>,
	"Dr . David Alan Gilbert" <dgilbert@redhat.com>
Subject: Re: [PATCH v2.1 04/26] mm: allow VM_FAULT_RETRY for multiple times
Date: Mon, 25 Feb 2019 14:19:09 +0800	[thread overview]
Message-ID: <20190225061835.GA28121@xz-x1> (raw)
In-Reply-To: <20190222151101.GA7783@redhat.com>

On Fri, Feb 22, 2019 at 10:11:58AM -0500, Jerome Glisse wrote:
> On Fri, Feb 22, 2019 at 12:25:44PM +0800, Peter Xu wrote:
> > On Thu, Feb 21, 2019 at 10:53:11AM -0500, Jerome Glisse wrote:
> > > On Thu, Feb 21, 2019 at 04:56:56PM +0800, Peter Xu wrote:
> > > > The idea comes from a discussion between Linus and Andrea [1].
> 
> [...]
> 
> > > > diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> > > > index 248ff0a28ecd..d842c3e02a50 100644
> > > > --- a/arch/x86/mm/fault.c
> > > > +++ b/arch/x86/mm/fault.c
> > > > @@ -1483,9 +1483,7 @@ void do_user_addr_fault(struct pt_regs *regs,
> > > >  	if (unlikely(fault & VM_FAULT_RETRY)) {
> > > >  		bool is_user = flags & FAULT_FLAG_USER;
> > > >  
> > > > -		/* Retry at most once */
> > > >  		if (flags & FAULT_FLAG_ALLOW_RETRY) {
> > > > -			flags &= ~FAULT_FLAG_ALLOW_RETRY;
> > > >  			flags |= FAULT_FLAG_TRIED;
> > > >  			if (is_user && signal_pending(tsk))
> > > >  				return;
> > > 
> > > So here you have a change in behavior, it can retry indefinitly for as
> > > long as they are no signal. Don't you want so test for FAULT_FLAG_TRIED ?
> > 
> > These first five patches do want to allow the page fault to retry as
> > much as needed.  "indefinitely" seems to be a scary word, but IMHO
> > this is fine for page faults since otherwise we'll simply crash the
> > program or even crash the system depending on the fault context, so it
> > seems to be nowhere worse.
> > 
> > For userspace programs, if anything really really go wrong (so far I
> > still cannot think a valid scenario in a bug-free system, but just
> > assuming...) and it loops indefinitely, IMHO it'll just hang the buggy
> > process itself rather than coredump, and the admin can simply kill the
> > process to retake the resources since we'll still detect signals.
> > 
> > Or did I misunderstood the question?
> 
> No i think you are right, it is fine to keep retrying while they are
> no signal maybe just add a comment that says so in so many words :)
> So people do not see that as a potential issue.

Sure thing.  I don't know whether commenting this on all the
architectures is good...  I'll try to add some comments above
FAULT_FLAG_* deinitions to explain this.

Thanks!

-- 
Peter Xu