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=-8.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED 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 21464C433E0 for ; Thu, 4 Mar 2021 15:48:39 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 8C95460230 for ; Thu, 4 Mar 2021 15:48:38 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 8C95460230 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 086956B0008; Thu, 4 Mar 2021 10:48:38 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 05EE66B0010; Thu, 4 Mar 2021 10:48:38 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E69806B0012; Thu, 4 Mar 2021 10:48:37 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0246.hostedemail.com [216.40.44.246]) by kanga.kvack.org (Postfix) with ESMTP id C22D96B0008 for ; Thu, 4 Mar 2021 10:48:37 -0500 (EST) Received: from smtpin14.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 7BC161802E63A for ; Thu, 4 Mar 2021 15:48:37 +0000 (UTC) X-FDA: 77882624274.14.CC00AE0 Received: from mail-qk1-f169.google.com (mail-qk1-f169.google.com [209.85.222.169]) by imf24.hostedemail.com (Postfix) with ESMTP id D7C70A0049EF for ; Thu, 4 Mar 2021 15:48:31 +0000 (UTC) Received: by mail-qk1-f169.google.com with SMTP id a9so13628325qkn.13 for ; Thu, 04 Mar 2021 07:48:33 -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=BPzzifR3jmKfmhbVLe6vXoybjuDrovQTeCnMZN/atRQ=; b=WWef2GAfbou80N5BMcLGOfy0SRUKVjvprQBlt+eQ+fl9znRom4oEqDUkcVMMFRhCsH HsSgPUCsRxtfs2w2UEYQT8vJ7rEWDZ2ACkOnMGQe65sLG88PHpR+1P/F+9hK0PA7JVeG 7UuWOMl76v8wjCcKe7UDA5MFirmoqXx4dzCu/bQ3QKktRLHNXPLldgF4/HqlPEFukYzz vrmARnzD/btz1fvFlMRfqt0RDiFQX5hbc2fi1W0JrT8ItDNEy+F/ItC7C9js8zRa2q9Y Mld0Q+guocOpuvIvtAEx8d0GvDnv1b/zleQfkTOv0IL7WjOCaqoUCmjMVWgUCbcSx4Lv k/RA== 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=BPzzifR3jmKfmhbVLe6vXoybjuDrovQTeCnMZN/atRQ=; b=sjM/yUkzpICCXTRILbbpo+u99JFOZNUhV54uRXuWgYYNsb2QnoldmPQg3vv6Ur4KCL B4Qry/rMSxEAfDz24WA9vie0/pQqsr7rDmFN6l1a7b2TgADJhunFif6ij2OjCLIQsS6F AKEb/SNN5s50vr3jFNhuQUoDS8mrnUPqcpDJFErW24c7SzyEk3ba86FG0HHfv3aQ74qw ph8H7eMVfmCt6rUjikxMxqM3IfevdDX2H2GJuo0wz9STQ52TMiAgvLAg6mZITWlP5VyW 0dXzd8jsZB7L9FbTmIg/9L8elC5Rq9E1++U9/R9lPX4drFpQA4LdqHuF3fNT7HBmik2w UXqQ== X-Gm-Message-State: AOAM532TV5rn3EfrW75RaODExbmezNkPPi8SDBTFkAkJQDyCrO6RrJMu rd9TH5cxpNJ8kwvkWBY0tgwqqQ== X-Google-Smtp-Source: ABdhPJxHWNi0keQbMNoJsGuf4uKSTRrd3e3gnQprSCI2sh41sPnTTTzbepxGjVS3MrOEBSsK107UVg== X-Received: by 2002:a37:4e01:: with SMTP id c1mr4577182qkb.16.1614872912480; Thu, 04 Mar 2021 07:48:32 -0800 (PST) Received: from localhost ([2620:10d:c091:480::1:b28e]) by smtp.gmail.com with ESMTPSA id h3sm15841551qtp.8.2021.03.04.07.48.31 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 04 Mar 2021 07:48:31 -0800 (PST) Date: Thu, 4 Mar 2021 10:48:30 -0500 From: Johannes Weiner To: Shakeel Butt Cc: Hugh Dickins , 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: <20210304014229.521351-1-shakeelb@google.com> X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: D7C70A0049EF X-Stat-Signature: 3ea4ng88r79tdhabnw1t7wezukft6m8t Received-SPF: none (cmpxchg.org>: No applicable sender policy available) receiver=imf24; identity=mailfrom; envelope-from=""; helo=mail-qk1-f169.google.com; client-ip=209.85.222.169 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1614872911-855289 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, Mar 03, 2021 at 05:42:29PM -0800, 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 The patch looks good to me, I have just a minor documentation nit below. But with that addressed, please add: Acked-by: Johannes Weiner > +/** > + * mem_cgroup_charge_swapin_page - charge a newly allocated page for swapin > + * @page: page to charge > + * @mm: mm context of the victim > + * @gfp: reclaim mode > + * @entry: swap entry for which the page is allocated > + * > + * This function marks the start of the transaction of charging the page for > + * swapin. Complete the transaction with mem_cgroup_finish_swapin_page(). > + * > + * Returns 0 on success. Otherwise, an error code is returned. > + */ > +int mem_cgroup_charge_swapin_page(struct page *page, struct mm_struct *mm, > + gfp_t gfp, swp_entry_t entry) > +{ > + struct mem_cgroup *memcg; > + unsigned short id; > + int ret; > > - if (!memcg) > - memcg = get_mem_cgroup_from_mm(mm); > + if (mem_cgroup_disabled()) > + return 0; > > - ret = try_charge(memcg, gfp_mask, nr_pages); > - if (ret) > - goto out_put; > + id = lookup_swap_cgroup_id(entry); > + rcu_read_lock(); > + memcg = mem_cgroup_from_id(id); > + if (!memcg || !css_tryget_online(&memcg->css)) > + memcg = get_mem_cgroup_from_mm(mm); > + rcu_read_unlock(); > > - css_get(&memcg->css); > - commit_charge(page, memcg); > + ret = __mem_cgroup_charge(page, memcg, gfp); > > - local_irq_disable(); > - mem_cgroup_charge_statistics(memcg, page, nr_pages); > - memcg_check_events(memcg, page); > - local_irq_enable(); > + css_put(&memcg->css); > + return ret; > +} > > +/* > + * 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. It's possible somebody later needs to change things around in the swapin path and it's not immediately obvious when exactly these two functions need to be called in the swapin sequence. Maybe add here and above that charge_swapin_page needs to be called before we try adding the page to the swapcache, and finish_swapin_page needs to be called when swapcache insertion has been successful?