Re: [v5 PATCH] arm64: mm: force write fault for atomic RMW instructions

[Yang Shi]

On 7/11/24 10:43 AM, Catalin Marinas wrote:
> On Wed, Jul 10, 2024 at 11:43:18AM -0700, Yang Shi wrote:
> > On 7/10/24 2:22 AM, Catalin Marinas wrote:
> > > > diff --git a/arch/arm64/mm/mmap.c b/arch/arm64/mm/mmap.c
> > > > index 642bdf908b22..d30265d424e4 100644
> > > > --- a/arch/arm64/mm/mmap.c
> > > > +++ b/arch/arm64/mm/mmap.c
> > > > @@ -19,7 +19,7 @@ static pgprot_t protection_map[16] __ro_after_init = {
> > > >           [VM_WRITE]                                      = PAGE_READONLY,
> > > >           [VM_WRITE | VM_READ]                            = PAGE_READONLY,
> > > >           /* PAGE_EXECONLY if Enhanced PAN */
> > > > -        [VM_EXEC]                                       = PAGE_READONLY_EXEC,
> > > > +        [VM_EXEC]                                       = PAGE_EXECONLY,
> > > >           [VM_EXEC | VM_READ]                             = PAGE_READONLY_EXEC,
> > > >           [VM_EXEC | VM_WRITE]                            = PAGE_READONLY_EXEC,
> > > >           [VM_EXEC | VM_WRITE | VM_READ]                  = PAGE_READONLY_EXEC,
> > > In theory you'd need to change the VM_SHARED | VM_EXEC entry as well.
> > > Otherwise it looks fine.
> > Thanks. I just ran the same benchmark. Ran the modified page_fault1_thread
> > (trigger read fault) in 100 iterations with 160 threads on 160 cores. This
> > should be the worst contention case and collected the max data (worst
> > latency). It shows the patch may incur ~30% overhead for exec-only case. The
> > overhead should just come from the permission fault.
> >
> >      N           Min           Max        Median           Avg Stddev
> > x 100        163840        219083        184471        183262 12593.229
> > + 100        211198        285947        233608     238819.98 15253.967
> > Difference at 95.0% confidence
> >      55558 +/- 3877
> >      30.3161% +/- 2.11555%
> >
> > This is a very extreme benchmark, I don't think any real life workload will
> > spend that much time (sys vs user) in page fault, particularly read fault.
> >
> > With my atomic fault benchmark (populate 1G memory with atomic instruction
> > then manipulate the value stored in the memory in 100 iterations so the user
> > time is much longer than sys time), I saw around 13% overhead on sys time
> > due to the permission fault, but no noticeable change for user and real
> > time.
> Thanks for running these tests.
>
> > So the permission fault does incur noticeable overhead for read fault on
> > exec-only, but it may be not that bad for real life workloads.
> So you are saying 3 faults is not that bad for real life workloads but
> the 2 faults behaviour we have currently is problematic for OpenJDK. For
> the OpenJDK case, I don't think the faulting is the main issue affecting
> run-time performance but, over a longer run (or with more iterations in
> this benchmark after the faulting in), you'd notice the breaking up of
> the initial THP pages.

I meant the extra permission fault for exec-only should be ok since the 
current implementation can't force write fault for exec-only anyway. It 
does incur noticeable overhead for read fault, but I'm not aware of any 
real life workloads are sensitive to read fault. The benchmark is for a 
very extreme worst case.

> Do you have any OpenJDK benchmarks that show the problem? It might be
> worth running them with this patch:
>
> https://lore.kernel.org/r/rjudrmg7nkkwfgviqeqluuww6wu6fdrgdsfimtmpjee7lkz2ej@iosd2f6pk4f7
>
> Or, if not, do you see any difference in the user time in your benchmark
> with the above mm patch? In subsequent iterations, linear accesses are
> not ideal for testing. Better to have some random accesses this 1GB of
> memory (after the initial touching). That would mimic heap accesses a
> bit better.

I didn't try that patch. I think we discussed this before. This patch 
can remove the THP shattering, we should be able to see some 
improvement, but TBLI is still needed and its cost should be still 
noticeable because the write protection fault still happens.

> Anyway, as it stands, I don't think we should merge this patch since it
> does incur an additional penalty with exec-only mappings and it would
> make things even worse for OpenJDK if distros change their default
> toolchain flags at some point to generate exec-only ELF text sections.
> While we could work around this by allowing the kernel to read the
> exec-only user mapping with a new flavour of get_user() (toggling PAN as
> well), I don't think it's worth it. Especially with the above mm change,
> I think the benefits of this patch aren't justified. Longer term, I hope
> that customers upgrade to OpenJDK v22 or, for proprietary tools, they
> either follow the madvise() approach or wait for the Arm architects to
> change the hardware behaviour.

If the overhead for exec-only is a concern, I think we can just skip 
exec-only segment for now, right? The exec-only is not that popular yet. 
And if the users prefer security, typically they may be not that 
sensitive to performance.

>