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 X-Spam-Level: X-Spam-Status: No, score=-13.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9E8C4C433DB for ; Fri, 5 Mar 2021 16:25:27 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 89F0B64F4E for ; Fri, 5 Mar 2021 16:25:26 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 89F0B64F4E Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=cmpxchg.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 115D96B006C; Fri, 5 Mar 2021 11:25:26 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 0C6296B006E; Fri, 5 Mar 2021 11:25:26 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EA7F36B0070; Fri, 5 Mar 2021 11:25:25 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0251.hostedemail.com [216.40.44.251]) by kanga.kvack.org (Postfix) with ESMTP id C91576B006C for ; Fri, 5 Mar 2021 11:25:25 -0500 (EST) Received: from smtpin24.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 77D1B364F for ; Fri, 5 Mar 2021 16:25:25 +0000 (UTC) X-FDA: 77886345810.24.EEBC3CC Received: from mail-qt1-f169.google.com (mail-qt1-f169.google.com [209.85.160.169]) by imf10.hostedemail.com (Postfix) with ESMTP id 3E36540B8CDF for ; Fri, 5 Mar 2021 16:25:16 +0000 (UTC) Received: by mail-qt1-f169.google.com with SMTP id t13so757710qta.11 for ; Fri, 05 Mar 2021 08:25:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cmpxchg-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=5GBpYtsOToNXALvaP0yVwWut+4euuVYAomBALxtd1po=; b=l5GR7801lJ+iexrPZFrMv2GyBBGWIkMKJrSkR2MfNTsFIDXHUpy3cm/SE8mt8iA8UW tX/fInL970tsjCxnzQF+FRUNwkBYl8ssYwqC0WnbEYVWqVCGsXbQPeWx+VWBu956APq5 mGp7vqfZN32cBBvXjmDpf/1oIPIh2R8ci632Envp0BMPk+oYYwrezZz94yY0QUPCCM5C 0MdWXR5gQwacJqCZHp30/0E3L+uYsM+aSN4osjQSY2P/XcQXN0bZ63NuW5fVyTMeSvMY +wgAS8a9Hke9BxQO5OP3oLzb5rE1rG9b5YRUFu7ogbuZnpaXfViZtSf1xkH9CLZTA469 WDKA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=5GBpYtsOToNXALvaP0yVwWut+4euuVYAomBALxtd1po=; b=VDM2qSt4rsy9t4G842o2Vx7WO3caL0EAtOxRiMiXTds8Mmh/LBn6DWSE9etoqNrqrW bbWl80G4rgYMIkAOJWb8invSaYJnLg/mKoz1K9fyFY2FqE9ASgsj41M9Jz5+9wIqsHxR OoESXgDik2PD+YsMG639AK/O5fAGfCThxqRxdfqJeQ2tWmUe4sFeAvScxU+Z6+OXgmrD +EVNNNMyt53BpFFnxT/txWZ7KfLQ3nWIFD94u/eWRlzmJC5FLJNVS5bvHC9dzuNGNVHu gIpXqRjhWonYD6+jMq1N0xry5IexDldVGsXG3OQVQQ2pISzDUOoOPKrlYdxyrAUQ+eRj rlAw== X-Gm-Message-State: AOAM532X85TMEuynBx5MueKWBLrUzavee7vobMRVN0wyGeiwTaYRAuWx IIcqLBHC9aqz/ijKWEo6Py+FFg== X-Google-Smtp-Source: ABdhPJzIotCPAUdL3jV2ZoIemcaQCaIniPbkJAHU9DuUli2t+rjKeNj1R/Nq2yNi6OIpFFy0yPusmw== X-Received: by 2002:a05:622a:486:: with SMTP id p6mr9500637qtx.81.1614961512027; Fri, 05 Mar 2021 08:25:12 -0800 (PST) Received: from localhost (70.44.39.90.res-cmts.bus.ptd.net. [70.44.39.90]) by smtp.gmail.com with ESMTPSA id a34sm2185343qtc.97.2021.03.05.08.25.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 05 Mar 2021 08:25:11 -0800 (PST) Date: Fri, 5 Mar 2021 11:25:10 -0500 From: Johannes Weiner To: Hugh Dickins Cc: Shakeel Butt , Roman Gushchin , Michal Hocko , Andrew Morton , cgroups@vger.kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3] memcg: charge before adding to swapcache on swapin Message-ID: References: <20210304014229.521351-1-shakeelb@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 3E36540B8CDF X-Stat-Signature: 3wznda84okzeyukq8s44hay4rfccrrwx Received-SPF: none (cmpxchg.org>: No applicable sender policy available) receiver=imf10; identity=mailfrom; envelope-from=""; helo=mail-qt1-f169.google.com; client-ip=209.85.160.169 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1614961516-412319 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 Fri, Mar 05, 2021 at 12:06:31AM -0800, Hugh Dickins wrote: > On Wed, 3 Mar 2021, Shakeel Butt wrote: > > > Currently the kernel adds the page, allocated for swapin, to the > > swapcache before charging the page. This is fine but now we want a > > per-memcg swapcache stat which is essential for folks who wants to > > transparently migrate from cgroup v1's memsw to cgroup v2's memory and > > swap counters. In addition charging a page before exposing it to other > > parts of the kernel is a step in the right direction. > > > > To correctly maintain the per-memcg swapcache stat, this patch has > > adopted to charge the page before adding it to swapcache. One > > challenge in this option is the failure case of add_to_swap_cache() on > > which we need to undo the mem_cgroup_charge(). Specifically undoing > > mem_cgroup_uncharge_swap() is not simple. > > > > To resolve the issue, this patch introduces transaction like interface > > to charge a page for swapin. The function mem_cgroup_charge_swapin_page() > > initiates the charging of the page and mem_cgroup_finish_swapin_page() > > completes the charging process. So, the kernel starts the charging > > process of the page for swapin with mem_cgroup_charge_swapin_page(), > > adds the page to the swapcache and on success completes the charging > > process with mem_cgroup_finish_swapin_page(). > > > > Signed-off-by: Shakeel Butt > > Quite apart from helping with the stat you want, what you've ended > up with here is a nice cleanup in several different ways (and I'm > glad Johannes talked you out of __GFP_NOFAIL: much better like this). > I'll say > > Acked-by: Hugh Dickins > > but I am quite unhappy with the name mem_cgroup_finish_swapin_page(): > it doesn't finish the swapin, it doesn't finish the page, and I'm > not persuaded by your paragraph above that there's any "transaction" > here (if there were, I'd suggest "commit" instead of "finish"'; and > I'd get worried by the css_put before it's called - but no, that's > fine, it's independent). > > How about complementing mem_cgroup_charge_swapin_page() with > mem_cgroup_uncharge_swapin_swap()? I think that describes well > what it does, at least in the do_memsw_account() case, and I hope > we can overlook that it does nothing at all in the other case. Yes, that's better. The sequence is still somewhat mysterious for people not overly familiar with memcg swap internals, but it's much clearer for people who are. I mildly prefer swapping the swapin bit: mem_cgroup_swapin_charge_page() mem_cgroup_swapin_uncharge_swap() But either way works for me. > And it really doesn't need a page argument: both places it's called > have just allocated an order-0 page, there's no chance of a THP here; > but you might have some idea of future expansion, or matching > put_swap_page() - I won't object if you prefer to pass in the page. Agreed. > > + * mem_cgroup_finish_swapin_page - complete the swapin page charge transaction > > + * @page: page charged for swapin > > + * @entry: swap entry for which the page is charged > > + * > > + * This function completes the transaction of charging the page allocated for > > + * swapin. > > + */ > > +void mem_cgroup_finish_swapin_page(struct page *page, swp_entry_t entry) > > +{ > > /* > > * Cgroup1's unified memory+swap counter has been charged with the > > * new swapcache page, finish the transfer by uncharging the swap > > @@ -6760,20 +6796,14 @@ int mem_cgroup_charge(struct page *page, struct mm_struct *mm, gfp_t gfp_mask) > > * correspond 1:1 to page and swap slot lifetimes: we charge the > > * page to memory here, and uncharge swap when the slot is freed. > > */ > > - if (do_memsw_account() && PageSwapCache(page)) { > > - swp_entry_t entry = { .val = page_private(page) }; > > + if (!mem_cgroup_disabled() && do_memsw_account()) { > > I understand why you put that !mem_cgroup_disabled() check in there, > but I have a series of observations on that. > > First I was going to say that it would be better left to > mem_cgroup_uncharge_swap() itself. > > Then I was going to say that I think it's already covered here > by the cgroup_memory_noswap check inside do_memsw_account(). > > Then, going back to mem_cgroup_uncharge_swap(), I realized that 5.8's > 2d1c498072de ("mm: memcontrol: make swap tracking an integral part of > memory control") removed the do_swap_account or cgroup_memory_noswap > checks from mem_cgroup_uncharge_swap() and swap_cgroup_swapon() and > swap_cgroup_swapoff() - so since then we have been allocating totally > unnecessary swap_cgroup arrays when mem_cgroup_disabled() (and > mem_cgroup_uncharge_swap() has worked by reading the zalloced array). > > I think, or am I confused? If I'm right on that, one of us ought to > send another patch putting back, either cgroup_memory_noswap checks > or mem_cgroup_disabled() checks in those three places - I suspect the > static key mem_cgroup_disabled() is preferable, but I'm getting dozy. You're right, that patch was overzealous. The point behind it was to ALWAYS track swap ownership when memcg is enabled, so that even if swap space COUNTING has been disabled, we'll charge pages back to their original owners during swap readahead and on swapoff. Therefor, we have to allocate the arrays even if the user requested cgroup_memory_noswap. But we certainly don't need to allocate and maintain the array when mem_cgroup_disabled() altogether. I'll send a patch to add those back. I'd also rename cgroup_memory_noswap to cgroup_swapaccount - to match the commandline and (hopefully) make a bit clearer what it effects. > Whatever we do with that - and it's really not any business for this > patch - I think you can drop the mem_cgroup_disabled() check from > mem_cgroup_uncharge_swapin_swap(). Yes. do_memsw_account() implies !mem_cgroup_disabled(), as disabling memcg sets cgroup_memory_noswap.