Messages in this thread | ![/](/images/icornerl.gif) | | Date | Fri, 17 May 2024 18:25:42 +0100 | From | Catalin Marinas <> | Subject | Re: [PATCH] arm64: mm: force write fault for atomic RMW instructions |
| |
On Fri, May 17, 2024 at 09:30:23AM -0700, Yang Shi wrote: > On 5/14/24 3:39 AM, Catalin Marinas wrote: > > It would be good to understand why openjdk is doing this instead of a > > plain write. Is it because it may be racing with some other threads > > already using the heap? That would be a valid pattern. > > Yes, you are right. I think I quoted the JVM justification in earlier email, > anyway they said "permit use of memory concurrently with pretouch".
Ah, sorry, I missed that. This seems like a valid reason.
> > A point Will raised was on potential ABI changes introduced by this > > patch. The ESR_EL1 reported to user remains the same as per the hardware > > spec (read-only), so from a SIGSEGV we may have some slight behaviour > > changes: > > > > 1. PTE invalid: > > > > a) vma is VM_READ && !VM_WRITE permission - SIGSEGV reported with > > ESR_EL1.WnR == 0 in sigcontext with your patch. Without this > > patch, the PTE is mapped as PTE_RDONLY first and a subsequent > > fault will report SIGSEGV with ESR_EL1.WnR == 1. > > I think I can do something like the below conceptually: > > if is_el0_atomic_instr && !is_write_abort > force_write = true > > if VM_READ && !VM_WRITE && force_write == true
Nit: write implies read, so you only need to check !write.
> vm_flags = VM_READ > mm_flags ~= FAULT_FLAG_WRITE > > Then we just fallback to read fault. The following write fault will trigger > SIGSEGV with consistent ABI.
I think this should work. So instead of reporting the write fault directly in case of a read-only vma, we let the core code handle the read fault and first and we retry the atomic instruction.
> > b) vma is !VM_READ && !VM_WRITE permission - SIGSEGV reported with > > ESR_EL1.WnR == 0, so no change from current behaviour, unless we > > fix the patch for (1.a) to fake the WnR bit which would change the > > current expectations. > > > > 2. PTE valid with PTE_RDONLY - we get a normal writeable fault in > > hardware, no need to fix ESR_EL1 up. > > > > The patch would have to address (1) above but faking the ESR_EL1.WnR bit > > based on the vma flags looks a bit fragile. > > I think we don't need to fake the ESR_EL1.WnR bit with the fallback.
I agree, with your approach above we don't need to fake WnR.
> > Similarly, we have userfaultfd that reports the fault to user. I think > > in scenario (1) the kernel will report UFFD_PAGEFAULT_FLAG_WRITE with > > your patch but no UFFD_PAGEFAULT_FLAG_WP. Without this patch, there are > > indeed two faults, with the second having both UFFD_PAGEFAULT_FLAG_WP > > and UFFD_PAGEFAULT_FLAG_WRITE set. > > I don't quite get what the problem is. IIUC, uffd just needs a signal from > kernel to tell this area will be written. It seems not break the semantic. > Added Peter Xu in this loop, who is the uffd developer. He may shed some > light.
Not really familiar with uffd but just looking at the code, if a handler is registered for both MODE_MISSING and MODE_WP, currently the atomic instruction signals a user fault without UFFD_PAGEFAULT_FLAG_WRITE (the do_anonymous_page() path). If the page is mapped by the uffd handler as the zero page, a restart of the instruction would signal UFFD_PAGEFAULT_FLAG_WRITE and UFFD_PAGEFAULT_FLAG_WP (the do_wp_page() path).
With your patch, we get the equivalent of UFFD_PAGEFAULT_FLAG_WRITE on the first attempt, just like having a STR instruction instead of separate LDR + STR (as the atomics behave from a fault perspective).
However, I don't think that's a problem, the uffd handler should cope with an STR anyway, so it's not some unexpected combination of flags.
-- Catalin
| ![\](/images/icornerr.gif) |