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_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 46BCFC433E0 for ; Fri, 12 Mar 2021 09:23:37 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id AF27264FE2 for ; Fri, 12 Mar 2021 09:23:36 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org AF27264FE2 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=bytedance.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 347AF8D0340; Fri, 12 Mar 2021 04:23:36 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 31DF68D0317; Fri, 12 Mar 2021 04:23:36 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 1BF698D0340; Fri, 12 Mar 2021 04:23:36 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0165.hostedemail.com [216.40.44.165]) by kanga.kvack.org (Postfix) with ESMTP id F2A158D0317 for ; Fri, 12 Mar 2021 04:23:35 -0500 (EST) Received: from smtpin16.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id B1C1175A9 for ; Fri, 12 Mar 2021 09:23:35 +0000 (UTC) X-FDA: 77910684390.16.1EF015D Received: from mail-pl1-f174.google.com (mail-pl1-f174.google.com [209.85.214.174]) by imf05.hostedemail.com (Postfix) with ESMTP id E230AE005F00 for ; Fri, 12 Mar 2021 09:23:33 +0000 (UTC) Received: by mail-pl1-f174.google.com with SMTP id j6so11643103plx.6 for ; Fri, 12 Mar 2021 01:23:34 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=26qB14f5N7ohF75H9aqLLsvkoHToTEccxcW0Z7uEKNg=; b=Aq9/6K3/OGRFRR7T2Vg9gTchrRkRQKbxs7ILwsSYD5WejwbSLDgRg8cVGez4VEPmdC m8nvU79XBqNllKmGqYyGzkz1/jqs0txwvK/UUMtkt8ENjkT9+3eh40bDDXb6jqG4gSoZ x1EHLFHZJ3duIbWIsj4mHHcbLx4jo6oX/f/DJAhX3PBwnWnuh0/yV2c4aUtuSCzxowKW kN57G2QkrNGNIJd/gAkWXIw7CWiyTmoyDnLnOVf93G7SbBbQGBttzfT3guxupI9mlGd7 nk/L9cFS1GJT5ZHABevqF970v8uS4XeCoQlDVe30OPjm5d/fNzkJBh8bGi7JEd0X63Ks Afqg== 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=26qB14f5N7ohF75H9aqLLsvkoHToTEccxcW0Z7uEKNg=; b=ZXRQJMz4y7fEazzdkQMaJeBETBOKZvnGBZCpzSxOFNQVJt5uB4V2MMaljvL9pff2a8 1IuQjLCWqYhc4tY8iqLcAMlNUp2bunXUJYNs5KsxyDOHrMi0wUzf/xut9uSyz+rbMeWC zoJjyPc1JmKgsS6u2FScB5/zvj8BbHAhouxsR5fOhV+hF6/lU+uzB92ng01Pu2d2Mdgm JlDts3FFlxspS8jx/A4p7oDp3EBLCdf+8ujDdM4F0q4wT37Jem1yDEY37ihTutEkKy3Z cbFGOVXi2dKUux6VkZy+fEiT09tRFI4d5JAq/i82ey2INduewZDE8Q6Up+mleBAecwT4 2W6Q== X-Gm-Message-State: AOAM531IItT44U8K8YpVgml2u3RKyLnjggcUx+EDews5hHCApy2zW2VT apKKhkZdoohAwyJUhpPG5XAAlGT6J6n25zoCLUBMCg== X-Google-Smtp-Source: ABdhPJx0DK9TvoXxbSqFH38Wn+o6fmv5Y9ZFBPATQS67rK5YLTb42JGEE+TPk0FyHD+43S3ZlQQYVTAPLo81O2rjDW8= X-Received: by 2002:a17:90a:f008:: with SMTP id bt8mr13547708pjb.13.1615541013335; Fri, 12 Mar 2021 01:23:33 -0800 (PST) MIME-Version: 1.0 References: <20210309100717.253-1-songmuchun@bytedance.com> <20210309100717.253-4-songmuchun@bytedance.com> In-Reply-To: From: Muchun Song Date: Fri, 12 Mar 2021 17:22:55 +0800 Message-ID: Subject: Re: [External] Re: [PATCH v3 3/4] mm: memcontrol: use obj_cgroup APIs to charge kmem pages To: Johannes Weiner Cc: Roman Gushchin , Michal Hocko , Andrew Morton , Shakeel Butt , Vladimir Davydov , LKML , Linux Memory Management List , Xiongchun duan Content-Type: text/plain; charset="UTF-8" X-Stat-Signature: f1beeiadmpe6y64bit65isnwm4zhzwkq X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: E230AE005F00 Received-SPF: none (bytedance.com>: No applicable sender policy available) receiver=imf05; identity=mailfrom; envelope-from=""; helo=mail-pl1-f174.google.com; client-ip=209.85.214.174 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1615541013-752417 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 Thu, Mar 11, 2021 at 6:05 AM Johannes Weiner wrote: > > Hello Munchun, > > On Tue, Mar 09, 2021 at 06:07:16PM +0800, Muchun Song wrote: > > @@ -6806,11 +6823,23 @@ static inline void uncharge_gather_clear(struct uncharge_gather *ug) > > static void uncharge_batch(const struct uncharge_gather *ug) > > { > > unsigned long flags; > > + unsigned long nr_pages; > > > > - if (!mem_cgroup_is_root(ug->memcg)) { > > - page_counter_uncharge(&ug->memcg->memory, ug->nr_pages); > > + /* > > + * The kmem pages can be reparented to the root memcg, in > > + * order to prevent the memory counter of root memcg from > > + * increasing indefinitely. We should decrease the memory > > + * counter when unchange. > > + */ > > + if (mem_cgroup_is_root(ug->memcg)) > > + nr_pages = ug->nr_kmem; > > + else > > + nr_pages = ug->nr_pages; > > Correct or not, I find this unreadable. We're uncharging nr_kmem on > the root, and nr_pages against leaf groups? > > It implies several things that might not be immediately obvious to the > reader of this function. Namely, that nr_kmem is a subset of nr_pages. > Or that we don't *want* to account LRU pages for the root cgroup. > > The old code followed a very simple pattern: the root memcg's page > counters aren't touched. > > This is no longer true: we modify them depending on very specific > circumstances. But that's too clever for the stupid uncharge_batch() > which is only supposed to flush a number of accumulators into their > corresponding page counters. > > This distinction really needs to be moved down to uncharge_page() now. OK. I will rework the code here to make it readable. > > > @@ -6828,7 +6857,7 @@ static void uncharge_batch(const struct uncharge_gather *ug) > > > > static void uncharge_page(struct page *page, struct uncharge_gather *ug) > > { > > - unsigned long nr_pages; > > + unsigned long nr_pages, nr_kmem; > > struct mem_cgroup *memcg; > > > > VM_BUG_ON_PAGE(PageLRU(page), page); > > @@ -6836,34 +6865,44 @@ static void uncharge_page(struct page *page, struct uncharge_gather *ug) > > if (!page_memcg_charged(page)) > > return; > > > > + nr_pages = compound_nr(page); > > /* > > * Nobody should be changing or seriously looking at > > - * page memcg at this point, we have fully exclusive > > - * access to the page. > > + * page memcg or objcg at this point, we have fully > > + * exclusive access to the page. > > */ > > - memcg = page_memcg_check(page); > > + if (PageMemcgKmem(page)) { > > + struct obj_cgroup *objcg; > > + > > + objcg = page_objcg(page); > > + memcg = obj_cgroup_memcg_get(objcg); > > + > > + page->memcg_data = 0; > > + obj_cgroup_put(objcg); > > + nr_kmem = nr_pages; > > + } else { > > + memcg = page_memcg(page); > > + page->memcg_data = 0; > > + nr_kmem = 0; > > + } > > Why is all this moved above the uncharge_batch() call? Before calling obj_cgroup_put(), we need set page->memcg_data to zero. So I move "page->memcg_data = 0" to here. > > It separates the pointer manipulations from the refcounting, which > makes the code very difficult to follow. > > > + > > if (ug->memcg != memcg) { > > if (ug->memcg) { > > uncharge_batch(ug); > > uncharge_gather_clear(ug); > > } > > ug->memcg = memcg; > > + ug->dummy_page = page; > > Why this change? Just like ug->memcg, we do not need to set ug->dummy_page in every loop. > > > /* pairs with css_put in uncharge_batch */ > > css_get(&ug->memcg->css); > > } > > > > - nr_pages = compound_nr(page); > > ug->nr_pages += nr_pages; > > + ug->nr_kmem += nr_kmem; > > + ug->pgpgout += !nr_kmem; > > Oof. > > Yes, this pgpgout line is an equivalent transformation for counting > LRU compound pages. But unless you already know that, it's completely > impossible to understand what the intent here is. > > Please avoid clever tricks like this. If you need to check whether the > page is kmem, test PageMemcgKmem() instead of abusing the counters as > boolean flags. This is supposed to be read by human beings, too. Got it. > > > - if (PageMemcgKmem(page)) > > - ug->nr_kmem += nr_pages; > > - else > > - ug->pgpgout++; > > - > > - ug->dummy_page = page; > > - page->memcg_data = 0; > > - css_put(&ug->memcg->css); > > + css_put(&memcg->css); > > Sorry, these two functions are no longer readable after your changes. > > Please retain the following sequence as discrete steps: > > 1. look up memcg from the page > 2. flush existing batch if memcg changed > 3. add page's various counts to the current batch > 4. clear page->memcg and decrease the referece count to whatever it was pointing to Got it. > > And as per above, step 3. is where we should check whether to uncharge > the memcg's page counter at all: > > if (PageMemcgKmem(page)) { > ug->nr_pages += nr_pages; > ug->nr_kmem += nr_pages; > } else { > /* LRU pages aren't accounted at the root level */ > if (!mem_cgroup_is_root(memcg)) > ug->nr_pages += nr_pages; > ug->pgpgout++; > } > > In fact, it might be a good idea to rename ug->nr_pages to > ug->nr_memory to highlight how it maps to the page_counter. I will rework the code in the next version. Thanks for your suggestions.