From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 51358C54798 for ; Sat, 9 Mar 2024 08:18:35 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C1F986B0071; Sat, 9 Mar 2024 03:18:34 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id BCFC16B0072; Sat, 9 Mar 2024 03:18:34 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A966A6B0074; Sat, 9 Mar 2024 03:18:34 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 980A46B0071 for ; Sat, 9 Mar 2024 03:18:34 -0500 (EST) Received: from smtpin28.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 3FE84160984 for ; Sat, 9 Mar 2024 08:18:34 +0000 (UTC) X-FDA: 81876798948.28.FEE6E13 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf19.hostedemail.com (Postfix) with ESMTP id 2BF391A000B for ; Sat, 9 Mar 2024 08:18:31 +0000 (UTC) Authentication-Results: imf19.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf19.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1709972312; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=QmYF8S2hPv2waCNlFCy/9LliTODPxT3Ligi4iOnfysw=; b=D3PWvjZqNuDjRCv2VgWyCPqsZaXP8JgTvbfSEFS5V/Bh7Z6jqzP+8iJ2vpUiH+VAQmSSzv +RgWF4/6VtEdjgL82gTHyboc1JARdcu98F4yrM1xwr7awYCiLqRR+OJe/RPg/Q0AZ6ctFf +2W0O6lh2MCL5IMK1j6L5dACImyoqtE= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf19.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1709972312; a=rsa-sha256; cv=none; b=bE4z8zAThJQzbLSJIboNXcDSf9s2h3VcLLvSsC5JC2A57JyK9pQ4VMqjwHBDhJ9hi/qf9o Bviz3yKBEm0eWsSlAyEnxkv7PwJX9cmoL8w8RWM+6AE/4ysbcNRKLap/qCIvFKYDoE5WQz vOJSNWC2n+ef8MWwMp042XL6x2JnD4U= Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 98A85C15; Sat, 9 Mar 2024 00:19:07 -0800 (PST) Received: from [10.57.67.186] (unknown [10.57.67.186]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id EBF3A3F73F; Sat, 9 Mar 2024 00:18:29 -0800 (PST) Message-ID: <95f91a7f-13dd-4212-97ce-cf0b4060828a@arm.com> Date: Sat, 9 Mar 2024 08:18:27 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 10/18] mm: Allow non-hugetlb large folios to be batch processed Content-Language: en-GB From: Ryan Roberts To: Matthew Wilcox Cc: Zi Yan , Andrew Morton , linux-mm@kvack.org, Yang Shi , Huang Ying References: <367a14f7-340e-4b29-90ae-bc3fcefdd5f4@arm.com> <85cc26ed-6386-4d6b-b680-1e5fba07843f@arm.com> <36bdda72-2731-440e-ad15-39b845401f50@arm.com> <03CE3A00-917C-48CC-8E1C-6A98713C817C@nvidia.com> <090c9d68-9296-4338-9afa-5369bb1db66c@arm.com> In-Reply-To: <090c9d68-9296-4338-9afa-5369bb1db66c@arm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspamd-Server: rspam09 X-Rspamd-Queue-Id: 2BF391A000B X-Stat-Signature: wa6fao7fuerqkai6n3y49zcqbrmzxxw3 X-Rspam-User: X-HE-Tag: 1709972311-898871 X-HE-Meta: U2FsdGVkX18Svnw7Ho7N7Z98Bg6HrFMF/F9ntSAKorCHo2UYfDNYRNPCWLGmz3arCHqpZQoYLzswXJrUMAMPhk/pcHCNXFSgBamD0KpYNol7yi/jeYpAS4xXh5tigOO/KClOBIp/1W2D0+5rufC7qSlMWTxItLdyETuJK3BocAnMVmmKN7I4hzC8mryfavwp0SX38wQLIK9L01B5XIphdgNcmP1vjAMNyl7+nXOpwD2b1l2oEPZv8KaePeRAYJw99WvOBwHQ99slNUztigDlxoAAQcCzPi3LwEeepiNvmBTC/L+0TVrKnv/OrgWRJey0zgE3L/masBQHqGCt2KUlZsGtamGenjQvj3i/OEkNavPCL+tqpeDgXunaDa8reHD0xzFXtmh0/0X9aptbTo7YJyPTRkYcohVZQc0TPelyOspq//1LRifpBjMK88TjKxlBkRjTk20f5fMRinHo+XmxiIGcNDrSUwGeA9pYn1F9QF+91RKaU0DuOOSAsTaZQOwW9U+m4+Pmo3l+ele2u2jL0FeFW/DaHRo/ZnVD3KoYljld+GnfsQoypec0/yl7SqHufbmBAtsDtFaDcqdBNSZbGCwUX/wF+ufJB3zBWtMEo9/1sTFLRlUvhC9YMiDe2bHCkO3VR0/+8101aSDXr09hjPzLKv1VSHv6lLbAhqVyH4Pw7MkN94LQ3eF3phWvjv/HG1LmnrYdBCs2l9XEUyIOLkiqVVhfGZhNEv01fZpGsdZyvu3sVFohjAMnSBOqKOrYoIzLFaMY31F5j2Jq05km7HuvGf3PSFGqDfGzK9strT1B1rsk1jAGok0oI0CLdjCLhSFuOSBfSoSoKN46+xQGbb9bQ4rmUlN2 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On 09/03/2024 07:59, Ryan Roberts wrote: > On 09/03/2024 06:09, Matthew Wilcox wrote: >> On Fri, Mar 08, 2024 at 11:44:35AM +0000, Ryan Roberts wrote: >>>> The thought occurs that we don't need to take the folios off the list. >>>> I don't know that will fix anything, but this will fix your "running out >>>> of memory" problem -- I forgot to drop the reference if folio_trylock() >>>> failed. Of course, I can't call folio_put() inside the lock, so may >>>> as well move the trylock back to the second loop. >> >> I think this was a bad thought ... > > The not-taking-folios-off-the-list thought? Yes, agreed. > >> >>> Dumping all the CPU back traces with gdb, all the cores (except one) are >>> contending on the the deferred split lock. >> >> I'm pretty sure that we can call the shrinker on multiple CPUs at the >> same time (can you confirm from the backtrace?) > > Yes, the vast majority of the CPUs were in deferred_split_scan() waiting for the > split_queue_lock. > >> >> struct pglist_data *pgdata = NODE_DATA(sc->nid); >> struct deferred_split *ds_queue = &pgdata->deferred_split_queue; >> >> so if two CPUs try to shrink the same node, they're going to try to >> process the same set of folios. Which means the split will keep failing >> because each of them will have a refcount on the folio, and ... yeah. > > Ahh, ouch. So this probably explains why things started going slow for me again > last night. > >> >> If so, we need to take the folios off the list (or otherwise mark them) >> so that they can't be processed by more than one CPU at a time. And >> that leads me to this patch (yes, folio_prep_large_rmappable() is >> now vestigial, but removing it increases the churn a bit much for this >> stage of debugging) > > Looks sensible on first review. I'll do some testing now to see if I can > re-triger the non-NULL mapping issue. Will get back to you in the next couple of > hours. > >> >> This time I've boot-tested it. I'm running my usual test-suite against >> it now with little expectation that it will trigger. If I have time >> I'll try to recreate your setup. >> >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >> index fd745bcc97ff..2ca033a6c3d8 100644 >> --- a/mm/huge_memory.c >> +++ b/mm/huge_memory.c >> @@ -792,8 +792,6 @@ void folio_prep_large_rmappable(struct folio *folio) >> { >> if (!folio || !folio_test_large(folio)) >> return; >> - if (folio_order(folio) > 1) >> - INIT_LIST_HEAD(&folio->_deferred_list); >> folio_set_large_rmappable(folio); >> } >> >> @@ -3312,7 +3310,7 @@ static unsigned long deferred_split_scan(struct shrinker *shrink, >> struct pglist_data *pgdata = NODE_DATA(sc->nid); >> struct deferred_split *ds_queue = &pgdata->deferred_split_queue; >> unsigned long flags; >> - LIST_HEAD(list); >> + struct folio_batch batch; >> struct folio *folio, *next; >> int split = 0; >> >> @@ -3321,36 +3319,40 @@ static unsigned long deferred_split_scan(struct shrinker *shrink, >> ds_queue = &sc->memcg->deferred_split_queue; >> #endif >> >> + folio_batch_init(&batch); >> spin_lock_irqsave(&ds_queue->split_queue_lock, flags); >> - /* Take pin on all head pages to avoid freeing them under us */ >> + /* Take ref on all folios to avoid freeing them under us */ >> list_for_each_entry_safe(folio, next, &ds_queue->split_queue, >> _deferred_list) { >> - if (folio_try_get(folio)) { >> - list_move(&folio->_deferred_list, &list); >> - } else { >> + list_del_init(&folio->_deferred_list); >> + sc->nr_to_scan--; >> + if (!folio_try_get(folio)) { >> /* We lost race with folio_put() */ >> - list_del_init(&folio->_deferred_list); >> ds_queue->split_queue_len--; I think split_queue_len is getting out of sync with the number of items on the queue? We only decrement it if we lost the race with folio_put(). But we are unconditionally taking folios off the list here. So we are definitely out of sync until we take the lock again below. But we only put folios back on the list that failed to split. A successful split used to decrement this variable (because the folio was on _a_ list). But now it doesn't. So we are always mismatched after the first failed split? I'll fix this up before I start testing. >> + } else if (folio_batch_add(&batch, folio) == 0) { >> + break; >> } >> - if (!--sc->nr_to_scan) >> + if (!sc->nr_to_scan) >> break; >> } >> spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags); >> >> - list_for_each_entry_safe(folio, next, &list, _deferred_list) { >> + while ((folio = folio_batch_next(&batch)) != NULL) { >> if (!folio_trylock(folio)) >> - goto next; >> - /* split_huge_page() removes page from list on success */ >> + continue; >> if (!split_folio(folio)) >> split++; >> folio_unlock(folio); >> -next: >> - folio_put(folio); >> } >> >> spin_lock_irqsave(&ds_queue->split_queue_lock, flags); >> - list_splice_tail(&list, &ds_queue->split_queue); >> + while ((folio = folio_batch_next(&batch)) != NULL) { >> + if (!folio_test_large(folio)) >> + continue; >> + list_add_tail(&folio->_deferred_list, &ds_queue->split_queue); >> + } >> spin_unlock_irqrestore(&ds_queue->split_queue_lock, flags); >> + folios_put(&batch); >> >> /* >> * Stop shrinker if we didn't split any page, but the queue is empty. >> diff --git a/mm/internal.h b/mm/internal.h >> index 1dfdc3bde1b0..14c21d06f233 100644 >> --- a/mm/internal.h >> +++ b/mm/internal.h >> @@ -432,6 +432,8 @@ static inline void prep_compound_head(struct page *page, unsigned int order) >> atomic_set(&folio->_entire_mapcount, -1); >> atomic_set(&folio->_nr_pages_mapped, 0); >> atomic_set(&folio->_pincount, 0); >> + if (order > 1) >> + INIT_LIST_HEAD(&folio->_deferred_list); >> } >> >> static inline void prep_compound_tail(struct page *head, int tail_idx) >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >> index 025ad1a7df7b..fc9c7ca24c4c 100644 >> --- a/mm/page_alloc.c >> +++ b/mm/page_alloc.c >> @@ -1007,9 +1007,12 @@ static int free_tail_page_prepare(struct page *head_page, struct page *page) >> break; >> case 2: >> /* >> - * the second tail page: ->mapping is >> - * deferred_list.next -- ignore value. >> + * the second tail page: ->mapping is deferred_list.next >> */ >> + if (unlikely(!list_empty(&folio->_deferred_list))) { >> + bad_page(page, "still on deferred list"); >> + goto out; >> + } >> break; >> default: >> if (page->mapping != TAIL_MAPPING) { >> >