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 2D6E4C54E58 for ; Mon, 11 Mar 2024 15:07:39 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id A72DD6B00B3; Mon, 11 Mar 2024 11:07:38 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 9FB896B00B5; Mon, 11 Mar 2024 11:07:38 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 89BF56B00B6; Mon, 11 Mar 2024 11:07:38 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0011.hostedemail.com [216.40.44.11]) by kanga.kvack.org (Postfix) with ESMTP id 701E56B00B3 for ; Mon, 11 Mar 2024 11:07:38 -0400 (EDT) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay01.hostedemail.com (Postfix) with ESMTP id 3502D1C0D25 for ; Mon, 11 Mar 2024 15:07:38 +0000 (UTC) X-FDA: 81885087396.04.E36DA24 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf27.hostedemail.com (Postfix) with ESMTP id 6EEBD4001B for ; Mon, 11 Mar 2024 15:07:36 +0000 (UTC) Authentication-Results: imf27.hostedemail.com; dkim=none; spf=pass (imf27.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1710169656; a=rsa-sha256; cv=none; b=a/YsQTK82QgsUqpewjg24zta9U3/gnWiCdcQ2pGmkFuklfdFV8+xdd/nIk4MbaFWYg2rLw 4NK5dQ12tG/sg8POYNqH0pYmtwcca2//vIwxh5Mrp1TgrpWW+CPQWB1NGgB+ceIe2WzEln 6FV6LTDzrGJy69p0J9SLnW3eJeZQAlM= ARC-Authentication-Results: i=1; imf27.hostedemail.com; dkim=none; spf=pass (imf27.hostedemail.com: domain of ryan.roberts@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=ryan.roberts@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1710169656; 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=Ei376N/iyhlft+fOz2XmrQ9fGiluWnpXFu8PSAnffI8=; b=tGJxvPSCu4nOSnkWeo2wEQpC0TJX2Eb/GkU59iGjHpMl7i5TJ864rL/HBwg9tmhiQ4pcG1 CI9dQhPFq94CSK9/CJTUXYf3rPUSvPiKBWjCDYm0b1mJC//jktArmOIyREnW06nmf1graT q5zDiVhTDLU13/4yRMe4lM5FxxJ3EI8= 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 253C8FEC; Mon, 11 Mar 2024 08:08:12 -0700 (PDT) Received: from [10.57.68.246] (unknown [10.57.68.246]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 49CBA3F64C; Mon, 11 Mar 2024 08:07:32 -0700 (PDT) Message-ID: <4090ae12-8fb9-4e58-a093-86c13cca1d47@arm.com> Date: Mon, 11 Mar 2024 15:07:30 +0000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 1/1] mm/madvise: enhance lazyfreeing with mTHP in madvise_free Content-Language: en-GB From: Ryan Roberts To: Barry Song <21cnbao@gmail.com>, Lance Yang , david@redhat.com, Vishal Moola Cc: akpm@linux-foundation.org, zokeefe@google.com, shy828301@gmail.com, mhocko@suse.com, fengwei.yin@intel.com, xiehuan09@gmail.com, wangkefeng.wang@huawei.com, songmuchun@bytedance.com, peterx@redhat.com, minchan@kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20240307061425.21013-1-ioworker0@gmail.com> <03458c20-5544-411b-9b8d-b4600a9b802f@arm.com> In-Reply-To: <03458c20-5544-411b-9b8d-b4600a9b802f@arm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspam-User: X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 6EEBD4001B X-Stat-Signature: 7jzwz39ah4qt4gou39bh4xpqu4i1k7qx X-HE-Tag: 1710169656-248752 X-HE-Meta: U2FsdGVkX19nOVg8ZGuXg6yGFuUO4x8iQ0KskTxVOtUH+GzftyEZ02ixlaUX28ftPaO84vq3++1/xgmcvL83/KX4IvftqslCaLY1ULKG/p8mMlNKYud6emYOboU2MtFXI82wx4ObwV8yjR5bWvi5/OHtygGOQPtSl9wkeNhdHu5MAixxmiD+tD75qZXA7L1/6R/POSRvEkDhOJKjFJkTG0byCx+SUBv3dRb3FaeEgiPu4FePadQzNNIqhwsKcAm+Hr1pe2q2tWE5Lf7hOHG3Usq121Y+NINvHgUXeOaylQ9MPCW/TkMMG3EkKbStoZor2LX6pavLZ04Eo/8GlStPaaF4hHFQwzBUrCiqT/xcR6y7SHWkk7IUmT0YgIYQKSbpcsOpdoZBzmLfRZJ1ou0jmWeU1eB+cW6NYTyvNkCIuHEn4ySEYTiQb578Zz6FNBvyOefsqmsw5kL1fUluI8dR/4BnaupF/NLNHfqch5rjxJe8PIdJTl+rvVIvqSYqEsZaeVsSmIfwcG/bGOH1czivd5fWYmuiN7LnKjz3iYJBVuFDvHEna19+obV7kpnFQtSMgTHgfp8MPrXUYA2WBorfW2FBk6rA1EQJLJzq1pudRhykk8U+NOfv2jk3XfDEfMnEEE7ZfaI5bCc1vnwqUftqx3ntUPI46dc8ktYlSkMoBVXrRoFDPop954K97avoxPvh7h8vT+BcsTjH5POS4BMaBJG/NFe85gduRGACHoCRYgbZKb837/pxQO/nbYsw9TE1cMwa9TxcDJfKnRHWz3ASHf48DjjhQwQtdrshogdDwEXBOyunRA4wJgx44ZhdfZpzTq3P79qVoX2KaT9mc3VjUSWi56UtPra65iYW1TRvXlhQgxi9XGY+hiZSr3sYBY55AiDoj8Wgzdiwc9JeVLt2wJ+ztm0FuWswb0WuqlXy75QA5Kft7+wQpm875f7WP8bPCF+nnI307B7WD6mhI4z YvA== 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 07/03/2024 09:07, Ryan Roberts wrote: > On 07/03/2024 08:10, Barry Song wrote: >> On Thu, Mar 7, 2024 at 9:00 PM Lance Yang wrote: >>> >>> Hey Barry, >>> >>> Thanks for taking time to review! >>> >>> On Thu, Mar 7, 2024 at 3:00 PM Barry Song <21cnbao@gmail.com> wrote: >>>> >>>> On Thu, Mar 7, 2024 at 7:15 PM Lance Yang wrote: >>>>> >>> [...] >>>>> +static inline bool can_mark_large_folio_lazyfree(unsigned long addr, >>>>> + struct folio *folio, pte_t *start_pte) >>>>> +{ >>>>> + int nr_pages = folio_nr_pages(folio); >>>>> + fpb_t flags = FPB_IGNORE_DIRTY | FPB_IGNORE_SOFT_DIRTY; >>>>> + >>>>> + for (int i = 0; i < nr_pages; i++) >>>>> + if (page_mapcount(folio_page(folio, i)) != 1) >>>>> + return false; >>>> >>>> we have moved to folio_estimated_sharers though it is not precise, so >>>> we don't do >>>> this check with lots of loops and depending on the subpage's mapcount. >>> >>> If we don't check the subpage’s mapcount, and there is a cow folio associated >>> with this folio and the cow folio has smaller size than this folio, >>> should we still >>> mark this folio as lazyfree? >> >> I agree, this is true. However, we've somehow accepted the fact that >> folio_likely_mapped_shared >> can result in false negatives or false positives to balance the >> overhead. So I really don't know :-) >> >> Maybe David and Vishal can give some comments here. >> >>> >>>> BTW, do we need to rebase our work against David's changes[1]? >>>> [1] https://lore.kernel.org/linux-mm/20240227201548.857831-1-david@redhat.com/ >>> >>> Yes, we should rebase our work against David’s changes. >>> >>>> >>>>> + >>>>> + return nr_pages == folio_pte_batch(folio, addr, start_pte, >>>>> + ptep_get(start_pte), nr_pages, flags, NULL); >>>>> +} >>>>> + >>>>> static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr, >>>>> unsigned long end, struct mm_walk *walk) >>>>> >>>>> @@ -676,11 +690,45 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr, >>>>> */ >>>>> if (folio_test_large(folio)) { >>>>> int err; >>>>> + unsigned long next_addr, align; >>>>> >>>>> - if (folio_estimated_sharers(folio) != 1) >>>>> - break; >>>>> - if (!folio_trylock(folio)) >>>>> - break; >>>>> + if (folio_estimated_sharers(folio) != 1 || >>>>> + !folio_trylock(folio)) >>>>> + goto skip_large_folio; >>>> >>>> >>>> I don't think we can skip all the PTEs for nr_pages, as some of them might be >>>> pointing to other folios. >>>> >>>> for example, for a large folio with 16PTEs, you do MADV_DONTNEED(15-16), >>>> and write the memory of PTE15 and PTE16, you get page faults, thus PTE15 >>>> and PTE16 will point to two different small folios. We can only skip when we >>>> are sure nr_pages == folio_pte_batch() is sure. >>> >>> Agreed. Thanks for pointing that out. >>> >>>> >>>>> + >>>>> + align = folio_nr_pages(folio) * PAGE_SIZE; >>>>> + next_addr = ALIGN_DOWN(addr + align, align); >>>>> + >>>>> + /* >>>>> + * If we mark only the subpages as lazyfree, or >>>>> + * cannot mark the entire large folio as lazyfree, >>>>> + * then just split it. >>>>> + */ >>>>> + if (next_addr > end || next_addr - addr != align || >>>>> + !can_mark_large_folio_lazyfree(addr, folio, pte)) >>>>> + goto split_large_folio; >>>>> + >>>>> + /* >>>>> + * Avoid unnecessary folio splitting if the large >>>>> + * folio is entirely within the given range. >>>>> + */ >>>>> + folio_clear_dirty(folio); >>>>> + folio_unlock(folio); >>>>> + for (; addr != next_addr; pte++, addr += PAGE_SIZE) { >>>>> + ptent = ptep_get(pte); >>>>> + if (pte_young(ptent) || pte_dirty(ptent)) { >>>>> + ptent = ptep_get_and_clear_full( >>>>> + mm, addr, pte, tlb->fullmm); >>>>> + ptent = pte_mkold(ptent); >>>>> + ptent = pte_mkclean(ptent); >>>>> + set_pte_at(mm, addr, pte, ptent); >>>>> + tlb_remove_tlb_entry(tlb, pte, addr); >>>>> + } >>>> >>>> Can we do this in batches? for a CONT-PTE mapped large folio, you are unfolding >>>> and folding again. It seems quite expensive. > > I'm not convinced we should be doing this in batches. We want the initial > folio_pte_batch() to be as loose as possible regarding permissions so that we > reduce our chances of splitting folios to the min. (e.g. ignore SW bits like > soft dirty, etc). I think it might be possible that some PTEs are RO and other > RW too (e.g. due to cow - although with the current cow impl, probably not. But > its fragile to assume that). Anyway, if we do an initial batch that ignores all > that then do this bit as a batch, you will end up smeering all the ptes with > whatever properties were set on the first pte, which probably isn't right. > > I've done a similar conversion for madvise_cold_or_pageout_pte_range() as part > of my swap-out series v4 (hoping to post imminently, but still working out a > latent bug that it triggers). I use ptep_test_and_clear_young() in that, which > arm64 can apply per-pte but avoid doing a contpte unfold/fold. I know you have > to clear dirty here too, but I think this pattern is preferable. > > FYI, my swap-out series also halfway-batches madvise_free_pte_range() so that I > can batch free_swap_and_cache() for the swap entry case. Ideally the work you > are doing here would be rebased on top of that and plug-in to the approach > implemented there. (subject to others' views of course). > > I'll cc you when I post it. I just sent out the swap-out series v4, as I presed the button I realized I forgot to cc you - sorry about that! It's at [1]. Patch 2 and 6 are the interesting ones from this PoV. [1] https://lore.kernel.org/linux-mm/20240311150058.1122862-1-ryan.roberts@arm.com/ > >>> >>> Thanks for your suggestion. I'll do this in batches in v3. >>> >>> Thanks again for your time! >>> >>> Best, >>> Lance >>> >>>> >>>>> + } >>>>> + folio_mark_lazyfree(folio); >>>>> + goto next_folio; >>>>> + >>>>> +split_large_folio: >>>>> folio_get(folio); >>>>> arch_leave_lazy_mmu_mode(); >>>>> pte_unmap_unlock(start_pte, ptl); >>>>> @@ -688,13 +736,28 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned long addr, >>>>> err = split_folio(folio); >>>>> folio_unlock(folio); >>>>> folio_put(folio); >>>>> - if (err) >>>>> - break; >>>>> - start_pte = pte = >>>>> - pte_offset_map_lock(mm, pmd, addr, &ptl); >>>>> - if (!start_pte) >>>>> - break; >>>>> - arch_enter_lazy_mmu_mode(); >>>>> + >>>>> + /* >>>>> + * If the large folio is locked or cannot be split, >>>>> + * we just skip it. >>>>> + */ >>>>> + if (err) { >>>>> +skip_large_folio: >>>>> + if (next_addr >= end) >>>>> + break; >>>>> + pte += (next_addr - addr) / PAGE_SIZE; >>>>> + addr = next_addr; >>>>> + } >>>>> + >>>>> + if (!start_pte) { >>>>> + start_pte = pte = pte_offset_map_lock( >>>>> + mm, pmd, addr, &ptl); >>>>> + if (!start_pte) >>>>> + break; >>>>> + arch_enter_lazy_mmu_mode(); >>>>> + } >>>>> + >>>>> +next_folio: >>>>> pte--; >>>>> addr -= PAGE_SIZE; >>>>> continue; >>>>> -- >>>>> 2.33.1 >>>>> >> >> Thanks >> Barry >