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 6D980C67861 for ; Mon, 8 Apr 2024 10:07:47 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id CA3136B0099; Mon, 8 Apr 2024 06:07:46 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C531D6B009B; Mon, 8 Apr 2024 06:07:46 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B1AF36B009C; Mon, 8 Apr 2024 06:07:46 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id 952756B0099 for ; Mon, 8 Apr 2024 06:07:46 -0400 (EDT) Received: from smtpin22.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id 1C68CA0952 for ; Mon, 8 Apr 2024 10:07:46 +0000 (UTC) X-FDA: 81985938132.22.FED91C2 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf15.hostedemail.com (Postfix) with ESMTP id 24C59A0003 for ; Mon, 8 Apr 2024 10:07:43 +0000 (UTC) Authentication-Results: imf15.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf15.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=1712570864; 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=o1sFHbTgX2VSxIWv6mjV9CpH6+/XSFJBXY5RBY7Y2ik=; b=Yd96y3IyHPoPSYSwphHzGNVND6258sfNFysvO1UypBi7meZPfnFcwVXeBi7/9Yh8xvbC43 uCLWmjXcGHa02IRPbnQU1H8yfzJuISqgJzBsg7URoiMkdqQ1GV2vk8jOBZ7s8MKR9imweU zZ4qBxz2BxrhrPeqgBjlmRbGftfbb1I= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=none; dmarc=pass (policy=none) header.from=arm.com; spf=pass (imf15.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=1712570864; a=rsa-sha256; cv=none; b=ndmZyzlFY25oVkisJhuH8NUQcwzur9fWGe13vDwjfgsLmirq+VD7mLsGSyBm61C7jcc03v QSGN0TFnQY/OtJHSI6wc5tyQ0qhN8CILeWH4bF5Xrx0UKCOy3+I+DuzNEx8HJ4UnYJ6BOY BDEVfE4eHRH4Hv37j2DKy83zvHr9iJw= 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 D8FFD1007; Mon, 8 Apr 2024 03:08:13 -0700 (PDT) Received: from [10.57.73.169] (unknown [10.57.73.169]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 88A3A3F766; Mon, 8 Apr 2024 03:07:41 -0700 (PDT) Message-ID: Date: Mon, 8 Apr 2024 11:07:40 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v6 2/6] mm: swap: free_swap_and_cache_nr() as batched free_swap_and_cache() Content-Language: en-GB To: David Hildenbrand , Andrew Morton , Matthew Wilcox , Huang Ying , Gao Xiang , Yu Zhao , Yang Shi , Michal Hocko , Kefeng Wang , Barry Song <21cnbao@gmail.com>, Chris Li , Lance Yang Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20240403114032.1162100-1-ryan.roberts@arm.com> <20240403114032.1162100-3-ryan.roberts@arm.com> <051052af-3b56-4290-98d3-fd5a1eb11ce1@redhat.com> <73adae65-4429-41d7-bbb6-4c58156060d3@arm.com> <7d3d3c58-1bb1-4d70-9ae3-fa96f6d624d8@redhat.com> From: Ryan Roberts In-Reply-To: <7d3d3c58-1bb1-4d70-9ae3-fa96f6d624d8@redhat.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: 24C59A0003 X-Rspam-User: X-Rspamd-Server: rspam04 X-Stat-Signature: aiox7gg7cdcjsndsmrfdadhtzjmbred5 X-HE-Tag: 1712570863-807761 X-HE-Meta: U2FsdGVkX196/3Cqka5E7AZmSftbf4Ue9RmTywLag03yLcOqs84uw4dcpp9cGg0Ke5z00abLGO7XrK14kqrEi+lZYKv70gxLfi2cFEaqg7BOZs9phSfugxxa129YgbmolhoEw3SYbvF8eNbbnRa/95hbBFsQZISCk5ch4Vp9/4aP5PFFkp4WGA47tqph72Q+Y6ZsuT4Jsj9VDEBGPU5tMd5wgsKsNfZDxEAA14bzct0NloCJGdAKSHbXirV6FRDjQo+bFEIhUieDU8msoW1u85sAo6JRiKKa14sawMJizewyQ9WSalBvv2rVCKTm5w1Vd+Dwq0eApQPUDWXoiUowTYZzUSOSKn7YxkxTIHgUYb2QqYodhnYxekQ+ccuhJwP4IwvEsLHTMqJ7L9ulPNzp7xd73Fb6NuHT77AN/OtK0HZGnAWVkceTFRfb81R6HYiHkWfjWv3dthRTZ5dwM4c+Dm9o2hgddDorKGK4ptvpW3I6YthKBYqjxQbW6PlmEdsuzdDraFSaWWNFQbeYQVaQZoK292hormhHvWZwdS/gBCgRs2HzK7tvLZQLAIFR9qVbHMlXTSpPeOriEz4LPtWsxpy6uiUpyCFhCUNL0i9mLaATWA0DDMBtmytOJqnVowM67FMAnOcnX5NELFIZ8kBQzITY8+3J5WlqrRSn79IIsa6qOORuw6nYl9N+I+gSNm/MaxIRZx1GdL2XkuNF1g4gHGwuu6qhiV8ufSgibojvs8dApbiYNqhPe9pFiY8N4yLvquH9UcFqxCsFL3JtiwITi6Co5UUVG/ARgQsnL4g2LTNeI9gVUgM5XPtN3mm+CDRPhYnCjBPvmK0oVDSQneFxo/jPevBn4pJz+fLAnt8Wv5ZbCH0y6BD9v5z8+jno0GzzOt6ORRfLPh4pFzPBUlguEbBkJFRAwygbZhsu10vgF4aNksfOO4LnZ4Qsig04da6jyZoy1v996Hn0VA8YYl6 L05Q+0qb nXB9Bm6wPrEyN2q5hV3FpbptDrKdmZOqWCqiQze8DBKeMzf2kJgCbNw4IWcQXkwW9lpCEJP/8fLlicu8UmprFhBFTHJ6bsWAmFmIHGE33SEpzzcPH+9owiWkiKtcqFN+bPVCI/GAwy3V+juloQEgQGXp/kRe9HgUXnuA3A6ngoIgebaR1uC6yb4LOaPiXbEb0jQjbYcxwmeDOEAsfy/o5bvzgZH7ahpxbuPm6 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 08/04/2024 10:43, David Hildenbrand wrote: > >>>> + >>>> +/** >>>> + * swap_pte_batch - detect a PTE batch for a set of contiguous swap entries >>>> + * @start_ptep: Page table pointer for the first entry. >>>> + * @max_nr: The maximum number of table entries to consider. >>>> + * @entry: Swap entry recovered from the first table entry. >>>> + * >>>> + * Detect a batch of contiguous swap entries: consecutive (non-present) PTEs >>>> + * containing swap entries all with consecutive offsets and targeting the same >>>> + * swap type. >>>> + * >>> >>> Likely you should document that any swp pte bits are ignored? () >> >> Sorry I don't understand this comment. I thought any non-none, non-present PTE >> was always considered to contain only a "swap entry" and a swap entry consists >> of a "type" and an "offset" only. (and its a special "non-swap" swap entry if >> type > SOME_CONSTANT) Are you saying there are additional fields in the PTE that >> are not part of the swap entry? > > > pte_swp_soft_dirty() > pte_swp_clear_exclusive() > pte_swp_uffd_wp() > > Are PTE bits used for swp PTE. Ahh wow. mind blown. Looks like a massive hack... why not store them in the arch-independent swap entry, rather than have them squat independently in the PTE? OK, my implementation is buggy. I'll re-spin to fix this. > > There is also dirty/young for migration entries, but that's not of a concern > here, because we stop for non_swap_entry(). Looks like these are part of the offset field in the arch-independent swap entry - much cleaner ;-). > >> >> >>> >>>> + * max_nr must be at least one and must be limited by the caller so scanning >>>> + * cannot exceed a single page table. >>>> + * >>>> + * Return: the number of table entries in the batch. >>>> + */ >>>> +static inline int swap_pte_batch(pte_t *start_ptep, int max_nr, >>>> +                 swp_entry_t entry) >>>> +{ >>>> +    const pte_t *end_ptep = start_ptep + max_nr; >>>> +    unsigned long expected_offset = swp_offset(entry) + 1; >>>> +    unsigned int expected_type = swp_type(entry); >>>> +    pte_t *ptep = start_ptep + 1; >>>> + >>>> +    VM_WARN_ON(max_nr < 1); >>>> +    VM_WARN_ON(non_swap_entry(entry)); >>>> + >>>> +    while (ptep < end_ptep) { >>>> +        pte_t pte = ptep_get(ptep); >>>> + >>>> +        if (pte_none(pte) || pte_present(pte)) >>>> +            break; >>>> + >>>> +        entry = pte_to_swp_entry(pte); >>>> + >>>> +        if (non_swap_entry(entry) || >>>> +            swp_type(entry) != expected_type || >>>> +            swp_offset(entry) != expected_offset) >>>> +            break; >>>> + >>>> +        expected_offset++; >>>> +        ptep++; >>>> +    } >>>> + >>>> +    return ptep - start_ptep; >>>> +} >>> >>> Looks very clean :) >>> >>> I was wondering whether we could similarly construct the expected swp PTE and >>> only check pte_same. >>> >>> expected_pte = __swp_entry_to_pte(__swp_entry(expected_type, expected_offset)); >>> >>> ... or have a variant to increase only the swp offset for an existing pte. But >>> non-trivial due to the arch-dependent format. >>> >>> But then, we'd fail on mismatch of other swp pte bits. >> >> Hmm, perhaps I have a misunderstanding regarding "swp pte bits"... >> >>> >>> >>> On swapin, when reusing this function (likely!), we'll might to make sure that >>> the PTE bits match as well. >>> >>> See below regarding uffd-wp. >>> >>> >>>>    #endif /* CONFIG_MMU */ >>>>      void __acct_reclaim_writeback(pg_data_t *pgdat, struct folio *folio, >>>> diff --git a/mm/madvise.c b/mm/madvise.c >>>> index 1f77a51baaac..070bedb4996e 100644 >>>> --- a/mm/madvise.c >>>> +++ b/mm/madvise.c >>>> @@ -628,6 +628,7 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned >>>> long addr, >>>>        struct folio *folio; >>>>        int nr_swap = 0; >>>>        unsigned long next; >>>> +    int nr, max_nr; >>>>          next = pmd_addr_end(addr, end); >>>>        if (pmd_trans_huge(*pmd)) >>>> @@ -640,7 +641,8 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned >>>> long addr, >>>>            return 0; >>>>        flush_tlb_batched_pending(mm); >>>>        arch_enter_lazy_mmu_mode(); >>>> -    for (; addr != end; pte++, addr += PAGE_SIZE) { >>>> +    for (; addr != end; pte += nr, addr += PAGE_SIZE * nr) { >>>> +        nr = 1; >>>>            ptent = ptep_get(pte); >>>>              if (pte_none(ptent)) >>>> @@ -655,9 +657,11 @@ static int madvise_free_pte_range(pmd_t *pmd, unsigned >>>> long addr, >>>>                  entry = pte_to_swp_entry(ptent); >>>>                if (!non_swap_entry(entry)) { >>>> -                nr_swap--; >>>> -                free_swap_and_cache(entry); >>>> -                pte_clear_not_present_full(mm, addr, pte, tlb->fullmm); >>>> +                max_nr = (end - addr) / PAGE_SIZE; >>>> +                nr = swap_pte_batch(pte, max_nr, entry); >>>> +                nr_swap -= nr; >>>> +                free_swap_and_cache_nr(entry, nr); >>>> +                clear_not_present_full_ptes(mm, addr, pte, nr, tlb->fullmm); >>>>                } else if (is_hwpoison_entry(entry) || >>>>                       is_poisoned_swp_entry(entry)) { >>>>                    pte_clear_not_present_full(mm, addr, pte, tlb->fullmm); >>>> diff --git a/mm/memory.c b/mm/memory.c >>>> index 7dc6c3d9fa83..ef2968894718 100644 >>>> --- a/mm/memory.c >>>> +++ b/mm/memory.c >>>> @@ -1637,12 +1637,13 @@ static unsigned long zap_pte_range(struct mmu_gather >>>> *tlb, >>>>                    folio_remove_rmap_pte(folio, page, vma); >>>>                folio_put(folio); >>>>            } else if (!non_swap_entry(entry)) { >>>> -            /* Genuine swap entry, hence a private anon page */ >>>> +            max_nr = (end - addr) / PAGE_SIZE; >>>> +            nr = swap_pte_batch(pte, max_nr, entry); >>>> +            /* Genuine swap entries, hence a private anon pages */ >>>>                if (!should_zap_cows(details)) >>>>                    continue; >>>> -            rss[MM_SWAPENTS]--; >>>> -            if (unlikely(!free_swap_and_cache(entry))) >>>> -                print_bad_pte(vma, addr, ptent, NULL); >>>> +            rss[MM_SWAPENTS] -= nr; >>>> +            free_swap_and_cache_nr(entry, nr); >>>>            } else if (is_migration_entry(entry)) { >>>>                folio = pfn_swap_entry_folio(entry); >>>>                if (!should_zap_folio(details, folio)) >>>> @@ -1665,8 +1666,8 @@ static unsigned long zap_pte_range(struct mmu_gather >>>> *tlb, >>>>                pr_alert("unrecognized swap entry 0x%lx\n", entry.val); >>>>                WARN_ON_ONCE(1); >>>>            } >>>> -        pte_clear_not_present_full(mm, addr, pte, tlb->fullmm); >>>> -        zap_install_uffd_wp_if_needed(vma, addr, pte, 1, details, ptent); >>>> +        clear_not_present_full_ptes(mm, addr, pte, nr, tlb->fullmm); >>> >>> For zap_install_uffd_wp_if_needed(), the uffd-wp bit has to match. >>> >>> zap_install_uffd_wp_if_needed() will use the uffd-wp information in >>> ptent->pteval to make a decision whether to place PTE_MARKER_UFFD_WP markers. >>> >>> On mixture, you either lose some or place too many markers. >> >> What path are you concerned about here? I don't get how what you describe can >> happen? swap_pte_batch() will only give me a batch of actual swap entries and >> actual swap entries don't contain uffd-wp info, IIUC. If the function gets to a >> "non-swap" swap entry, it bails. I thought the uffd-wp info was populated based >> on the VMA state at swap-in? I think you are telling me that it's persisted >> across the swap per-pte? > > Please see zap_install_uffd_wp_if_needed(): > > if (unlikely(pte_swp_uffd_wp_any(pteval))) >     arm_uffd_pte = true; > > The PTEs (swp PTEs to be precise) contain uffd-wp informtation. > > [...] > >>>> +    /* >>>> +     * Short-circuit the below loop if none of the entries had their >>>> +     * reference drop to zero. >>>> +     */ >>>> +    if (!any_only_cache) >>>> +        goto out; >>>>    -        count = __swap_entry_free(p, entry); >>>> -        if (count == SWAP_HAS_CACHE) >>>> -            __try_to_reclaim_swap(p, swp_offset(entry), >>>> +    /* >>>> +     * Now go back over the range trying to reclaim the swap cache. This is >>>> +     * more efficient for large folios because we will only try to reclaim >>>> +     * the swap once per folio in the common case. If we do >>>> +     * __swap_entry_free() and __try_to_reclaim_swap() in the same loop, the >>>> +     * latter will get a reference and lock the folio for every individual >>>> +     * page but will only succeed once the swap slot for every subpage is >>>> +     * zero. >>>> +     */ >>>> +    for (offset = swp_offset(entry); offset < end; offset += nr) { >>>> +        nr = 1; >>>> +        if (READ_ONCE(si->swap_map[offset]) == SWAP_HAS_CACHE) { >>> >>> Here we use READ_ONCE() only, above data_race(). Hmmm. >> >> Yes. I think this is correct. >> >> READ_ONCE() is a "marked access" which KCSAN understands, so it won't complain >> about it. So data_race() isn't required when READ_ONCE() (or WRITE_ONCE()) is >> used. I believe READ_ONCE() is required here because we don't have a lock and we >> want to make sure we read it in a non-tearing manner. >> >> We don't need the READ_ONCE() above since we don't care about the exact value - >> only that it's not 0 (because we should be holding a ref). So do a plain access >> to give the compiler a bit more freedom. But we need to mark that with >> data_race() to stop KCSAN from complaining. > > Okay. >