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 mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 35A34C433EF for ; Thu, 18 Nov 2021 04:15:51 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id D355461B44 for ; Thu, 18 Nov 2021 04:15:50 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org D355461B44 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 0BC7B6B0072; Wed, 17 Nov 2021 23:15:40 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 043606B0073; Wed, 17 Nov 2021 23:15:39 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id E4CFF6B0074; Wed, 17 Nov 2021 23:15:39 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0147.hostedemail.com [216.40.44.147]) by kanga.kvack.org (Postfix) with ESMTP id D34C26B0072 for ; Wed, 17 Nov 2021 23:15:39 -0500 (EST) Received: from smtpin04.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 88C848249980 for ; Thu, 18 Nov 2021 04:15:29 +0000 (UTC) X-FDA: 78820736778.04.E225860 Received: from mail-io1-f47.google.com (mail-io1-f47.google.com [209.85.166.47]) by imf30.hostedemail.com (Postfix) with ESMTP id 7C512E0019B5 for ; Thu, 18 Nov 2021 04:15:27 +0000 (UTC) Received: by mail-io1-f47.google.com with SMTP id c3so6294142iob.6 for ; Wed, 17 Nov 2021 20:15:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=Dv4FMpzTlhmMmFiifkPCQFCgRBuUEUnuMSwoqw5Fr4o=; b=omqr4cQnnPmevelwOr+q6Fi7C6sTodMQdwtDb3HvpoNVQa77lXcCb/dH3PXFqJMuTs jr9AZrmXT1DmrQpZU4wlwTOHAXpvP/+pqiII7JIq43ICkCV7JPTxAw3j+Z/q8hp7rDJg w3cIgOumYkd14BgMin0r0eU68m1UuKYP42up0V9aDnJjwcJcyxay/XqabvmNOCDo9Jvn s342YAKrSypETDBbiSWtwvxweyYi3WRFEaj6h+Eujf5DsY5upwrnews6eiY/rWJrfIUv t3NvRg+pUV+Uy4k/nVRNVeH+EofbNEYawSIqYhwWMGkq/f8wSWGrSmflOCGgvhv+sBmw Z0Pw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=Dv4FMpzTlhmMmFiifkPCQFCgRBuUEUnuMSwoqw5Fr4o=; b=yJf0pnSqEocwRlIJ5EzLFBeJ6u8KQ5e52wYMgPHgXEaaojvCCYNI1xQOeNKuKJ5koh 4zaZTYr9xZ0JxoqbXBndJGP747cgRxeOvIEZeT5PMfUM7+8CwwMUSdBuxtOKpytqnjKU zWmzdREVkUefo6c6aMOOZVJezduw2pYaGUnib8tGYqtQ2Kgn5treNyMaGqUfnxR715q0 XtNB4ggyUlL5dPwugGkBMgwfNSj8c8xsJUdke5EnF9x6bzutCXJA3PpiEOFvpJ5cggcT Pg+7G8EMq2gZucV9FNbSgJVUbbzbXRTyCFX6FtB6D48UNrJVuiCT1osCqNzfRynS6P56 Bmpg== X-Gm-Message-State: AOAM532yXjOVk3f45LvYwhmXVw4hAKpnasJH14dgR9s0IZSqIwZnufYi VNfCvzbTvLQ1D3nF5rMdpIB8/yQPgkxLqRnnjkzFRQ== X-Google-Smtp-Source: ABdhPJxu6AkgtL/4msTfyEuWlVQQDgdP0qHDDtygnoDktA5q5rQTxfaknVLaSBngH7GgVixVcfpZ2bzBmVMCfOgK1Vk= X-Received: by 2002:a05:6602:3c2:: with SMTP id g2mr15805739iov.65.1637208928333; Wed, 17 Nov 2021 20:15:28 -0800 (PST) MIME-Version: 1.0 References: <20211117201825.429650-1-almasrymina@google.com> <86d3ba7a-3706-d66c-cbf7-d2c39ad2cd4c@oracle.com> In-Reply-To: From: Mina Almasry Date: Wed, 17 Nov 2021 20:15:17 -0800 Message-ID: Subject: Re: [PATCH v7] hugetlb: Add hugetlb.*.numa_stat file To: Muchun Song Cc: Mike Kravetz , Tejun Heo , Zefan Li , Johannes Weiner , Jonathan Corbet , Andrew Morton , Shuah Khan , Miaohe Lin , Oscar Salvador , Michal Hocko , David Rientjes , Shakeel Butt , Jue Wang , Yang Yao , Joanna Li , Cannon Matthews , Linux Memory Management List , LKML , Cgroups , linux-doc@vger.kernel.org Content-Type: text/plain; charset="UTF-8" X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 7C512E0019B5 X-Stat-Signature: 3j7g4w1wds69ntyfb5kxy87q66yku7wo Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=omqr4cQn; spf=pass (imf30.hostedemail.com: domain of almasrymina@google.com designates 209.85.166.47 as permitted sender) smtp.mailfrom=almasrymina@google.com; dmarc=pass (policy=reject) header.from=google.com X-HE-Tag: 1637208927-47706 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 17, 2021 at 7:55 PM Muchun Song wrote: > > On Thu, Nov 18, 2021 at 8:13 AM Mike Kravetz wrote: > > > > On 11/17/21 12:18, Mina Almasry wrote: > > ... > > > diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c > > ... > > > @@ -288,11 +317,21 @@ static void __hugetlb_cgroup_commit_charge(int idx, unsigned long nr_pages, > > > struct hugetlb_cgroup *h_cg, > > > struct page *page, bool rsvd) > > > { > > > + unsigned long *usage; > > > + > > > > I assume the use of a pointer is just to make the following WRITE_ONCE > > look better? I prefer the suggestion by Muchun: > > > > unsigned long usage = h_cg->nodeinfo[page_to_nid(page)]->usage[idx]; > > > > usage += nr_pages; > > WRITE_ONCE(h_cg->nodeinfo[page_to_nid(page)]->usage[idx], usage); > > > > I had to think for just a second 'why are we using/passing a pointer?'. > > Not insisting we use Muchun's suggestion, it just caused me to think > > a little more than necessary. > > At least I have the same question here. For me I think it's > unnecessary to use a pointer. > Hmm to be honest I would have not thought it would be preferable to duplicate a long string like h_cg->nodeinfo[page_to_nid(page)]->usage[idx], and then for future code changes to keep them in sync. I think Marco had the same thinking and that was his initial suggestion, but I don't mind much either way. I'll submit another iteration with the change :-) > > > > In any case, I would move the variable usage inside the > > 'if (!rsvd)' block. > > > > > if (hugetlb_cgroup_disabled() || !h_cg) > > > return; > > > > > > __set_hugetlb_cgroup(page, h_cg, rsvd); > > > - return; > > > + if (!rsvd) { > > > + usage = &h_cg->nodeinfo[page_to_nid(page)]->usage[idx]; > > > + /* > > > + * This write is not atomic due to fetching *usage and writing > > > + * to it, but that's fine because we call this with > > > + * hugetlb_lock held anyway. > > > + */ > > > + WRITE_ONCE(*usage, *usage + nr_pages); > > > + } > > > } > > > > > > void hugetlb_cgroup_commit_charge(int idx, unsigned long nr_pages, > > > @@ -316,6 +355,7 @@ static void __hugetlb_cgroup_uncharge_page(int idx, unsigned long nr_pages, > > > struct page *page, bool rsvd) > > > { > > > struct hugetlb_cgroup *h_cg; > > > + unsigned long *usage; > > > > Same here. > > > > Otherwise, looks good to me. > > -- > > Mike Kravetz