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 05FF6C43217 for ; Thu, 1 Dec 2022 15:51:57 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 06C0B6B0071; Thu, 1 Dec 2022 10:51:57 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 01B996B0072; Thu, 1 Dec 2022 10:51:56 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E256E6B0073; Thu, 1 Dec 2022 10:51:56 -0500 (EST) 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 D34EF6B0071 for ; Thu, 1 Dec 2022 10:51:56 -0500 (EST) Received: from smtpin06.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay03.hostedemail.com (Postfix) with ESMTP id ADC89A015E for ; Thu, 1 Dec 2022 15:51:56 +0000 (UTC) X-FDA: 80194178232.06.6529ABA Received: from mail-qt1-f182.google.com (mail-qt1-f182.google.com [209.85.160.182]) by imf15.hostedemail.com (Postfix) with ESMTP id 0E3CDA0014 for ; Thu, 1 Dec 2022 15:51:55 +0000 (UTC) Received: by mail-qt1-f182.google.com with SMTP id jr1so1261196qtb.7 for ; Thu, 01 Dec 2022 07:51:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cmpxchg-org.20210112.gappssmtp.com; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:from:to:cc:subject:date:message-id:reply-to; bh=QZICexc8xsGDU62se8MLzCQs14y/akiq818luXMxAoY=; b=nLdF3MgZ41GcFh+AMHGP6e37c5gGALVjl1T7QxmWOXvPoDybB4EyOcncA+v8e1aQ3y kF43faFVT5i2ogbHi8Xh9fRkjxddevBI+Xa801fZJ6sogb5BSFkZJynRCMCnm5Hcln+z h6ULkbiQ8wFSdz2qqMnvaApw9XIUQJO5Lce4eaS39G3bbtx350i2m055PGe4Y/Q8k7Xk UGBqPrjAwCr73TCtjw2ewuQuCFGRRNKnBzkRxQrFdjLXxArH9Scphip7TWKAeFVRTA7S NukPVM39vrjE/Z0SjdhAePvS10HX5K8//ggATbKtCj25ubtQYITvpperZj74K9mcTju9 lu+Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:x-gm-message-state:from:to:cc:subject:date :message-id:reply-to; bh=QZICexc8xsGDU62se8MLzCQs14y/akiq818luXMxAoY=; b=ev0oJ4TCTx7zxwENtA7R2Gp+o0zEhncMbh99uIitUz9RJxf4rDOCA1S5XEOO/RU0if dEotbHTKEvZgzJfgU7/HGF7nB3avVjkcZgk2lX2dTRnEOlXiW99kxIFvQrhozQFyYtS4 DRyN5fVxslptS3AhsPuZGmq9Z8P+kH7Nv/HOTEliTzyIDEm9BheKlvePiRSPJy4Bsvqv inElFkuR9nIEBsugmZJXZtSsM8S+xFkZGYEp7xZjHHFURSlskOZl3dDmhXObhVvu5y2R 4C+tZNYvFT5Aw1DUwi3HT2TvakJFIH5RxBqXwEv3Q9lhFhg+HdNhnFgu97S3wTCcV5NG 0y3g== X-Gm-Message-State: ANoB5pk+bSChUrYP5CoRrkpxDq/RKtAvIucD3PxWdYrlXx9MoD6SDxfz KZECBzyEBxFc9E8e1/jfmL9ROQ== X-Google-Smtp-Source: AA0mqf6dEkhBqcRQnXdcdHj5BwV21Ho7Twrv7AZM0ULHipx4751l1/9u0cMtl5iGzi6eNruqBR97aw== X-Received: by 2002:ac8:7eed:0:b0:3a5:40ab:5952 with SMTP id r13-20020ac87eed000000b003a540ab5952mr42915558qtc.254.1669909915024; Thu, 01 Dec 2022 07:51:55 -0800 (PST) Received: from localhost (2603-7000-0c01-2716-3663-3884-f85a-44bb.res6.spectrum.com. [2603:7000:c01:2716:3663:3884:f85a:44bb]) by smtp.gmail.com with ESMTPSA id y6-20020a05620a25c600b006b9c9b7db8bsm3767587qko.82.2022.12.01.07.51.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 01 Dec 2022 07:51:54 -0800 (PST) Date: Thu, 1 Dec 2022 10:52:31 -0500 From: Johannes Weiner To: Hugh Dickins Cc: 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 Message-ID: References: <20221123181838.1373440-1-hannes@cmpxchg.org> <16dd09c-bb6c-6058-2b3-7559b5aefe9@google.com> <3659bbe0-ccf2-7feb-5465-b287593aa421@google.com> <33f2f836-98a0-b593-1d43-b289d645db5@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <33f2f836-98a0-b593-1d43-b289d645db5@google.com> ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1669909916; a=rsa-sha256; cv=none; b=wUT2lPrk4TXllGt7qy5yAO2m8DLGXuhXmY71HrFDU69qw9Yl2KwSIpjp3Y9ePJBGA3/Tkg 1K0bqb26za8UOL4VmsKFRqcJLAWrqwrg+UZhgSpagrrvZPl2Z4P5MzSTkvABRIFvFSMv29 pDwEtSp3rVKbFuKItwu+9Cf/DyygemI= ARC-Authentication-Results: i=1; imf15.hostedemail.com; dkim=pass header.d=cmpxchg-org.20210112.gappssmtp.com header.s=20210112 header.b=nLdF3MgZ; dmarc=pass (policy=none) header.from=cmpxchg.org; spf=pass (imf15.hostedemail.com: domain of hannes@cmpxchg.org designates 209.85.160.182 as permitted sender) smtp.mailfrom=hannes@cmpxchg.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1669909916; 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=QZICexc8xsGDU62se8MLzCQs14y/akiq818luXMxAoY=; b=HHCkGL2h0muG8VOgAlJ1qQnfGNVivll+yx46G2qnTjr3zF8B2Jy5vnaE/Phreps+v+AW4E bPpYZ7FGUzGy/QIcwZg9E7LTm4onCMEoVN4FeEyvKWfG04cXZgm9tS0ZW/S5cpAP62dQGn hHnt3Y8Z3wR9QtMTdeUKxySUkL6ErJY= X-Rspamd-Queue-Id: 0E3CDA0014 X-Rspam-User: Authentication-Results: imf15.hostedemail.com; dkim=pass header.d=cmpxchg-org.20210112.gappssmtp.com header.s=20210112 header.b=nLdF3MgZ; dmarc=pass (policy=none) header.from=cmpxchg.org; spf=pass (imf15.hostedemail.com: domain of hannes@cmpxchg.org designates 209.85.160.182 as permitted sender) smtp.mailfrom=hannes@cmpxchg.org X-Rspamd-Server: rspam09 X-Stat-Signature: g5aixsktcheo3ujrj8k47suujehuqmob X-HE-Tag: 1669909915-382911 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, Nov 30, 2022 at 04:13:23PM -0800, Hugh Dickins wrote: > 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? No, it just seemed odd to move shared swap *unless* it's partially faulted. But you're right, it's probably not worth the hassle. I'll cut this down to the page_mapped() check. The struggle of writing code for Schroedinger's User... > > @@ -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. Yep, thanks! > > * 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()?? mem_cgroup_move_account() of course! Will fix. > > + * 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. ... and edit this down accordingly. > > + */ > > + 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. Right, same rules apply there. I only pasted the swap one to make sure we get aligned on the basic strategy. > And matching trylock in mc_handle_present_pte() (and get_mctgt_type_thp()), > instead of in mem_cgroup_move_account(). Yes. > I haven't checked to see where the page then needs to be unlocked, > probably some new places. Yes, the callers of get_mctgt_type*() need to unlock (if target is passed and the page is returned). It looks straight-forward, they already have to do put_page(). > 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. We could make it conditional on target, which precharge doesn't pass, but I agree it's likely not worth optimizing that code at this point. Thanks for taking a look, Hugh, that's excellent input. I'll finish this patch, rebase the rmap patch on it, and add a new one to issue a deprecation warning in mem_cgroup_move_charge_write(). Johannes