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=-0.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS autolearn=no 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 AE83AC433E0 for ; Sat, 20 Jun 2020 21:00:23 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 601E2206B7 for ; Sat, 20 Jun 2020 21:00:23 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="2AaUMIhT" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 601E2206B7 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=linux-foundation.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id C39508D0012; Sat, 20 Jun 2020 17:00:22 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id BEA948D0008; Sat, 20 Jun 2020 17:00:22 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B00DF8D0012; Sat, 20 Jun 2020 17:00:22 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0240.hostedemail.com [216.40.44.240]) by kanga.kvack.org (Postfix) with ESMTP id 90DE38D0008 for ; Sat, 20 Jun 2020 17:00:22 -0400 (EDT) Received: from smtpin04.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 4946C824556B for ; Sat, 20 Jun 2020 21:00:21 +0000 (UTC) X-FDA: 76950808242.04.milk94_5412c0226e24 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin04.hostedemail.com (Postfix) with ESMTP id 28F338006998 for ; Sat, 20 Jun 2020 21:00:21 +0000 (UTC) X-HE-Tag: milk94_5412c0226e24 X-Filterd-Recvd-Size: 5178 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by imf48.hostedemail.com (Postfix) with ESMTP for ; Sat, 20 Jun 2020 21:00:20 +0000 (UTC) Received: from X1 (nat-ab2241.sltdut.senawave.net [162.218.216.4]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 693FF206B7; Sat, 20 Jun 2020 21:00:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1592686819; bh=o8uHws6VLwHyqYldC7EmQtIl/C7Bn2oBRr57FeHYGSU=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=2AaUMIhThXFiP4OUgqM/0+7Ygl9MrmPX6icm5BrOMkpPZ88R8p9Zc62sBBR8inI2u 0IVFC3benztc9rKUfOrNPLOPhpe2rAYTb8u/ImrNV+6yVV0qS2G5dbwoNJB8ESQsxq vkj1+4NL8K8BkZu/DRbDC2GIgPPWSlmc1yLSLTPQ= Date: Sat, 20 Jun 2020 14:00:18 -0700 From: Andrew Morton To: Waiman Long Cc: Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Johannes Weiner , Shakeel Butt , Roman Gushchin , linux-mm@kvack.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] mm, slab: Fix sign conversion problem in memcg_uncharge_slab() Message-Id: <20200620140018.a305aebd01b2cf4226547944@linux-foundation.org> In-Reply-To: <20200620184719.10994-1-longman@redhat.com> References: <20200620184719.10994-1-longman@redhat.com> X-Mailer: Sylpheed 3.5.1 (GTK+ 2.24.32; x86_64-pc-linux-gnu) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 28F338006998 X-Spamd-Result: default: False [0.00 / 100.00] X-Rspamd-Server: rspam03 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 Sat, 20 Jun 2020 14:47:19 -0400 Waiman Long wrote: > It was found that running the LTP test on a PowerPC system could produce > erroneous values in /proc/meminfo, like: > > MemTotal: 531915072 kB > MemFree: 507962176 kB > MemAvailable: 1100020596352 kB > > Using bisection, the problem is tracked down to commit 9c315e4d7d8c > ("mm: memcg/slab: cache page number in memcg_(un)charge_slab()"). > > In memcg_uncharge_slab() with a "int order" argument: > > unsigned int nr_pages = 1 << order; > : > mod_lruvec_state(lruvec, cache_vmstat_idx(s), -nr_pages); > > The mod_lruvec_state() function will eventually call the > __mod_zone_page_state() which accepts a long argument. Depending on > the compiler and how inlining is done, "-nr_pages" may be treated as > a negative number or a very large positive number. Apparently, it was > treated as a large positive number in that PowerPC system leading to > incorrect stat counts. This problem hasn't been seen in x86-64 yet, > perhaps the gcc compiler there has some slight difference in behavior. > > It is fixed by making nr_pages a signed value. For consistency, a > similar change is applied to memcg_charge_slab() as well. This is somewhat disturbing. > --- a/mm/slab.h > +++ b/mm/slab.h > @@ -348,7 +348,7 @@ static __always_inline int memcg_charge_slab(struct page *page, > gfp_t gfp, int order, > struct kmem_cache *s) > { > - unsigned int nr_pages = 1 << order; > + int nr_pages = 1 << order; > struct mem_cgroup *memcg; > struct lruvec *lruvec; > int ret; > @@ -388,7 +388,7 @@ static __always_inline int memcg_charge_slab(struct page *page, > static __always_inline void memcg_uncharge_slab(struct page *page, int order, > struct kmem_cache *s) > { > - unsigned int nr_pages = 1 << order; > + int nr_pages = 1 << order; > struct mem_cgroup *memcg; > struct lruvec *lruvec; > I grabbed the patch, but Roman's "mm: memcg/slab: charge individual slab objects instead of pages" (http://lkml.kernel.org/r/20200608230654.828134-10-guro@fb.com) deletes both these functions. It replaces the offending code with, afaict, static inline void memcg_slab_free_hook(struct kmem_cache *s, struct page *page, void *p) { struct obj_cgroup *objcg; unsigned int off; if (!memcg_kmem_enabled() || is_root_cache(s)) return; off = obj_to_index(s, page, p); objcg = page_obj_cgroups(page)[off]; page_obj_cgroups(page)[off] = NULL; obj_cgroup_uncharge(objcg, obj_full_size(s)); mod_objcg_state(objcg, page_pgdat(page), cache_vmstat_idx(s), >>> -obj_full_size(s)); obj_cgroup_put(objcg); } -obj_full_size() returns size_t so I guess that's OK. Also static __always_inline void uncharge_slab_page(struct page *page, int order, struct kmem_cache *s) { #ifdef CONFIG_MEMCG_KMEM if (memcg_kmem_enabled() && !is_root_cache(s)) { memcg_free_page_obj_cgroups(page); percpu_ref_put_many(&s->memcg_params.refcnt, 1 << order); } #endif mod_node_page_state(page_pgdat(page), cache_vmstat_idx(s), >>> -(PAGE_SIZE << order)); } PAGE_SIZE is unsigned long so I guess that's OK as well. Still, perhaps both could be improved. Negating an unsigned scalar is a pretty ugly thing to do. Am I wrong in thinking that all those mod_foo() functions need careful review?