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=-18.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_IN_DEF_DKIM_WL 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 9ECBDC433E0 for ; Fri, 5 Mar 2021 19:00:56 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 2D3D1650A3 for ; Fri, 5 Mar 2021 19:00:56 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2D3D1650A3 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id B8D536B006E; Fri, 5 Mar 2021 14:00:55 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id B650A6B0070; Fri, 5 Mar 2021 14:00:55 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A2D6B6B0071; Fri, 5 Mar 2021 14:00:55 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0195.hostedemail.com [216.40.44.195]) by kanga.kvack.org (Postfix) with ESMTP id 893586B006E for ; Fri, 5 Mar 2021 14:00:55 -0500 (EST) Received: from smtpin21.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 4AFA418045C0E for ; Fri, 5 Mar 2021 19:00:55 +0000 (UTC) X-FDA: 77886737670.21.4A5968D Received: from mail-lf1-f49.google.com (mail-lf1-f49.google.com [209.85.167.49]) by imf22.hostedemail.com (Postfix) with ESMTP id 8DDE5C005A2D for ; Fri, 5 Mar 2021 19:00:53 +0000 (UTC) Received: by mail-lf1-f49.google.com with SMTP id b1so5361150lfb.7 for ; Fri, 05 Mar 2021 11:00:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=rBJywJ/55SdNNzhqEcBb4hZL99EvDvjWTnWLT8JPIUw=; b=RoBRx/Ju+scxCBTVd/KvQrJIbS48I+xnjFvh4bTmMOSNIxQ5JM+JiCXo3NaDckbE9M DCZOWmAjHLYlX+7BObWmlV2P6zUj25fU/3weEb+3W5YxaOZgDWeus+AMMuY/jCLgLbH/ Vpi+6D4B/nRB2tiuuyiNPmfxiMVKU5PB6VhT09Ls9yc41FsKwAwBlS1xHhpAjVwHtXkZ NH7VYMRF+Qpbw7xrXRbxVz1JGegIJ1ZstGik6Q8xIWI91LWicEzNIhUkJhwBRiL/9BXU lTjrxy8IvulF0o22LYBTRU4oz2GuDEKwqyQgwfFPXW20DoV+1kPFGXVXJT24zB1F4txg 3J7g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=rBJywJ/55SdNNzhqEcBb4hZL99EvDvjWTnWLT8JPIUw=; b=jZZPbHW/TJGN31NkTBtFf3/lta1PK118j6JhbzLlbMdbGPbb8m1iMUmxGUcyzt3a4W rTMMSC47uxdjW9nF9ux1Sa2XqagWScIphFSdlIK3cBKIzzUCTgjglM7lb13xMeXa4qvK yJJFzt0QfQ0BBri18F87Nw0l3pWfkYOm69l21xnhGfFEYvDo6+BaMFXMxC3g9+bJmK7M ZUfNp94ZJ58DRsXpAxCCVZlfV7C9SUPe60OamFtWVTwrmNUrEbn8OPUMr6j2eoNUEUN/ hBWaSg2LJeqz9OuwU9bfgS5wOdux3u4hIQzDyUNROeEq6pdDSW1RNjSvAPiEMZ8BYrw+ E7qg== X-Gm-Message-State: AOAM531d6j7Gyi7vTj7z4JcdOIreV+jY3Ij007d6xMLjO/75HMBGkZ96 BMc4KaYg6PTqGdq0tgAm5cC7jo7Pv/Q4P6tOhJdfyw== X-Google-Smtp-Source: ABdhPJzcyA4Nh6MPfMONYL+w0cPJ8mMcfZGWjB1dLulIsm91W7L4R/sQAqp324Jf9O1MEs4981U9YCjiZarO5kbHO8g= X-Received: by 2002:ac2:4144:: with SMTP id c4mr6344311lfi.549.1614970852892; Fri, 05 Mar 2021 11:00:52 -0800 (PST) MIME-Version: 1.0 References: <20210304014229.521351-1-shakeelb@google.com> In-Reply-To: From: Shakeel Butt Date: Fri, 5 Mar 2021 11:00:40 -0800 Message-ID: Subject: Re: [PATCH v3] memcg: charge before adding to swapcache on swapin To: Johannes Weiner Cc: Hugh Dickins , Roman Gushchin , Michal Hocko , Andrew Morton , Cgroups , Linux MM , LKML Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: 8DDE5C005A2D X-Stat-Signature: h9nc9t5t4h5cxne8a84ya9kyty8xkqqi Received-SPF: none (google.com>: No applicable sender policy available) receiver=imf22; identity=mailfrom; envelope-from=""; helo=mail-lf1-f49.google.com; client-ip=209.85.167.49 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1614970853-171136 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 5, 2021 at 8:25 AM Johannes Weiner wrote: > > 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. > I will do a coin toss to select one. > > 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. Will remove the page parameter. BTW I will keep mem_cgroup_disabled() check in the uncharge swap function as I am thinking of removing "swapaccount=0" functionality.