Re: [PATCH] mm: split thp synchronously on MADV_DONTNEED

From: Shakeel Butt <shakeelb@google.com>
To: Peter Xu <peterx@redhat.com>
Cc: David Hildenbrand <david@redhat.com>,
	 "Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
	Yang Shi <shy828301@gmail.com>,  Zi Yan <ziy@nvidia.com>,
	Matthew Wilcox <willy@infradead.org>,
	 Andrew Morton <akpm@linux-foundation.org>,
	linux-mm@kvack.org,  linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mm: split thp synchronously on MADV_DONTNEED
Date: Thu, 25 Nov 2021 19:21:54 -0800	[thread overview]
Message-ID: <CALvZod7L5Ga2xZOy_hgocsTxSuOYs840TiviOAhRwz59ATubWg@mail.gmail.com> (raw)
In-Reply-To: <YZ9kUD5AG6inbUEg@xz-m1.local>

On Thu, Nov 25, 2021 at 2:24 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Mon, Nov 22, 2021 at 10:40:54AM -0800, Shakeel Butt wrote:
> > > Do we have a performance evaluation how much overhead is added e.g., for
> > > a single 4k MADV_DONTNEED call on a THP or on a MADV_DONTNEED call that
> > > covers the whole THP?
> >
> > I did a simple benchmark of madvise(MADV_DONTNEED) on 10000 THPs on
> > x86 for both settings you suggested. I don't see any statistically
> > significant difference with and without the patch. Let me know if you
> > want me to try something else.
>
> I'm a bit surprised that sync split thp didn't bring any extra overhead.
>
> "unmap whole thp" is understandable from that pov, because afaict that won't
> even trigger any thp split anyway even delayed, if this is the simplest case
> that only this process mapped this thp, and it mapped once.
>
> For "unmap 4k upon thp" IIUC that's the worst case and zapping 4k should be
> fast; while what I don't understand since thp split requires all hand-made work
> for copying thp flags into small pages and so on, so I thought there should at
> least be some overhead measured.  Shakeel, could there be something overlooked
> in the test, or maybe it's me that overlooked?
>

Thanks for making me rerun this and yes indeed I had a very silly bug in the
benchmark code (i.e. madvise the same page for the whole loop) and this is
indeed several times slower than without the patch (sorry David for misleading
you).

To better understand what is happening, I profiled the benchmark:

-   31.27%     0.01%  dontneed  [kernel.kallsyms]  [k] zap_page_range_sync
   - 31.27% zap_page_range_sync
      - 30.25% split_local_deferred_list
         - 30.16% split_huge_page_to_list
            - 21.05% try_to_migrate
               + rmap_walk_anon
            - 7.47% remove_migration_ptes
               + 7.34% rmap_walk_locked
      + 1.02% zap_page_range_details

The overhead is not due to copying page flags but rather due to two rmap walks.
I don't think this much overhead is justified for current users of MADV_DONTNEED
and munmap. I have to rethink the approach.

thanks,
Shakeel