| Message ID | 20240403083805.1818160-6-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | Needs ACK |
| Headers | show |
| Series | arch/mm/fault: accelerate pagefault when badaccess | expand |
Hi Kefeng, On 03/04/2024 10:38, Kefeng Wang wrote: > The access_error() of vma already checked under per-VMA lock, if it > is a bad access, directly handle error, no need to retry with mmap_lock > again. Since the page faut is handled under per-VMA lock, count it as > a vma lock event with VMA_LOCK_SUCCESS. > > Reviewed-by: Suren Baghdasaryan <surenb@google.com> > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > --- > arch/riscv/mm/fault.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c > index 3ba1d4dde5dd..b3fcf7d67efb 100644 > --- a/arch/riscv/mm/fault.c > +++ b/arch/riscv/mm/fault.c > @@ -292,7 +292,10 @@ void handle_page_fault(struct pt_regs *regs) > > if (unlikely(access_error(cause, vma))) { > vma_end_read(vma); > - goto lock_mmap; > + count_vm_vma_lock_event(VMA_LOCK_SUCCESS); > + tsk->thread.bad_cause = SEGV_ACCERR; I think we should use the cause variable here instead of SEGV_ACCERR, as bad_cause is a riscv internal status which describes the real fault that happened. Thanks, Alex > + bad_area_nosemaphore(regs, code, addr); > + return; > } > > fault = handle_mm_fault(vma, addr, flags | FAULT_FLAG_VMA_LOCK, regs);
On 2024/4/10 15:32, Alexandre Ghiti wrote: > Hi Kefeng, > > On 03/04/2024 10:38, Kefeng Wang wrote: >> The access_error() of vma already checked under per-VMA lock, if it >> is a bad access, directly handle error, no need to retry with mmap_lock >> again. Since the page faut is handled under per-VMA lock, count it as >> a vma lock event with VMA_LOCK_SUCCESS. >> >> Reviewed-by: Suren Baghdasaryan <surenb@google.com> >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >> --- >> arch/riscv/mm/fault.c | 5 ++++- >> 1 file changed, 4 insertions(+), 1 deletion(-) >> >> diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c >> index 3ba1d4dde5dd..b3fcf7d67efb 100644 >> --- a/arch/riscv/mm/fault.c >> +++ b/arch/riscv/mm/fault.c >> @@ -292,7 +292,10 @@ void handle_page_fault(struct pt_regs *regs) >> if (unlikely(access_error(cause, vma))) { >> vma_end_read(vma); >> - goto lock_mmap; >> + count_vm_vma_lock_event(VMA_LOCK_SUCCESS); >> + tsk->thread.bad_cause = SEGV_ACCERR; > > > I think we should use the cause variable here instead of SEGV_ACCERR, as > bad_cause is a riscv internal status which describes the real fault that > happened. Oh, I see, it is exception causes on riscv, so it should be diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c index b3fcf7d67efb..5224f3733802 100644 --- a/arch/riscv/mm/fault.c +++ b/arch/riscv/mm/fault.c @@ -293,8 +293,8 @@ void handle_page_fault(struct pt_regs *regs) if (unlikely(access_error(cause, vma))) { vma_end_read(vma); count_vm_vma_lock_event(VMA_LOCK_SUCCESS); - tsk->thread.bad_cause = SEGV_ACCERR; - bad_area_nosemaphore(regs, code, addr); + tsk->thread.bad_cause = cause; + bad_area_nosemaphore(regs, SEGV_ACCERR, addr); return; } Hi Alex, could you help to check it? Hi Andrew, please help to squash it after Alex ack it. Thanks both. > > Thanks, > > Alex > > >> + bad_area_nosemaphore(regs, code, addr); >> + return; >> } >> fault = handle_mm_fault(vma, addr, flags | FAULT_FLAG_VMA_LOCK, >> regs);
On 10/04/2024 10:07, Kefeng Wang wrote: > > > On 2024/4/10 15:32, Alexandre Ghiti wrote: >> Hi Kefeng, >> >> On 03/04/2024 10:38, Kefeng Wang wrote: >>> The access_error() of vma already checked under per-VMA lock, if it >>> is a bad access, directly handle error, no need to retry with mmap_lock >>> again. Since the page faut is handled under per-VMA lock, count it as >>> a vma lock event with VMA_LOCK_SUCCESS. >>> >>> Reviewed-by: Suren Baghdasaryan <surenb@google.com> >>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >>> --- >>> arch/riscv/mm/fault.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c >>> index 3ba1d4dde5dd..b3fcf7d67efb 100644 >>> --- a/arch/riscv/mm/fault.c >>> +++ b/arch/riscv/mm/fault.c >>> @@ -292,7 +292,10 @@ void handle_page_fault(struct pt_regs *regs) >>> if (unlikely(access_error(cause, vma))) { >>> vma_end_read(vma); >>> - goto lock_mmap; >>> + count_vm_vma_lock_event(VMA_LOCK_SUCCESS); >>> + tsk->thread.bad_cause = SEGV_ACCERR; >> >> >> I think we should use the cause variable here instead of SEGV_ACCERR, >> as bad_cause is a riscv internal status which describes the real >> fault that happened. > > Oh, I see, it is exception causes on riscv, so it should be > > diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c > index b3fcf7d67efb..5224f3733802 100644 > --- a/arch/riscv/mm/fault.c > +++ b/arch/riscv/mm/fault.c > @@ -293,8 +293,8 @@ void handle_page_fault(struct pt_regs *regs) > if (unlikely(access_error(cause, vma))) { > vma_end_read(vma); > count_vm_vma_lock_event(VMA_LOCK_SUCCESS); > - tsk->thread.bad_cause = SEGV_ACCERR; > - bad_area_nosemaphore(regs, code, addr); > + tsk->thread.bad_cause = cause; > + bad_area_nosemaphore(regs, SEGV_ACCERR, addr); > return; > } > > Hi Alex, could you help to check it? > > Hi Andrew, please help to squash it after Alex ack it. > > Thanks both. So I have just tested Kefeng's fixup on my usual CI and with a simple program that triggers such bad access, everything went fine so with the fixup applied: Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com> Tested-by: Alexandre Ghiti <alexghiti@rivosinc.com> Thanks, Alex > > >> >> Thanks, >> >> Alex >> >> >>> + bad_area_nosemaphore(regs, code, addr); >>> + return; >>> } >>> fault = handle_mm_fault(vma, addr, flags | >>> FAULT_FLAG_VMA_LOCK, regs);
On 2024/4/11 1:28, Alexandre Ghiti wrote: > On 10/04/2024 10:07, Kefeng Wang wrote: >> >> >> On 2024/4/10 15:32, Alexandre Ghiti wrote: >>> Hi Kefeng, >>> >>> On 03/04/2024 10:38, Kefeng Wang wrote: >>>> The access_error() of vma already checked under per-VMA lock, if it >>>> is a bad access, directly handle error, no need to retry with mmap_lock >>>> again. Since the page faut is handled under per-VMA lock, count it as >>>> a vma lock event with VMA_LOCK_SUCCESS. >>>> >>>> Reviewed-by: Suren Baghdasaryan <surenb@google.com> >>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >>>> --- >>>> arch/riscv/mm/fault.c | 5 ++++- >>>> 1 file changed, 4 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c >>>> index 3ba1d4dde5dd..b3fcf7d67efb 100644 >>>> --- a/arch/riscv/mm/fault.c >>>> +++ b/arch/riscv/mm/fault.c >>>> @@ -292,7 +292,10 @@ void handle_page_fault(struct pt_regs *regs) >>>> if (unlikely(access_error(cause, vma))) { >>>> vma_end_read(vma); >>>> - goto lock_mmap; >>>> + count_vm_vma_lock_event(VMA_LOCK_SUCCESS); >>>> + tsk->thread.bad_cause = SEGV_ACCERR; >>> >>> >>> I think we should use the cause variable here instead of SEGV_ACCERR, >>> as bad_cause is a riscv internal status which describes the real >>> fault that happened. >> >> Oh, I see, it is exception causes on riscv, so it should be >> >> diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c >> index b3fcf7d67efb..5224f3733802 100644 >> --- a/arch/riscv/mm/fault.c >> +++ b/arch/riscv/mm/fault.c >> @@ -293,8 +293,8 @@ void handle_page_fault(struct pt_regs *regs) >> if (unlikely(access_error(cause, vma))) { >> vma_end_read(vma); >> count_vm_vma_lock_event(VMA_LOCK_SUCCESS); >> - tsk->thread.bad_cause = SEGV_ACCERR; >> - bad_area_nosemaphore(regs, code, addr); >> + tsk->thread.bad_cause = cause; >> + bad_area_nosemaphore(regs, SEGV_ACCERR, addr); >> return; >> } >> >> Hi Alex, could you help to check it? >> >> Hi Andrew, please help to squash it after Alex ack it. >> >> Thanks both. > > > So I have just tested Kefeng's fixup on my usual CI and with a simple > program that triggers such bad access, everything went fine so with the > fixup applied: > > Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com> > Tested-by: Alexandre Ghiti <alexghiti@rivosinc.com> Great, thanks. > > Thanks, > > Alex > > > >> >> >>> >>> Thanks, >>> >>> Alex >>> >>> >>>> + bad_area_nosemaphore(regs, code, addr); >>>> + return; >>>> } >>>> fault = handle_mm_fault(vma, addr, flags | >>>> FAULT_FLAG_VMA_LOCK, regs);
diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c index 3ba1d4dde5dd..b3fcf7d67efb 100644 --- a/arch/riscv/mm/fault.c +++ b/arch/riscv/mm/fault.c @@ -292,7 +292,10 @@ void handle_page_fault(struct pt_regs *regs) if (unlikely(access_error(cause, vma))) { vma_end_read(vma); - goto lock_mmap; + count_vm_vma_lock_event(VMA_LOCK_SUCCESS); + tsk->thread.bad_cause = SEGV_ACCERR; + bad_area_nosemaphore(regs, code, addr); + return; } fault = handle_mm_fault(vma, addr, flags | FAULT_FLAG_VMA_LOCK, regs);