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
