Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <da2846a1-f950-d330-7ada-ad3c9abfde74@redhat.com>
Date:   Thu, 20 Jan 2022 21:09:46 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.4.0
Subject: Re: [PATCH] mm: reuse the unshared swapcache page in do_wp_page
Content-Language: en-US
To:     Matthew Wilcox <willy@infradead.org>
Cc:     Nadav Amit <nadav.amit@gmail.com>,
        "zhangliang (AG)" <zhangliang5@huawei.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Linux-MM <linux-mm@kvack.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        wangzhigang17@huawei.com,
        Linus Torvalds <torvalds@linux-foundation.org>
References: <a93988da-80fb-dd32-4717-a6a0bae9e4ee@huawei.com>
 <dc415c4a-63aa-19b0-0fbc-795989970f6d@redhat.com>
 <fb02087a-b102-c91e-ab65-fb02cc8ee0a2@huawei.com>
 <9cd7eee2-91fd-ddb8-e47d-e8585e5baa05@redhat.com>
 <b6df4f7f-c080-ad6c-d1ad-098115f016f3@huawei.com>
 <747ff31c-6c9e-df6c-f14d-c43aa1c77b4a@redhat.com>
 <C8734D0B-B855-4323-A7DF-2D96245951B2@gmail.com>
 <8931808d-db61-0f06-ceb3-f48a83b1f74c@redhat.com>
 <6225EAFF-B323-4DC5-AC4C-885B29ED7261@gmail.com>
 <9071d5a8-ed2d-5cf5-5526-43fe7dd377ec@redhat.com>
 <YenA7Xzd2G2OYvqz@casper.infradead.org>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
In-Reply-To: <YenA7Xzd2G2OYvqz@casper.infradead.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 20.01.22 21:07, Matthew Wilcox wrote:
> On Thu, Jan 20, 2022 at 08:55:12PM +0100, David Hildenbrand wrote:
>>>>> David, does any of it regards the lru_cache_add() reference issue that I
>>>>> mentioned? [1]
> 
>> +++ b/mm/memory.c
>> @@ -3291,19 +3291,28 @@ static vm_fault_t do_wp_page(struct vm_fault *vmf)
>>         if (PageAnon(vmf->page)) {
>>                 struct page *page = vmf->page;
>>  
>> -               /* PageKsm() doesn't necessarily raise the page refcount */
>> -               if (PageKsm(page) || page_count(page) != 1)
>> +               /*
>> +                * PageKsm() doesn't necessarily raise the page refcount.
>> +                *
>> +                * These checks are racy as long as we haven't locked the page;
>> +                * they are a pure optimization to avoid trying to lock the page
>> +                * and trying to free the swap cache when there is little hope
>> +                * it will actually result in a refcount of 1.
>> +                */
>> +               if (PageKsm(page) || page_count(page) > 1 + PageSwapCache(page))
>>                         goto copy;
>>                 if (!trylock_page(page))
>>                         goto copy;
>> -               if (PageKsm(page) || page_mapcount(page) != 1 || page_count(page) != 1) {
>> +               if (PageSwapCache(page))
>> +                       try_to_free_swap(page);
>> +               if (PageKsm(page) || page_count(page) != 1) {
>>                         unlock_page(page);
>>                         goto copy;
>>                 }
>>                 /*
>> -                * Ok, we've got the only map reference, and the only
>> -                * page count reference, and the page is locked,
>> -                * it's dark out, and we're wearing sunglasses. Hit it.
>> +                * Ok, we've got the only page reference from our mapping
>> +                * and the page is locked, it's dark out, and we're wearing
>> +                * sunglasses. Hit it.
>>                  */
>>                 unlock_page(page);
>>                 wp_page_reuse(vmf);
>>
>>
>> I added some vmstats that monitor various paths. After one run of
>> 	./forceswap 2 1000000 1
>> I'm left with a rough delta (including some noise) of
>> 	anon_wp_copy_count 1799
>> 	anon_wp_copy_count_early 1
>> 	anon_wp_copy_lock 983396
>> 	anon_wp_reuse 0
>>
>> The relevant part of your reproducer is
>>
>> 	for (i = 0; i < nops; i++) {
>> 		if (madvise((void *)p, PAGE_SIZE * npages, MADV_PAGEOUT)) {
>> 			perror("madvise");
>> 			exit(-1);
>> 		}
>>
>> 		for (j = 0; j < npages; j++) {
>> 			c = p[j * PAGE_SIZE];
>> 			c++;
>> 			time -= rdtscp();
>> 			p[j * PAGE_SIZE] = c;
>> 			time += rdtscp();
>> 		}
>> 	}
>>
>> For this specific reproducer at least, the page lock seems to be the thingy that prohibits
>> reuse if I interpret the numbers correctly. We pass the initial page_count() check.
>>
>> Haven't looked into the details, and I would be curious how that performs with actual
>> workloads, if we can reproduce similar behavior.
> 
> I don't see how that patch addresses the lru issue.  Wouldn't we need
> something like ...
> 
> 	if (!PageLRU(page))
> 		lru_add_drain_all();
> 

See my other reply "No, unfortunately not in that part of my work.".

Would the lru handling help here where we force swapout of a single
page, reuse code passes the "page_count(page) > 1 + PageSwapCache(page)"
check but fails to lock the page?

-- 
Thanks,

David / dhildenb