Re: darktable performance regression on AMD systems caused by "mm: align larger anonymous mappings on THP boundaries"

[Vlastimil Babka <vbabka@suse.cz>]

On 10/24/24 13:13, Petr Tesarik wrote:
> On Thu, 24 Oct 2024 12:56:27 +0200
> Vlastimil Babka <vbabka@suse.cz> wrote:
> 
>> On 10/24/24 12:49, Petr Tesarik wrote:
>> > On Thu, 24 Oct 2024 12:23:48 +0200
>> > Vlastimil Babka <vbabka@suse.cz> wrote:
>> >   
>> >> On 10/24/24 11:58, Vlastimil Babka wrote:  
>> >> > On 10/24/24 09:45, Thorsten Leemhuis wrote:    
>> >> >> Hi, Thorsten here, the Linux kernel's regression tracker.
>> >> >> 
>> >> >> Rik, I noticed a report about a regression in bugzilla.kernel.org that
>> >> >> appears to be caused by the following change of yours:
>> >> >> 
>> >> >> efa7df3e3bb5da ("mm: align larger anonymous mappings on THP boundaries")
>> >> >> [v6.7]
>> >> >> 
>> >> >> It might be one of those "some things got faster, a few things became
>> >> >> slower" situations. Not sure. Felt odd that the reporter was able to
>> >> >> reproduce it on two AMD systems, but not on a Intel system. Maybe there
>> >> >> is a bug somewhere else that was exposed by this.    
>> >> > 
>> >> > It seems very similar to what we've seen with spec benchmarks such as cactus
>> >> > and bisected to the same commit:
>> >> > 
>> >> > https://bugzilla.suse.com/show_bug.cgi?id=1229012
>> >> > 
>> >> > The exact regression varies per system. Intel regresses too but relatively
>> >> > less. The theory is that there are many large-ish allocations that don't
>> >> > have individual sizes aligned to 2MB and would have been merged, commit
>> >> > efa7df3e3bb5da causes them to become separate areas where each aligns its
>> >> > start at 2MB boundary and there are gaps between. This (gaps and vma
>> >> > fragmentation) itself is not great, but most of the problem seemed to be
>> >> > from the start alignment, which togethter with the access pattern causes
>> >> > more TLB or cache missess due to limited associtativity.
>> >> > 
>> >> > So maybe darktable has a similar problem. A simple candidate fix could
>> >> > change commit efa7df3e3bb5da so that the mapping size has to be a multiple
>> >> > of THP size (2MB) in order to become aligned, right now it's enough if it's
>> >> > THP sized or larger.    
>> >> 
>> >> Maybe this could be enough to fix the issue? (on 6.12-rc4)  
>> > 
>> > 
>> > Yes, this should work. I was unsure if thp_get_unmapped_area_vmflags()
>> > differs in other ways from mm_get_unmapped_area_vmflags(), which might
>> > still be relevant. I mean, does mm_get_unmapped_area_vmflags() also
>> > prefer to allocate THPs if the virtual memory block is large enough?  
>> 
>> Well any sufficiently large area spanning a PMD aligned/sized block (either
>> a result of a single allocation or merging of several allocations) can
>> become populated by THPs (at least in those aligned blocks), and the
>> preference depends on system-wide THP settings and madvise(MADV_HUGEPAGE) or
>> prctl.
>> 
>> But mm_get_unmapped_area_vmflags() will AFAIK not try to align the area to
>> PMD size like the thp_ version would, even if the request is large enough.
> 
> Then it sounds like exactly what we want. But I prefer to move the
> check down to __thp_get_unmapped_area() like this:

I wanted to limit the fix to the place commit efa7df3e3bb5da changes, i.e.
anonymous mappings, because there are other callers of
__thp_get_unmapped_area(), namely the filesystems via
thp_get_unmapped_area() and I wasn't sure if that wouldn't regress them. But
since you suggested I had a brief look now...

> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 2fb328880b50..8d80f3af5525 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1082,6 +1082,9 @@ static unsigned long __thp_get_unmapped_area(struct file *filp,
>  	if (!IS_ENABLED(CONFIG_64BIT) || in_compat_syscall())
>  		return 0;
>  
> +	if (!IS_ALIGNED(len, size))
> +		return 0;

I think the filesystem might be asked to create a mapping for e.g. a
[1MB, 4MB] range from a file, thus the offset would be 1MB (with anonymous
pages an off=0 is passed) and the current implementation would try to do the
right thing for that (align the [2MB, 4MB] range to THP) but after your
patch it would see len is 3MB and give up, no?

> +
>  	if (off_end <= off_align || (off_end - off_align) < size)
>  		return 0;
>