| Message ID | 20190221085656.18529-1-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
On Thu, Feb 21, 2019 at 04:56:56PM +0800, Peter Xu wrote: > The idea comes from a discussion between Linus and Andrea [1]. > > Before this patch we only allow a page fault to retry once. We > achieved this by clearing the FAULT_FLAG_ALLOW_RETRY flag when doing > handle_mm_fault() the second time. This was majorly used to avoid > unexpected starvation of the system by looping over forever to handle > the page fault on a single page. However that should hardly happen, > and after all for each code path to return a VM_FAULT_RETRY we'll > first wait for a condition (during which time we should possibly yield > the cpu) to happen before VM_FAULT_RETRY is really returned. > > This patch removes the restriction by keeping the > FAULT_FLAG_ALLOW_RETRY flag when we receive VM_FAULT_RETRY. It means > that the page fault handler now can retry the page fault for multiple > times if necessary without the need to generate another page fault > event. Meanwhile we still keep the FAULT_FLAG_TRIED flag so page > fault handler can still identify whether a page fault is the first > attempt or not. > > Then we'll have these combinations of fault flags (only considering > ALLOW_RETRY flag and TRIED flag): > > - ALLOW_RETRY and !TRIED: this means the page fault allows to > retry, and this is the first try > > - ALLOW_RETRY and TRIED: this means the page fault allows to > retry, and this is not the first try > > - !ALLOW_RETRY and !TRIED: this means the page fault does not allow > to retry at all > > - !ALLOW_RETRY and TRIED: this is forbidden and should never be used > > In existing code we have multiple places that has taken special care > of the first condition above by checking against (fault_flags & > FAULT_FLAG_ALLOW_RETRY). This patch introduces a simple helper to > detect the first retry of a page fault by checking against > both (fault_flags & FAULT_FLAG_ALLOW_RETRY) and !(fault_flag & > FAULT_FLAG_TRIED) because now even the 2nd try will have the > ALLOW_RETRY set, then use that helper in all existing special paths. > One example is in __lock_page_or_retry(), now we'll drop the mmap_sem > only in the first attempt of page fault and we'll keep it in follow up > retries, so old locking behavior will be retained. > > This will be a nice enhancement for current code [2] at the same time > a supporting material for the future userfaultfd-writeprotect work, > since in that work there will always be an explicit userfault > writeprotect retry for protected pages, and if that cannot resolve the > page fault (e.g., when userfaultfd-writeprotect is used in conjunction > with swapped pages) then we'll possibly need a 3rd retry of the page > fault. It might also benefit other potential users who will have > similar requirement like userfault write-protection. > > GUP code is not touched yet and will be covered in follow up patch. > > Please read the thread below for more information. > > [1] https://lkml.org/lkml/2017/11/2/833 > [2] https://lkml.org/lkml/2018/12/30/64 I have few comments on this one. See below. > > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> > Suggested-by: Andrea Arcangeli <aarcange@redhat.com> > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > > arch/alpha/mm/fault.c | 2 +- > arch/arc/mm/fault.c | 1 - > arch/arm/mm/fault.c | 3 --- > arch/arm64/mm/fault.c | 5 ----- > arch/hexagon/mm/vm_fault.c | 1 - > arch/ia64/mm/fault.c | 1 - > arch/m68k/mm/fault.c | 3 --- > arch/microblaze/mm/fault.c | 1 - > arch/mips/mm/fault.c | 1 - > arch/nds32/mm/fault.c | 1 - > arch/nios2/mm/fault.c | 3 --- > arch/openrisc/mm/fault.c | 1 - > arch/parisc/mm/fault.c | 2 -- > arch/powerpc/mm/fault.c | 6 ------ > arch/riscv/mm/fault.c | 5 ----- > arch/s390/mm/fault.c | 5 +---- > arch/sh/mm/fault.c | 1 - > arch/sparc/mm/fault_32.c | 1 - > arch/sparc/mm/fault_64.c | 1 - > arch/um/kernel/trap.c | 1 - > arch/unicore32/mm/fault.c | 6 +----- > arch/x86/mm/fault.c | 2 -- > arch/xtensa/mm/fault.c | 1 - > drivers/gpu/drm/ttm/ttm_bo_vm.c | 12 +++++++++--- > include/linux/mm.h | 12 +++++++++++- > mm/filemap.c | 2 +- > mm/shmem.c | 2 +- > 27 files changed, 25 insertions(+), 57 deletions(-) > [...] > diff --git a/arch/parisc/mm/fault.c b/arch/parisc/mm/fault.c > index 29422eec329d..7d3e96a9a7ab 100644 > --- a/arch/parisc/mm/fault.c > +++ b/arch/parisc/mm/fault.c > @@ -327,8 +327,6 @@ void do_page_fault(struct pt_regs *regs, unsigned long code, > else > current->min_flt++; > if (fault & VM_FAULT_RETRY) { > - flags &= ~FAULT_FLAG_ALLOW_RETRY; Don't you need to also add: flags |= FAULT_FLAG_TRIED; Like other arch. [...] > 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 ? [...] > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 80bb6408fe73..4e11c9639f1b 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -341,11 +341,21 @@ extern pgprot_t protection_map[16]; > #define FAULT_FLAG_ALLOW_RETRY 0x04 /* Retry fault if blocking */ > #define FAULT_FLAG_RETRY_NOWAIT 0x08 /* Don't drop mmap_sem and wait when retrying */ > #define FAULT_FLAG_KILLABLE 0x10 /* The fault task is in SIGKILL killable region */ > -#define FAULT_FLAG_TRIED 0x20 /* Second try */ > +#define FAULT_FLAG_TRIED 0x20 /* We've tried once */ > #define FAULT_FLAG_USER 0x40 /* The fault originated in userspace */ > #define FAULT_FLAG_REMOTE 0x80 /* faulting for non current tsk/mm */ > #define FAULT_FLAG_INSTRUCTION 0x100 /* The fault was during an instruction fetch */ > > +/* > + * Returns true if the page fault allows retry and this is the first > + * attempt of the fault handling; false otherwise. > + */ You should add why it returns false if it is not the first try ie to avoid starvation. > +static inline bool fault_flag_allow_retry_first(unsigned int flags) > +{ > + return (flags & FAULT_FLAG_ALLOW_RETRY) && > + (!(flags & FAULT_FLAG_TRIED)); > +} > + > #define FAULT_FLAG_TRACE \ > { FAULT_FLAG_WRITE, "WRITE" }, \ > { FAULT_FLAG_MKWRITE, "MKWRITE" }, \ [...]
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]. > > > > Before this patch we only allow a page fault to retry once. We > > achieved this by clearing the FAULT_FLAG_ALLOW_RETRY flag when doing > > handle_mm_fault() the second time. This was majorly used to avoid > > unexpected starvation of the system by looping over forever to handle > > the page fault on a single page. However that should hardly happen, > > and after all for each code path to return a VM_FAULT_RETRY we'll > > first wait for a condition (during which time we should possibly yield > > the cpu) to happen before VM_FAULT_RETRY is really returned. > > > > This patch removes the restriction by keeping the > > FAULT_FLAG_ALLOW_RETRY flag when we receive VM_FAULT_RETRY. It means > > that the page fault handler now can retry the page fault for multiple > > times if necessary without the need to generate another page fault > > event. Meanwhile we still keep the FAULT_FLAG_TRIED flag so page > > fault handler can still identify whether a page fault is the first > > attempt or not. > > > > Then we'll have these combinations of fault flags (only considering > > ALLOW_RETRY flag and TRIED flag): > > > > - ALLOW_RETRY and !TRIED: this means the page fault allows to > > retry, and this is the first try > > > > - ALLOW_RETRY and TRIED: this means the page fault allows to > > retry, and this is not the first try > > > > - !ALLOW_RETRY and !TRIED: this means the page fault does not allow > > to retry at all > > > > - !ALLOW_RETRY and TRIED: this is forbidden and should never be used > > > > In existing code we have multiple places that has taken special care > > of the first condition above by checking against (fault_flags & > > FAULT_FLAG_ALLOW_RETRY). This patch introduces a simple helper to > > detect the first retry of a page fault by checking against > > both (fault_flags & FAULT_FLAG_ALLOW_RETRY) and !(fault_flag & > > FAULT_FLAG_TRIED) because now even the 2nd try will have the > > ALLOW_RETRY set, then use that helper in all existing special paths. > > One example is in __lock_page_or_retry(), now we'll drop the mmap_sem > > only in the first attempt of page fault and we'll keep it in follow up > > retries, so old locking behavior will be retained. > > > > This will be a nice enhancement for current code [2] at the same time > > a supporting material for the future userfaultfd-writeprotect work, > > since in that work there will always be an explicit userfault > > writeprotect retry for protected pages, and if that cannot resolve the > > page fault (e.g., when userfaultfd-writeprotect is used in conjunction > > with swapped pages) then we'll possibly need a 3rd retry of the page > > fault. It might also benefit other potential users who will have > > similar requirement like userfault write-protection. > > > > GUP code is not touched yet and will be covered in follow up patch. > > > > Please read the thread below for more information. > > > > [1] https://lkml.org/lkml/2017/11/2/833 > > [2] https://lkml.org/lkml/2018/12/30/64 > > I have few comments on this one. See below. > > > > > > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> > > Suggested-by: Andrea Arcangeli <aarcange@redhat.com> > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > > > arch/alpha/mm/fault.c | 2 +- > > arch/arc/mm/fault.c | 1 - > > arch/arm/mm/fault.c | 3 --- > > arch/arm64/mm/fault.c | 5 ----- > > arch/hexagon/mm/vm_fault.c | 1 - > > arch/ia64/mm/fault.c | 1 - > > arch/m68k/mm/fault.c | 3 --- > > arch/microblaze/mm/fault.c | 1 - > > arch/mips/mm/fault.c | 1 - > > arch/nds32/mm/fault.c | 1 - > > arch/nios2/mm/fault.c | 3 --- > > arch/openrisc/mm/fault.c | 1 - > > arch/parisc/mm/fault.c | 2 -- > > arch/powerpc/mm/fault.c | 6 ------ > > arch/riscv/mm/fault.c | 5 ----- > > arch/s390/mm/fault.c | 5 +---- > > arch/sh/mm/fault.c | 1 - > > arch/sparc/mm/fault_32.c | 1 - > > arch/sparc/mm/fault_64.c | 1 - > > arch/um/kernel/trap.c | 1 - > > arch/unicore32/mm/fault.c | 6 +----- > > arch/x86/mm/fault.c | 2 -- > > arch/xtensa/mm/fault.c | 1 - > > drivers/gpu/drm/ttm/ttm_bo_vm.c | 12 +++++++++--- > > include/linux/mm.h | 12 +++++++++++- > > mm/filemap.c | 2 +- > > mm/shmem.c | 2 +- > > 27 files changed, 25 insertions(+), 57 deletions(-) > > > > [...] > > > diff --git a/arch/parisc/mm/fault.c b/arch/parisc/mm/fault.c > > index 29422eec329d..7d3e96a9a7ab 100644 > > --- a/arch/parisc/mm/fault.c > > +++ b/arch/parisc/mm/fault.c > > @@ -327,8 +327,6 @@ void do_page_fault(struct pt_regs *regs, unsigned long code, > > else > > current->min_flt++; > > if (fault & VM_FAULT_RETRY) { > > - flags &= ~FAULT_FLAG_ALLOW_RETRY; > > Don't you need to also add: > flags |= FAULT_FLAG_TRIED; > > Like other arch. Yes I can add that, thanks for noticing this. Actually I only changed one of the same cases in current patch (alpha, parisc, unicore32 are special cases here where TRIED is never used). I think it's fine to even not have TRIED flag here because if we pass in fault flag with !ALLOW_RETRY and !TRIED it'll simply be the synchronize case so we'll probably be safe too just like a normal 2nd fault retry and we'll wait until page fault resolved. Though after a second thought I think maybe this is also a good chance that we clean this whole thing up to make sure all the archs are using the same pattern to pass fault flags. So I'll touch up the other two places together to make sure TRIED will be there if it's the 2nd retry or more. > > > [...] > > > 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? > > [...] > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index 80bb6408fe73..4e11c9639f1b 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -341,11 +341,21 @@ extern pgprot_t protection_map[16]; > > #define FAULT_FLAG_ALLOW_RETRY 0x04 /* Retry fault if blocking */ > > #define FAULT_FLAG_RETRY_NOWAIT 0x08 /* Don't drop mmap_sem and wait when retrying */ > > #define FAULT_FLAG_KILLABLE 0x10 /* The fault task is in SIGKILL killable region */ > > -#define FAULT_FLAG_TRIED 0x20 /* Second try */ > > +#define FAULT_FLAG_TRIED 0x20 /* We've tried once */ > > #define FAULT_FLAG_USER 0x40 /* The fault originated in userspace */ > > #define FAULT_FLAG_REMOTE 0x80 /* faulting for non current tsk/mm */ > > #define FAULT_FLAG_INSTRUCTION 0x100 /* The fault was during an instruction fetch */ > > > > +/* > > + * Returns true if the page fault allows retry and this is the first > > + * attempt of the fault handling; false otherwise. > > + */ > > You should add why it returns false if it is not the first try ie to > avoid starvation. How about: Returns true if the page fault allows retry and this is the first attempt of the fault handling; false otherwise. This is mostly used for places where we want to try to avoid taking the mmap_sem for too long a time when waiting for another condition to change, in which case we can try to be polite to release the mmap_sem in the first round to avoid potential starvation of other processes that would also want the mmap_sem. ? Thanks,
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. > > [...] > > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > > index 80bb6408fe73..4e11c9639f1b 100644 > > > --- a/include/linux/mm.h > > > +++ b/include/linux/mm.h > > > @@ -341,11 +341,21 @@ extern pgprot_t protection_map[16]; > > > #define FAULT_FLAG_ALLOW_RETRY 0x04 /* Retry fault if blocking */ > > > #define FAULT_FLAG_RETRY_NOWAIT 0x08 /* Don't drop mmap_sem and wait when retrying */ > > > #define FAULT_FLAG_KILLABLE 0x10 /* The fault task is in SIGKILL killable region */ > > > -#define FAULT_FLAG_TRIED 0x20 /* Second try */ > > > +#define FAULT_FLAG_TRIED 0x20 /* We've tried once */ > > > #define FAULT_FLAG_USER 0x40 /* The fault originated in userspace */ > > > #define FAULT_FLAG_REMOTE 0x80 /* faulting for non current tsk/mm */ > > > #define FAULT_FLAG_INSTRUCTION 0x100 /* The fault was during an instruction fetch */ > > > > > > +/* > > > + * Returns true if the page fault allows retry and this is the first > > > + * attempt of the fault handling; false otherwise. > > > + */ > > > > You should add why it returns false if it is not the first try ie to > > avoid starvation. > > How about: > > Returns true if the page fault allows retry and this is the > first attempt of the fault handling; false otherwise. This is > mostly used for places where we want to try to avoid taking > the mmap_sem for too long a time when waiting for another > condition to change, in which case we can try to be polite to > release the mmap_sem in the first round to avoid potential > starvation of other processes that would also want the > mmap_sem. > > ? Looks perfect to me. Cheers, Jérôme
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!
diff --git a/arch/alpha/mm/fault.c b/arch/alpha/mm/fault.c index 8a2ef90b4bfc..6a02c0fb36b9 100644 --- a/arch/alpha/mm/fault.c +++ b/arch/alpha/mm/fault.c @@ -169,7 +169,7 @@ do_page_fault(unsigned long address, unsigned long mmcsr, else current->min_flt++; if (fault & VM_FAULT_RETRY) { - flags &= ~FAULT_FLAG_ALLOW_RETRY; + flags |= FAULT_FLAG_TRIED; /* No need to up_read(&mm->mmap_sem) as we would * have already released it in __lock_page_or_retry diff --git a/arch/arc/mm/fault.c b/arch/arc/mm/fault.c index dc5f1b8859d2..664e18a8749f 100644 --- a/arch/arc/mm/fault.c +++ b/arch/arc/mm/fault.c @@ -167,7 +167,6 @@ void do_page_fault(unsigned long address, struct pt_regs *regs) } if (fault & VM_FAULT_RETRY) { - flags &= ~FAULT_FLAG_ALLOW_RETRY; flags |= FAULT_FLAG_TRIED; goto retry; } diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c index c41c021bbe40..7910b4b5205d 100644 --- a/arch/arm/mm/fault.c +++ b/arch/arm/mm/fault.c @@ -342,9 +342,6 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs) regs, addr); } if (fault & VM_FAULT_RETRY) { - /* Clear FAULT_FLAG_ALLOW_RETRY to avoid any risk - * of starvation. */ - flags &= ~FAULT_FLAG_ALLOW_RETRY; flags |= FAULT_FLAG_TRIED; goto retry; } diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c index a38ff8c49a66..d1d3c98f9ffb 100644 --- a/arch/arm64/mm/fault.c +++ b/arch/arm64/mm/fault.c @@ -523,12 +523,7 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr, return 0; } - /* - * Clear FAULT_FLAG_ALLOW_RETRY to avoid any risk of - * starvation. - */ if (mm_flags & FAULT_FLAG_ALLOW_RETRY) { - mm_flags &= ~FAULT_FLAG_ALLOW_RETRY; mm_flags |= FAULT_FLAG_TRIED; goto retry; } diff --git a/arch/hexagon/mm/vm_fault.c b/arch/hexagon/mm/vm_fault.c index be10b441d9cc..576751597e77 100644 --- a/arch/hexagon/mm/vm_fault.c +++ b/arch/hexagon/mm/vm_fault.c @@ -115,7 +115,6 @@ void do_page_fault(unsigned long address, long cause, struct pt_regs *regs) else current->min_flt++; if (fault & VM_FAULT_RETRY) { - flags &= ~FAULT_FLAG_ALLOW_RETRY; flags |= FAULT_FLAG_TRIED; goto retry; } diff --git a/arch/ia64/mm/fault.c b/arch/ia64/mm/fault.c index 62c2d39d2bed..9de95d39935e 100644 --- a/arch/ia64/mm/fault.c +++ b/arch/ia64/mm/fault.c @@ -189,7 +189,6 @@ ia64_do_page_fault (unsigned long address, unsigned long isr, struct pt_regs *re else current->min_flt++; if (fault & VM_FAULT_RETRY) { - flags &= ~FAULT_FLAG_ALLOW_RETRY; flags |= FAULT_FLAG_TRIED; /* No need to up_read(&mm->mmap_sem) as we would diff --git a/arch/m68k/mm/fault.c b/arch/m68k/mm/fault.c index d9808a807ab8..b1b2109e4ab4 100644 --- a/arch/m68k/mm/fault.c +++ b/arch/m68k/mm/fault.c @@ -162,9 +162,6 @@ int do_page_fault(struct pt_regs *regs, unsigned long address, else current->min_flt++; if (fault & VM_FAULT_RETRY) { - /* Clear FAULT_FLAG_ALLOW_RETRY to avoid any risk - * of starvation. */ - flags &= ~FAULT_FLAG_ALLOW_RETRY; flags |= FAULT_FLAG_TRIED; /* diff --git a/arch/microblaze/mm/fault.c b/arch/microblaze/mm/fault.c index 4fd2dbd0c5ca..05a4847ac0bf 100644 --- a/arch/microblaze/mm/fault.c +++ b/arch/microblaze/mm/fault.c @@ -236,7 +236,6 @@ void do_page_fault(struct pt_regs *regs, unsigned long address, else current->min_flt++; if (fault & VM_FAULT_RETRY) { - flags &= ~FAULT_FLAG_ALLOW_RETRY; flags |= FAULT_FLAG_TRIED; /* diff --git a/arch/mips/mm/fault.c b/arch/mips/mm/fault.c index 92374fd091d2..9953b5b571df 100644 --- a/arch/mips/mm/fault.c +++ b/arch/mips/mm/fault.c @@ -178,7 +178,6 @@ static void __kprobes __do_page_fault(struct pt_regs *regs, unsigned long write, tsk->min_flt++; } if (fault & VM_FAULT_RETRY) { - flags &= ~FAULT_FLAG_ALLOW_RETRY; flags |= FAULT_FLAG_TRIED; /* diff --git a/arch/nds32/mm/fault.c b/arch/nds32/mm/fault.c index 9f6e477b9e30..32259afc751a 100644 --- a/arch/nds32/mm/fault.c +++ b/arch/nds32/mm/fault.c @@ -242,7 +242,6 @@ void do_page_fault(unsigned long entry, unsigned long addr, 1, regs, addr); } if (fault & VM_FAULT_RETRY) { - flags &= ~FAULT_FLAG_ALLOW_RETRY; flags |= FAULT_FLAG_TRIED; /* No need to up_read(&mm->mmap_sem) as we would diff --git a/arch/nios2/mm/fault.c b/arch/nios2/mm/fault.c index 5939434a31ae..9dd1c51acc22 100644 --- a/arch/nios2/mm/fault.c +++ b/arch/nios2/mm/fault.c @@ -158,9 +158,6 @@ asmlinkage void do_page_fault(struct pt_regs *regs, unsigned long cause, else current->min_flt++; if (fault & VM_FAULT_RETRY) { - /* Clear FAULT_FLAG_ALLOW_RETRY to avoid any risk - * of starvation. */ - flags &= ~FAULT_FLAG_ALLOW_RETRY; flags |= FAULT_FLAG_TRIED; /* diff --git a/arch/openrisc/mm/fault.c b/arch/openrisc/mm/fault.c index 873ecb5d82d7..ff92c5674781 100644 --- a/arch/openrisc/mm/fault.c +++ b/arch/openrisc/mm/fault.c @@ -185,7 +185,6 @@ asmlinkage void do_page_fault(struct pt_regs *regs, unsigned long address, else tsk->min_flt++; if (fault & VM_FAULT_RETRY) { - flags &= ~FAULT_FLAG_ALLOW_RETRY; flags |= FAULT_FLAG_TRIED; /* No need to up_read(&mm->mmap_sem) as we would diff --git a/arch/parisc/mm/fault.c b/arch/parisc/mm/fault.c index 29422eec329d..7d3e96a9a7ab 100644 --- a/arch/parisc/mm/fault.c +++ b/arch/parisc/mm/fault.c @@ -327,8 +327,6 @@ void do_page_fault(struct pt_regs *regs, unsigned long code, else current->min_flt++; if (fault & VM_FAULT_RETRY) { - flags &= ~FAULT_FLAG_ALLOW_RETRY; - /* * No need to up_read(&mm->mmap_sem) as we would * have already released it in __lock_page_or_retry diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c index aaa853e6592f..c831cb3ce03f 100644 --- a/arch/powerpc/mm/fault.c +++ b/arch/powerpc/mm/fault.c @@ -583,13 +583,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address, * case. */ if (unlikely(fault & VM_FAULT_RETRY)) { - /* We retry only once */ if (flags & FAULT_FLAG_ALLOW_RETRY) { - /* - * Clear FAULT_FLAG_ALLOW_RETRY to avoid any risk - * of starvation. - */ - flags &= ~FAULT_FLAG_ALLOW_RETRY; flags |= FAULT_FLAG_TRIED; if (is_user && signal_pending(current)) return 0; diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c index 4fc8d746bec3..aad2c0557d2f 100644 --- a/arch/riscv/mm/fault.c +++ b/arch/riscv/mm/fault.c @@ -154,11 +154,6 @@ asmlinkage void do_page_fault(struct pt_regs *regs) 1, regs, addr); } if (fault & VM_FAULT_RETRY) { - /* - * Clear FAULT_FLAG_ALLOW_RETRY to avoid any risk - * of starvation. - */ - flags &= ~(FAULT_FLAG_ALLOW_RETRY); flags |= FAULT_FLAG_TRIED; /* diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c index aba1dad1efcd..4e8c066964a9 100644 --- a/arch/s390/mm/fault.c +++ b/arch/s390/mm/fault.c @@ -513,10 +513,7 @@ static inline vm_fault_t do_exception(struct pt_regs *regs, int access) fault = VM_FAULT_PFAULT; goto out_up; } - /* Clear FAULT_FLAG_ALLOW_RETRY to avoid any risk - * of starvation. */ - flags &= ~(FAULT_FLAG_ALLOW_RETRY | - FAULT_FLAG_RETRY_NOWAIT); + flags &= ~FAULT_FLAG_RETRY_NOWAIT; flags |= FAULT_FLAG_TRIED; down_read(&mm->mmap_sem); goto retry; diff --git a/arch/sh/mm/fault.c b/arch/sh/mm/fault.c index baf5d73df40c..cd710e2d7c57 100644 --- a/arch/sh/mm/fault.c +++ b/arch/sh/mm/fault.c @@ -498,7 +498,6 @@ asmlinkage void __kprobes do_page_fault(struct pt_regs *regs, regs, address); } if (fault & VM_FAULT_RETRY) { - flags &= ~FAULT_FLAG_ALLOW_RETRY; flags |= FAULT_FLAG_TRIED; /* diff --git a/arch/sparc/mm/fault_32.c b/arch/sparc/mm/fault_32.c index a2c83104fe35..6735cd1c09b9 100644 --- a/arch/sparc/mm/fault_32.c +++ b/arch/sparc/mm/fault_32.c @@ -261,7 +261,6 @@ asmlinkage void do_sparc_fault(struct pt_regs *regs, int text_fault, int write, 1, regs, address); } if (fault & VM_FAULT_RETRY) { - flags &= ~FAULT_FLAG_ALLOW_RETRY; flags |= FAULT_FLAG_TRIED; /* No need to up_read(&mm->mmap_sem) as we would diff --git a/arch/sparc/mm/fault_64.c b/arch/sparc/mm/fault_64.c index cad71ec5c7b3..28d5b4d012c6 100644 --- a/arch/sparc/mm/fault_64.c +++ b/arch/sparc/mm/fault_64.c @@ -459,7 +459,6 @@ asmlinkage void __kprobes do_sparc64_fault(struct pt_regs *regs) 1, regs, address); } if (fault & VM_FAULT_RETRY) { - flags &= ~FAULT_FLAG_ALLOW_RETRY; flags |= FAULT_FLAG_TRIED; /* No need to up_read(&mm->mmap_sem) as we would diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c index 09baf37b65b9..c63fc292aea0 100644 --- a/arch/um/kernel/trap.c +++ b/arch/um/kernel/trap.c @@ -99,7 +99,6 @@ int handle_page_fault(unsigned long address, unsigned long ip, else current->min_flt++; if (fault & VM_FAULT_RETRY) { - flags &= ~FAULT_FLAG_ALLOW_RETRY; flags |= FAULT_FLAG_TRIED; goto retry; diff --git a/arch/unicore32/mm/fault.c b/arch/unicore32/mm/fault.c index 3611f19234a1..fdf577956f5f 100644 --- a/arch/unicore32/mm/fault.c +++ b/arch/unicore32/mm/fault.c @@ -260,12 +260,8 @@ static int do_pf(unsigned long addr, unsigned int fsr, struct pt_regs *regs) tsk->maj_flt++; else tsk->min_flt++; - if (fault & VM_FAULT_RETRY) { - /* Clear FAULT_FLAG_ALLOW_RETRY to avoid any risk - * of starvation. */ - flags &= ~FAULT_FLAG_ALLOW_RETRY; + if (fault & VM_FAULT_RETRY) goto retry; - } } up_read(&mm->mmap_sem); 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; diff --git a/arch/xtensa/mm/fault.c b/arch/xtensa/mm/fault.c index 792dad5e2f12..7cd55f2d66c9 100644 --- a/arch/xtensa/mm/fault.c +++ b/arch/xtensa/mm/fault.c @@ -128,7 +128,6 @@ void do_page_fault(struct pt_regs *regs) else current->min_flt++; if (fault & VM_FAULT_RETRY) { - flags &= ~FAULT_FLAG_ALLOW_RETRY; flags |= FAULT_FLAG_TRIED; /* No need to up_read(&mm->mmap_sem) as we would diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c index a1d977fbade5..5fac635f72a5 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c @@ -61,9 +61,10 @@ static vm_fault_t ttm_bo_vm_fault_idle(struct ttm_buffer_object *bo, /* * If possible, avoid waiting for GPU with mmap_sem - * held. + * held. We only do this if the fault allows retry and this + * is the first attempt. */ - if (vmf->flags & FAULT_FLAG_ALLOW_RETRY) { + if (fault_flag_allow_retry_first(vmf->flags)) { ret = VM_FAULT_RETRY; if (vmf->flags & FAULT_FLAG_RETRY_NOWAIT) goto out_unlock; @@ -136,7 +137,12 @@ static vm_fault_t ttm_bo_vm_fault(struct vm_fault *vmf) if (err != -EBUSY) return VM_FAULT_NOPAGE; - if (vmf->flags & FAULT_FLAG_ALLOW_RETRY) { + /* + * If the fault allows retry and this is the first + * fault attempt, we try to release the mmap_sem + * before waiting + */ + if (fault_flag_allow_retry_first(vmf->flags)) { if (!(vmf->flags & FAULT_FLAG_RETRY_NOWAIT)) { ttm_bo_get(bo); up_read(&vmf->vma->vm_mm->mmap_sem); diff --git a/include/linux/mm.h b/include/linux/mm.h index 80bb6408fe73..4e11c9639f1b 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -341,11 +341,21 @@ extern pgprot_t protection_map[16]; #define FAULT_FLAG_ALLOW_RETRY 0x04 /* Retry fault if blocking */ #define FAULT_FLAG_RETRY_NOWAIT 0x08 /* Don't drop mmap_sem and wait when retrying */ #define FAULT_FLAG_KILLABLE 0x10 /* The fault task is in SIGKILL killable region */ -#define FAULT_FLAG_TRIED 0x20 /* Second try */ +#define FAULT_FLAG_TRIED 0x20 /* We've tried once */ #define FAULT_FLAG_USER 0x40 /* The fault originated in userspace */ #define FAULT_FLAG_REMOTE 0x80 /* faulting for non current tsk/mm */ #define FAULT_FLAG_INSTRUCTION 0x100 /* The fault was during an instruction fetch */ +/* + * Returns true if the page fault allows retry and this is the first + * attempt of the fault handling; false otherwise. + */ +static inline bool fault_flag_allow_retry_first(unsigned int flags) +{ + return (flags & FAULT_FLAG_ALLOW_RETRY) && + (!(flags & FAULT_FLAG_TRIED)); +} + #define FAULT_FLAG_TRACE \ { FAULT_FLAG_WRITE, "WRITE" }, \ { FAULT_FLAG_MKWRITE, "MKWRITE" }, \ diff --git a/mm/filemap.c b/mm/filemap.c index 9f5e323e883e..a2b5c53166de 100644 --- a/mm/filemap.c +++ b/mm/filemap.c @@ -1351,7 +1351,7 @@ EXPORT_SYMBOL_GPL(__lock_page_killable); int __lock_page_or_retry(struct page *page, struct mm_struct *mm, unsigned int flags) { - if (flags & FAULT_FLAG_ALLOW_RETRY) { + if (fault_flag_allow_retry_first(flags)) { /* * CAUTION! In this case, mmap_sem is not released * even though return 0. diff --git a/mm/shmem.c b/mm/shmem.c index 6ece1e2fe76e..06fd5e79e1c9 100644 --- a/mm/shmem.c +++ b/mm/shmem.c @@ -1949,7 +1949,7 @@ static vm_fault_t shmem_fault(struct vm_fault *vmf) DEFINE_WAIT_FUNC(shmem_fault_wait, synchronous_wake_function); ret = VM_FAULT_NOPAGE; - if ((vmf->flags & FAULT_FLAG_ALLOW_RETRY) && + if (fault_flag_allow_retry_first(flags) && !(vmf->flags & FAULT_FLAG_RETRY_NOWAIT)) { /* It's polite to up mmap_sem if we can */ up_read(&vma->vm_mm->mmap_sem);
The idea comes from a discussion between Linus and Andrea [1]. Before this patch we only allow a page fault to retry once. We achieved this by clearing the FAULT_FLAG_ALLOW_RETRY flag when doing handle_mm_fault() the second time. This was majorly used to avoid unexpected starvation of the system by looping over forever to handle the page fault on a single page. However that should hardly happen, and after all for each code path to return a VM_FAULT_RETRY we'll first wait for a condition (during which time we should possibly yield the cpu) to happen before VM_FAULT_RETRY is really returned. This patch removes the restriction by keeping the FAULT_FLAG_ALLOW_RETRY flag when we receive VM_FAULT_RETRY. It means that the page fault handler now can retry the page fault for multiple times if necessary without the need to generate another page fault event. Meanwhile we still keep the FAULT_FLAG_TRIED flag so page fault handler can still identify whether a page fault is the first attempt or not. Then we'll have these combinations of fault flags (only considering ALLOW_RETRY flag and TRIED flag): - ALLOW_RETRY and !TRIED: this means the page fault allows to retry, and this is the first try - ALLOW_RETRY and TRIED: this means the page fault allows to retry, and this is not the first try - !ALLOW_RETRY and !TRIED: this means the page fault does not allow to retry at all - !ALLOW_RETRY and TRIED: this is forbidden and should never be used In existing code we have multiple places that has taken special care of the first condition above by checking against (fault_flags & FAULT_FLAG_ALLOW_RETRY). This patch introduces a simple helper to detect the first retry of a page fault by checking against both (fault_flags & FAULT_FLAG_ALLOW_RETRY) and !(fault_flag & FAULT_FLAG_TRIED) because now even the 2nd try will have the ALLOW_RETRY set, then use that helper in all existing special paths. One example is in __lock_page_or_retry(), now we'll drop the mmap_sem only in the first attempt of page fault and we'll keep it in follow up retries, so old locking behavior will be retained. This will be a nice enhancement for current code [2] at the same time a supporting material for the future userfaultfd-writeprotect work, since in that work there will always be an explicit userfault writeprotect retry for protected pages, and if that cannot resolve the page fault (e.g., when userfaultfd-writeprotect is used in conjunction with swapped pages) then we'll possibly need a 3rd retry of the page fault. It might also benefit other potential users who will have similar requirement like userfault write-protection. GUP code is not touched yet and will be covered in follow up patch. Please read the thread below for more information. [1] https://lkml.org/lkml/2017/11/2/833 [2] https://lkml.org/lkml/2018/12/30/64 Suggested-by: Linus Torvalds <torvalds@linux-foundation.org> Suggested-by: Andrea Arcangeli <aarcange@redhat.com> Signed-off-by: Peter Xu <peterx@redhat.com> --- arch/alpha/mm/fault.c | 2 +- arch/arc/mm/fault.c | 1 - arch/arm/mm/fault.c | 3 --- arch/arm64/mm/fault.c | 5 ----- arch/hexagon/mm/vm_fault.c | 1 - arch/ia64/mm/fault.c | 1 - arch/m68k/mm/fault.c | 3 --- arch/microblaze/mm/fault.c | 1 - arch/mips/mm/fault.c | 1 - arch/nds32/mm/fault.c | 1 - arch/nios2/mm/fault.c | 3 --- arch/openrisc/mm/fault.c | 1 - arch/parisc/mm/fault.c | 2 -- arch/powerpc/mm/fault.c | 6 ------ arch/riscv/mm/fault.c | 5 ----- arch/s390/mm/fault.c | 5 +---- arch/sh/mm/fault.c | 1 - arch/sparc/mm/fault_32.c | 1 - arch/sparc/mm/fault_64.c | 1 - arch/um/kernel/trap.c | 1 - arch/unicore32/mm/fault.c | 6 +----- arch/x86/mm/fault.c | 2 -- arch/xtensa/mm/fault.c | 1 - drivers/gpu/drm/ttm/ttm_bo_vm.c | 12 +++++++++--- include/linux/mm.h | 12 +++++++++++- mm/filemap.c | 2 +- mm/shmem.c | 2 +- 27 files changed, 25 insertions(+), 57 deletions(-)