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 5E131C4321E for ; Thu, 1 Dec 2022 00:13:38 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C15816B0071; Wed, 30 Nov 2022 19:13:37 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id BC5496B0073; Wed, 30 Nov 2022 19:13:37 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A8DA16B0074; Wed, 30 Nov 2022 19:13:37 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 9B0756B0071 for ; Wed, 30 Nov 2022 19:13:37 -0500 (EST) Received: from smtpin12.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 62E47120E09 for ; Thu, 1 Dec 2022 00:13:37 +0000 (UTC) X-FDA: 80191813674.12.0A41F4D Received: from mail-qk1-f171.google.com (mail-qk1-f171.google.com [209.85.222.171]) by imf06.hostedemail.com (Postfix) with ESMTP id ECA8918000E for ; Thu, 1 Dec 2022 00:13:36 +0000 (UTC) Received: by mail-qk1-f171.google.com with SMTP id d7so93040qkk.3 for ; Wed, 30 Nov 2022 16:13:36 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:message-id:in-reply-to:subject:cc:to:from :date:from:to:cc:subject:date:message-id:reply-to; bh=aHV8O4q/YJYqi3y8OK2heA1MTuVYBE9Z71+HHXjgjL4=; b=IdVVPMRbeJD3ERLIYatf7Qhl7gSloz9FAjQA5VPxurI7mOMGOlRe1SmMkxP8nXen+h ctCSsHsndDMVyouwDaQm0v7TXfHq0aby4aifQ/E+OXRoPNWWRgh40xnSMefoKPKxsmiQ ZtIHvUylWyyCoco0II+oxM9XH6gfMt1tybat6VJivkyIA44CQk1UoG15QByOzCWQKUUR 2w4ENrKjCgXgpRcCgsQFoYOFzLSHwZSBMSIxZC5+Qjkn+HaH+uJfn1bOX7w8JWghI2MW 3oH9d2vIdfEElOxXDK/quYJ7tUiZOVe1aS7CsaLh9UpRs/BE+dX7qVyUmLRqtQSSj0EE N5TA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=mime-version:references:message-id:in-reply-to:subject:cc:to:from :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=aHV8O4q/YJYqi3y8OK2heA1MTuVYBE9Z71+HHXjgjL4=; b=34AkWsoxvNYi4VX5hJ8N5+ZIXZZvD5FCJpUR09DjoXwmI8ubz1FmQIxlA3D6GVajc8 XzH2W0xU6kLfNBnqFkVWVmntbCv2p9QOVjVkMCw+JMJiuEf2lntFdWrc+7Rgdm5bPrp2 hA8NuCA32mJKKaYFQIjaP9peyubVKnxnS2UHwWZ8BftYFxhHNCWY+I8ZW2ZhN7HpKcR/ l9l8rrQGMMWr+qBTUCt/5WNzGTMCrAAnjVdnkXeKgXT4/SOyC7HOZt71wv5lZSM1o+7Z 2bWdNFJk/pj64CpttGE4sUPliPiIaoqi7fBAG4WMJKWcjjZF/Nxv6hOGugfo2rBZQVBD aekQ== X-Gm-Message-State: ANoB5pkzNn5iNrNRPzqZ85FzHBOHqbofgp91oXQFh0RpTeknzqWzV/f8 AwRUuz8odYdJMq+OKKBZY1nLvw== X-Google-Smtp-Source: AA0mqf6TePEUGMD3StSeLumfJ10Eo16EklOUBHLtB4+ZnlRfBXSytT3eYYPo1hFoETQOHA+hUimEow== X-Received: by 2002:a37:34f:0:b0:6fc:a68e:43ec with SMTP id 76-20020a37034f000000b006fca68e43ecmr185704qkd.436.1669853616057; Wed, 30 Nov 2022 16:13:36 -0800 (PST) Received: from ripple.attlocal.net (172-10-233-147.lightspeed.sntcca.sbcglobal.net. [172.10.233.147]) by smtp.gmail.com with ESMTPSA id s12-20020a05620a0bcc00b006ee7e223bb8sm2328277qki.39.2022.11.30.16.13.34 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 30 Nov 2022 16:13:35 -0800 (PST) Date: Wed, 30 Nov 2022 16:13:23 -0800 (PST) From: Hugh Dickins X-X-Sender: hugh@ripple.attlocal.net To: Johannes Weiner cc: Hugh Dickins , Shakeel Butt , Andrew Morton , Linus Torvalds , Michal Hocko , linux-mm@kvack.org, cgroups@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mm: remove lock_page_memcg() from rmap In-Reply-To: Message-ID: <33f2f836-98a0-b593-1d43-b289d645db5@google.com> References: <20221123181838.1373440-1-hannes@cmpxchg.org> <16dd09c-bb6c-6058-2b3-7559b5aefe9@google.com> <3659bbe0-ccf2-7feb-5465-b287593aa421@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1669853617; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=aHV8O4q/YJYqi3y8OK2heA1MTuVYBE9Z71+HHXjgjL4=; b=BG/zpWn8wZg/BhYSjkL8sKQIZgVQJXGD27F7UbhbVSIeUICsDU4Xu8tVROzmeiGPH3gGpo WJ9YrImy2Go163Esh8/0Cnmzx7oE9Cuik91J0+XUbyEjsAkcot0Kqo1tXw4ylVXLF5tKVa FxkF4SWO4+8OH/PgYhoa564MvibfuI8= ARC-Authentication-Results: i=1; imf06.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=IdVVPMRb; spf=pass (imf06.hostedemail.com: domain of hughd@google.com designates 209.85.222.171 as permitted sender) smtp.mailfrom=hughd@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1669853617; a=rsa-sha256; cv=none; b=gKE9yFwcT+nC/YCwisztJ9QgdkY83MZqvyvJKac54udOw7Dt7o4qezn553pbs9sk0c2wuG qWeG5RKjjxv/5p0zIK6D9iUCsU9jItq6t0RuFiLGrICozXK7ecqD2aaNwJD3bLeH+umqH0 uujpumMUQYz1zWmMH6OBI084qlTWaFA= X-Stat-Signature: 6zj3jyuq6965ozyn3fix9d4tanfxpncc X-Rspam-User: X-Rspamd-Queue-Id: ECA8918000E X-Rspamd-Server: rspam11 Authentication-Results: imf06.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=IdVVPMRb; spf=pass (imf06.hostedemail.com: domain of hughd@google.com designates 209.85.222.171 as permitted sender) smtp.mailfrom=hughd@google.com; dmarc=pass (policy=reject) header.from=google.com X-HE-Tag: 1669853616-338468 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 Wed, 30 Nov 2022, Johannes Weiner wrote: > > Hm, I think the below should work for swap pages. Do you see anything > obviously wrong with it, or scenarios I haven't considered? > I think you're overcomplicating it, with the __swap_count(ent) business, and consequent unnecessarily detailed comments on the serialization. Page/folio lock prevents a !page_mapped(page) becoming a page_mapped(page), whether it's in swap cache or in file cache; it does not stop the sharing count going further up, or down even to 0, but we just don't need to worry about that sharing count - the MC_TARGET_PAGE case does not reject pages with mapcount > 1, so why complicate the swap or file case in that way? (Yes, it can be argued that all such sharing should be rejected; but we didn't come here to argue improvements to memcg charge moving semantics: just to minimize its effect on rmap, before it is fully deprecated.) Or am I missing the point of why you add that complication? > @@ -5637,6 +5645,46 @@ static struct page *mc_handle_swap_pte(struct vm_area_struct *vma, Don't forget to trylock the page in the device_private case before this. > * we call find_get_page() with swapper_space directly. > */ > page = find_get_page(swap_address_space(ent), swp_offset(ent)); > + > + /* > + * Don't move shared charges. This isn't just for saner move > + * semantics, it also ensures that page_mapped() is stable for > + * the accounting in mem_cgroup_mapcount(). mem_cgroup_mapcount()?? > + * > + * We have to serialize against the following paths: fork > + * (which may copy a page map or a swap pte), fault (which may > + * change a swap pte into a page map), unmap (which may cause > + * a page map or a swap pte to disappear), and reclaim (which > + * may change a page map into a swap pte). > + * > + * - Without swapcache, we only want to move the charge if > + * there are no other swap ptes. With the pte lock, the > + * swapcount is stable against all of the above scenarios > + * when it's 1 (our pte), which is the case we care about. > + * > + * - When there is a page in swapcache, we only want to move > + * charges when neither the page nor the swap entry are > + * mapped elsewhere. The pte lock prevents our pte from > + * being forked or unmapped. The page lock will stop faults > + * against, and reclaim of, the swapcache page. So if the > + * page isn't mapped, and the swap count is 1 (our pte), the > + * test results are stable and the charge is exclusive. > + */ > + if (!page && __swap_count(ent) != 1) > + return NULL; > + > + if (page) { > + if (!trylock_page(page)) { > + put_page(page); > + return NULL; > + } > + if (page_mapped(page) || __swap_count(ent) != 1) { > + unlock_page(page); > + put_page(page); > + return NULL; > + } > + } > + > entry->val = ent.val; > > return page; Looks right, without the __swap_count() additions and swap count comments. And similar code in mc_handle_file_pte() - or are you saying that only swap should be handled this way? I would disagree. And matching trylock in mc_handle_present_pte() (and get_mctgt_type_thp()), instead of in mem_cgroup_move_account(). I haven't checked to see where the page then needs to be unlocked, probably some new places. And I don't know what will be best for the preliminary precharge pass: doesn't really want the page lock at all, but it may be unnecessary complication to avoid taking it then unlocking it in that pass. Hugh