Message-ID: <86d3ba7a-3706-d66c-cbf7-d2c39ad2cd4c@oracle.com>
Date:   Wed, 17 Nov 2021 16:12:49 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.3.0
Subject: Re: [PATCH v7] hugetlb: Add hugetlb.*.numa_stat file
Content-Language: en-US
To:     Mina Almasry <almasrymina@google.com>, Tejun Heo <tj@kernel.org>,
        Zefan Li <lizefan.x@bytedance.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Jonathan Corbet <corbet@lwn.net>,
        Andrew Morton <akpm@linux-foundation.org>
Cc:     Shuah Khan <shuah@kernel.org>, Miaohe Lin <linmiaohe@huawei.com>,
        Oscar Salvador <osalvador@suse.de>,
        Michal Hocko <mhocko@suse.com>,
        Muchun Song <songmuchun@bytedance.com>,
        David Rientjes <rientjes@google.com>,
        Shakeel Butt <shakeelb@google.com>, Jue Wang <juew@google.com>,
        Yang Yao <ygyao@google.com>, Joanna Li <joannali@google.com>,
        Cannon Matthews <cannonmatthews@google.com>,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        cgroups@vger.kernel.org, linux-doc@vger.kernel.org
References: <20211117201825.429650-1-almasrymina@google.com>
From:   Mike Kravetz <mike.kravetz@oracle.com>
In-Reply-To: <20211117201825.429650-1-almasrymina@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?RUh6L0tFSlMzVEl6NFJ5NFJiVGhhWG5jTU9KQVFWQ0Z5Z1p5RCtxbkhDNitC?=
 =?utf-8?B?cklFTEpPclBQczduRTFQa1FwRjY4a0o4eitwNGRLMGcxSDlUd2Y2WnF4ZXhO?=
 =?utf-8?B?dXcyTHVoeDRLNWtxV2xCNlczQ0R4aHFyZGI2STRWU2pGQ29kd3JxVkZ0Yy9n?=
 =?utf-8?B?VG5IeCtxMVRSdVRBd0krKzlMWFkxQ0ZZVks0Z25uVURnUkZoNURpckhXV21E?=
 =?utf-8?B?VnhYMVcvcmJzSWZCck1LOXNkeUJnSWNVeFVxN253RytQbFBHVGtFL29ZbCtu?=
 =?utf-8?B?L3V3cFlJenlqN2dtRkFTeXVBZGw4UkJ4cU5SbFVET1dXTGVsWU0wb0pvOEI4?=
 =?utf-8?B?Q1VGQ24rcEhUMEdESWpsY1plUTJPZE5IQXN2aFEvWmwwL1kyTy9wc0QrU2Zk?=
 =?utf-8?B?Q0FaNmdEV1lVem5ybVlOMUNkZS9wZjJ3UFlIckY5Zjg2Nk0yYk5mUktUVHds?=
 =?utf-8?B?eXRiZEFsakNHblJ2V2hjeUJlMUZodFk3K2wzMEFwS3pIeFpSWmthUDRnQjFT?=
 =?utf-8?B?M1VQUzNZWjRqMC9xcisxSzVLYk9RWE9naHZwZk5DVEUvNWVldktqU0NkcFd6?=
 =?utf-8?B?aWphZkxQOGhDT2hFbnhCYVpXZytRN2NIcWc4VUJacDdOVjcydmhJZEVueWY2?=
 =?utf-8?B?RG9yWjR6bytpRDR6QTkyd0VScnlGUnAxS3pLdjRSM0tzWUswb28vbkJGdURk?=
 =?utf-8?B?eXFRUmxEamtmUVNLK3o2Y3hXWTdQbW54WXFiamwvdC9za3pJUStMYzhwRXhk?=
 =?utf-8?B?N3NJOUtLaWpPalV0NUo5NGkrUFU0dDNQSlVyNHVmN1U0a3NwL0IrbUExYTlj?=
 =?utf-8?B?ZE8wUDRIZ0dpb2FHS0hSMHNreEU2bXhnNVFxZytvQThIMnlVQWgwR0tBbVVE?=
 =?utf-8?B?ejV3ZG5jMkVOQmlhRDBNVVRreWlueHRLeGcwZlNaa3hrWTFORWtSRUpCQ1A4?=
 =?utf-8?B?dC9GK1Y3M3Rld3ZQNENnT21EWU91c1g5cVNrUHk1L01ZQ1owNDVOMUpsTENs?=
 =?utf-8?B?Ukw3Y29NMVg0a0JibjFrUENXT082aG5vTE1jUXNRZFZGb1crMzAwdEJhUEpY?=
 =?utf-8?B?d3FnT0RKMTZhZXlUb0NBVnRwVVN3SXorb09uUnFqLzR6R0JRZmp2eDZrU1VQ?=
 =?utf-8?B?Tm81eWN5VHZEWnk4TzA4V0hMQWJzdkc4VGpIVkI0blhKU2szWHl3aXB6UXlS?=
 =?utf-8?B?ZVpjeDRBV0NNZ2xtNDRYMmFsdTZTR0QvY3FCWHgyNGdRL0NLTUVLYUgyZTg1?=
 =?utf-8?B?Nk1MOStiVnFocjJ4YUgxbHBCd3UxaTNub3RYTG56LzQxbmE1eis0cEluWEd6?=
 =?utf-8?B?TEQ2M1pNZXd0STllQysvT3dlbE1SMkpmY3o0OHcraHZkbGZWSlA3T2FsaHNt?=
 =?utf-8?B?Q1p0dlZka1lwTXJjM1pad3k4TVByZk5DQ3JSRmtRY1pPbmYwMFhjVkVZZklI?=
 =?utf-8?B?U3hNcnlKM2RvbG8wR2R2cVJrY3BQNFRvZVZHVXh6VEM2eDZld0l3NVBvaFFX?=
 =?utf-8?B?TjNtRDkzbGhHZVR3enErVlZMeld0aTA3MGRJVWlzSWtNMU1ubWZmZFlud2ho?=
 =?utf-8?B?bnlkditzL0ZBZlQ2cFZTTC9HQUFsT2pOOUw5c0VYdHI4ajNsQ2lFaDcwb0tp?=
 =?utf-8?B?WForays2VjlaeGI3SFF2ZWorNW9vUHFLKzJkTkEybWh0ZlJSMFAxWlEvU2t4?=
 =?utf-8?B?ZFBzd2wzcDNUQjg1cVoyTko0bVQ3dzBwTUJBbjROdVRmUG5xTUF4WHF6NjJ1?=
 =?utf-8?B?WlNDd0c2YjBMdjI3TGFvVUNQOGtudGJlSE9HR3BRODJlTkdhbFZBY1h2Sngy?=
 =?utf-8?B?bW5oNjBCaldvM3pQc1l1N09HUml0MTMrNmt6K2tRSmEwTk1BSzNDNlhMU1RD?=
 =?utf-8?B?SWJLRDBzcmcxRy9tRDRLRXE1VDcxTmwwT3NmMlQ1OFdDZVIxdVRuaUpOVFgz?=
 =?utf-8?B?SHJ3QlVmNGVJbEEvTzI1NElyTUp1bFRGV0lnKzhwM0ZEQlhocE5YaGtUT0V4?=
 =?utf-8?B?eVlGejVoTUxpSWdibFMwWmFVZm9vNFcrQThkNXhoMFJ2YnZzNWhJWmYvZE9a?=
 =?utf-8?B?dHF3RFVCY3pldFBYakdWVnVzZnFEbVdLNVJTVFpuaXJrcms3dnBYenBOSzJG?=
 =?utf-8?B?Wk1hMGZCZndqa25NcmJoeWRlVHl5SVdKMDZKZ1pjTGlIV0RDUVA3ZXFxYkd1?=
 =?utf-8?Q?5ELW4zt2wLkpOqDXPfzCoqg=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 65895c45-e52e-4a86-0817-08d9aa282998
X-MS-Exchange-CrossTenant-AuthSource: BY5PR10MB4196.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Nov 2021 00:12:52.5955
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: LWshhZEnDACdLm/+vC8lKJz0pNqxb8BE620fcSN87f5GSDxSdEjAHKTn0YSRy9Yv1trT8RwhUU+w31NEFkbFNw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR10MB3542
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10171 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 spamscore=0 phishscore=0 bulkscore=0
 adultscore=0 mlxscore=0 malwarescore=0 mlxlogscore=999 suspectscore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2110150000
 definitions=main-2111170113
X-Proofpoint-GUID: CWsCO6Von24_srqKJxogL79nFRPAxznE
X-Proofpoint-ORIG-GUID: CWsCO6Von24_srqKJxogL79nFRPAxznE
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.

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