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 91AA1C433F5 for ; Tue, 29 Mar 2022 14:00:03 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 1B2F38D0002; Tue, 29 Mar 2022 10:00:03 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 163178D0001; Tue, 29 Mar 2022 10:00:03 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F1E6E8D0002; Tue, 29 Mar 2022 10:00:02 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (relay.hostedemail.com [64.99.140.27]) by kanga.kvack.org (Postfix) with ESMTP id DF4F38D0001 for ; Tue, 29 Mar 2022 10:00:02 -0400 (EDT) Received: from smtpin11.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay12.hostedemail.com (Postfix) with ESMTP id B03E712160A for ; Tue, 29 Mar 2022 14:00:02 +0000 (UTC) X-FDA: 79297582644.11.2CA54EB Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf03.hostedemail.com (Postfix) with ESMTP id ACB0720004 for ; Tue, 29 Mar 2022 14:00:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1648562401; h=from:from: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=/ZqemDiilpXZRsGW64Filc3A1/e3UcE51b6PVnuxGic=; b=d0a54JcknmtIqp5NCxGxQKEuwZ2hUkm5JiidUp63Z2E6H+jd8nWZguTQqYfbIAcW7Yt9kW uqspddwfV5fxJxV3uYEf8n1/6A27tq37hVscDk3Yd6G1V257E1U4IKnXkKUFzuDcMYYytI nQWG/pyI208VP9rrYgcWj3yJ7l/YAng= Received: from mail-wm1-f71.google.com (mail-wm1-f71.google.com [209.85.128.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-246-Yj20AKW_PdiBn6ZPKUuQqA-1; Tue, 29 Mar 2022 09:59:59 -0400 X-MC-Unique: Yj20AKW_PdiBn6ZPKUuQqA-1 Received: by mail-wm1-f71.google.com with SMTP id h127-20020a1c2185000000b0038c6f7e22a4so901304wmh.9 for ; Tue, 29 Mar 2022 06:59:59 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent :content-language:to:cc:references:from:organization:subject :in-reply-to:content-transfer-encoding; bh=/ZqemDiilpXZRsGW64Filc3A1/e3UcE51b6PVnuxGic=; b=ebN5mWeuXSmipYhght2T3SKOOvjOT43oW/ArKhD7JnX0jT+It4Nqt5zunQhzwPgYLT TJqJ0FsANLpN11YSVrDvQGZ76h+AMvv+wlZummMUmnzxw53v9ZRK2qzIziuzrg8h49Hd /miERFwTIFtweEm66xB+aEP068NIbAttQP3J5AzPT0TtBRJvIKk9Yp32wkhhFgCTRTxb MJ7NL7/n+gugvlg5ashIZay1LnDUmzWJIaw6JP7uNTai+H6K7DLwBv5bVodYzX+Zxxyq ws8FO/gQ8s2iJrHdh0P+UUjZ7n0AprsnmS7iuUtp2IgSK/Djp2VjEoqqH5PPN6o6R3fb g0wA== X-Gm-Message-State: AOAM533lULc03dvjL8pa0+RiQawDa7z04gPk54iRCO0pOCTAXMzfVp4z eKKmQF+HlT+41vIllJGj+DWwuApDjWMaI0sMm+iPK9wWfcmftTdA+Yp8jZfRFDqvCbVgJn5GXya s/gkixnd2pzY= X-Received: by 2002:a5d:6c6f:0:b0:204:555:73cf with SMTP id r15-20020a5d6c6f000000b00204055573cfmr30183183wrz.24.1648562398367; Tue, 29 Mar 2022 06:59:58 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwBbJdDFpIw2PVYMOn0HR+uQcrMf86yObuHBVJz108nl3QEF3sbNHSGEVByeUJ9zQeXwtkZSQ== X-Received: by 2002:a5d:6c6f:0:b0:204:555:73cf with SMTP id r15-20020a5d6c6f000000b00204055573cfmr30183121wrz.24.1648562398042; Tue, 29 Mar 2022 06:59:58 -0700 (PDT) Received: from ?IPV6:2003:cb:c708:af00:7a8a:46df:a7c3:c4c7? (p200300cbc708af007a8a46dfa7c3c4c7.dip0.t-ipconnect.de. [2003:cb:c708:af00:7a8a:46df:a7c3:c4c7]) by smtp.gmail.com with ESMTPSA id r15-20020a5d6c6f000000b002040552e88esm17207749wrz.29.2022.03.29.06.59.56 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 29 Mar 2022 06:59:57 -0700 (PDT) Message-ID: Date: Tue, 29 Mar 2022 15:59:55 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.6.2 To: linux-kernel@vger.kernel.org, Khalid Aziz Cc: Andrew Morton , Hugh Dickins , Linus Torvalds , David Rientjes , Shakeel Butt , John Hubbard , Jason Gunthorpe , Mike Kravetz , Mike Rapoport , Yang Shi , "Kirill A . Shutemov" , Matthew Wilcox , Vlastimil Babka , Jann Horn , Michal Hocko , Nadav Amit , Rik van Riel , Roman Gushchin , Andrea Arcangeli , Peter Xu , Donald Dutile , Christoph Hellwig , Oleg Nesterov , Jan Kara , Liang Zhang , Pedro Gomes , Oded Gabbay , linux-mm@kvack.org References: <20220315104741.63071-1-david@redhat.com> <20220315104741.63071-2-david@redhat.com> From: David Hildenbrand Organization: Red Hat Subject: Re: [PATCH v2 01/15] mm/rmap: fix missing swap_free() in try_to_unmap() after arch_unmap_one() failed In-Reply-To: <20220315104741.63071-2-david@redhat.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Rspam-User: X-Stat-Signature: mdtmambymqwr3irnfhc76qe8d46esdi4 Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=d0a54Jck; spf=none (imf03.hostedemail.com: domain of david@redhat.com has no SPF policy when checking 170.10.133.124) smtp.mailfrom=david@redhat.com; dmarc=pass (policy=none) header.from=redhat.com X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: ACB0720004 X-HE-Tag: 1648562401-675021 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: On 15.03.22 11:47, David Hildenbrand wrote: > In case arch_unmap_one() fails, we already did a swap_duplicate(). let's > undo that properly via swap_free(). > > Fixes: ca827d55ebaa ("mm, swap: Add infrastructure for saving page metadata on swap") > Reviewed-by: Khalid Aziz > Signed-off-by: David Hildenbrand > --- > mm/rmap.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/mm/rmap.c b/mm/rmap.c > index 6a1e8c7f6213..f825aeef61ca 100644 > --- a/mm/rmap.c > +++ b/mm/rmap.c > @@ -1625,6 +1625,7 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma, > break; > } > if (arch_unmap_one(mm, vma, address, pteval) < 0) { > + swap_free(entry); > set_pte_at(mm, address, pvmw.pte, pteval); > ret = false; > page_vma_mapped_walk_done(&pvmw); Hi Khalid, I'm a bit confused about the semantics if arch_unmap_one(), I hope you can clarify. See patch #11 in this series, were we can fail unmapping after arch_unmap_one() succeeded. E.g., @@ -1623,6 +1634,24 @@ static bool try_to_unmap_one(struct page *page, struct vm_area_struct *vma, page_vma_mapped_walk_done(&pvmw); break; } + if (anon_exclusive && + page_try_share_anon_rmap(subpage)) { + swap_free(entry); + set_pte_at(mm, address, pvmw.pte, pteval); + ret = false; + page_vma_mapped_walk_done(&pvmw); + break; + } + /* + * Note: We don't remember yet if the page was mapped + * exclusively in the swap entry, so swapin code has + * to re-determine that manually and might detect the + * page as possibly shared, for example, if there are + * other references on the page or if the page is under + * writeback. We made sure that there are no GUP pins + * on the page that would rely on it, so for GUP pins + * this is fine. + */ if (list_empty(&mm->mmlist)) { spin_lock(&mmlist_lock); if (list_empty(&mm->mmlist)) For now, I was under the impression that we don't have to undo anything after arch_unmap_one() succeeded, because we seem to not do anything for two cases below. But looking into arch_unmap_one() and how it allocates stuff I do wonder what we would actually want to do here -- I'd assume we'd want to trigger the del_tag_store() somehow? arch_unmap_one() calls adi_save_tags(), which allocates memory. adi_restore_tags()->del_tag_store() reverts that operation and ends up freeing memory conditionally; However, it's only called from arch_do_swap_page(). Here is why I have to scratch my head: a) arch_do_swap_page() is only called from do_swap_page(). We don't do anything similar for mm/swapfile.c:unuse_pte(), aren't we missing something? b) try_to_migrate_one() does the arch_unmap_one(), but who will do the restore+free after migration succeeded or failed, aren't we missing something? I assume that we won't be properly freeing the tag space in case of a), and won't be properly restoring/migrating the tags in case of a) and b). I'll send out v3 of this series today, and I'll keep ignoring arch_unmap_one() for now, because I have no clue what to do and it looks incomplete already, unless I am missing something important. -- Thanks, David / dhildenb