On Fri, Jan 28, 2022 at 12:41 AM David Hildenbrand <david@xxxxxxxxxx> wrote: > > On 27.01.22 22:23, Yang Shi wrote: > > On Wed, Jan 26, 2022 at 2:00 AM David Hildenbrand <david@xxxxxxxxxx> wrote: > >> > >> reuse_swap_page() currently indicates if we can write to an anon page > >> without COW. A COW is required if the page is shared by multiple > >> processes (either already mapped or via swap entries) or if there is > >> concurrent writeback that cannot tolerate concurrent page modifications. > >> > >> reuse_swap_page() doesn't check for pending references from other > >> processes that already unmapped the page, however, > >> is_refcount_suitable() essentially does the same thing in the context of > >> khugepaged. khugepaged is the last remaining user of reuse_swap_page() and > >> we want to remove that function. > >> > >> In the context of khugepaged, we are not actually going to write to the > >> page and we don't really care about other processes mapping the page: > >> for example, without swap, we don't care about shared pages at all. > >> > >> The current logic seems to be: > >> * Writable: -> Not shared, but might be in the swapcache. Nobody can > >> fault it in from the swapcache as there are no other swap entries. > >> * Readable and not in the swapcache: Might be shared (but nobody can > >> fault it in from the swapcache). > >> * Readable and in the swapcache: Might be shared and someone might be > >> able to fault it in from the swapcache. Make sure we're the exclusive > >> owner via reuse_swap_page(). > >> > >> Having to guess due to lack of comments and documentation, the current > >> logic really only wants to make sure that a page that might be shared > >> cannot be faulted in from the swapcache while khugepaged is active. > >> It's hard to guess why that is that case and if it's really still required, > >> but let's try keeping that logic unmodified. > > > > I don't think it could be faulted in while khugepaged is active since > > khugepaged does hold mmap_lock in write mode IIUC. So page fault is > > serialized against khugepaged. > > It could get faulted in by another process sharing the page, because we > only synchronize against the current process. Yes, sure. I'm supposed you meant do_swap_page() called by another process. But it is serialized by page lock. So khugepaged won't see something in limbo state IIUC. > > > > > My wild guess is that collapsing shared pages was not supported before > > v5.8, so we need reuse_swap_page() to tell us if the page in swap > > cache is shared or not. But it is not true anymore. And khugepaged > > just allocates a THP then copy the data from base pages to huge page > > then replace PTEs to PMD, it doesn't change the content of the page, > > so I failed to see a problem by collapsing a shared page in swap > > cache. But I'm really not entirely sure, I may miss something... > > > Looking more closely where this logic originates from, it was introduced in: > > commit 10359213d05acf804558bda7cc9b8422a828d1cd > Author: Ebru Akagunduz <ebru.akagunduz@xxxxxxxxx> > Date: Wed Feb 11 15:28:28 2015 -0800 > > mm: incorporate read-only pages into transparent huge pages > > This patch aims to improve THP collapse rates, by allowing THP collapse in > the presence of read-only ptes, like those left in place by do_swap_page > after a read fault. > > Currently THP can collapse 4kB pages into a THP when there are up to > khugepaged_max_ptes_none pte_none ptes in a 2MB range. This patch applies > the same limit for read-only ptes. > > > The change essentially results in a read-only mapped PTE page getting copied and > mapped writable via a new PMD-mapped THP. > > It mentions do_swap_page(), so I assume it just tried to do what do_swap_page() > would do when trying to map a page swapped in from the page cache writable > immediately. > > But we differ from do_swap_page() that we're not actually going to map the page > writable, we're going to copy the page (__collapse_huge_page_copy()) and map > the copy writable. Yeah, this is the point. Khugepaged or the process being collapsed won't write to the original page. Just unshare it. > > I assume we can remove that logic. > > -- > Thanks, > > David / dhildenb >
