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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id B7BAAC2BD09 for ; Fri, 28 Jun 2024 02:20:35 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 02DA86B008A; Thu, 27 Jun 2024 22:20:35 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id F1E7A6B008C; Thu, 27 Jun 2024 22:20:34 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id DE6446B0092; Thu, 27 Jun 2024 22:20:34 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id C10C26B008A for ; Thu, 27 Jun 2024 22:20:34 -0400 (EDT) Received: from smtpin19.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 406541601DD for ; Fri, 28 Jun 2024 02:20:34 +0000 (UTC) X-FDA: 82278693588.19.62E3BD0 Received: from szxga04-in.huawei.com (szxga04-in.huawei.com [45.249.212.190]) by imf19.hostedemail.com (Postfix) with ESMTP id C5CEE1A000A for ; Fri, 28 Jun 2024 02:20:30 +0000 (UTC) Authentication-Results: imf19.hostedemail.com; dkim=none; spf=pass (imf19.hostedemail.com: domain of xiujianfeng@huawei.com designates 45.249.212.190 as permitted sender) smtp.mailfrom=xiujianfeng@huawei.com; dmarc=pass (policy=quarantine) header.from=huawei.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1719541215; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=HK2bodfCB56dBuCoodpnt1+OsiSWyPbESnkwD88MdLM=; b=gcSeX8f/AC2qe2uvKHXRJ6mi6+VyllvYs9zFITujKHHwRdQM8RUOOY/Pp/Sp2p8JIiZ/0A ljt6obGm+KnqtZEpkHx6gZnp5OHas3v7QgahAudduR6QYK+hKgBtOWlZeKaO3fVEYX3wh7 je5VKfRotvw//Aush7tOWaJ3a/ctN7g= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1719541215; a=rsa-sha256; cv=none; b=wf9QYsyQBcZuBDyFXQAxetGn7OmhmFK5mQNkvnglJIoBj2zZLmeCWRmhLoEId4OrekneL+ jNDAw+wzks9OGcPFTu8kUegNuMGSbZzziXFrsZvohRQwkBoot4zbzsei5AVJZAOw8u9nXx Qr+6w9X1AdwZ5NCuF1BKyd6Icbtc9HY= ARC-Authentication-Results: i=1; imf19.hostedemail.com; dkim=none; spf=pass (imf19.hostedemail.com: domain of xiujianfeng@huawei.com designates 45.249.212.190 as permitted sender) smtp.mailfrom=xiujianfeng@huawei.com; dmarc=pass (policy=quarantine) header.from=huawei.com Received: from mail.maildlp.com (unknown [172.19.162.112]) by szxga04-in.huawei.com (SkyGuard) with ESMTP id 4W9Jvp5d7Wz2Ckks; Fri, 28 Jun 2024 10:16:22 +0800 (CST) Received: from dggpeml500023.china.huawei.com (unknown [7.185.36.114]) by mail.maildlp.com (Postfix) with ESMTPS id 4514714011F; Fri, 28 Jun 2024 10:20:24 +0800 (CST) Received: from [10.67.110.112] (10.67.110.112) by dggpeml500023.china.huawei.com (7.185.36.114) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.39; Fri, 28 Jun 2024 10:20:23 +0800 Message-ID: Date: Fri, 28 Jun 2024 10:20:23 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.5.1 Subject: Re: [PATCH -next] mm: memcg: remove redundant seq_buf_has_overflowed() Content-Language: en-US To: Michal Hocko CC: , , , , , , , References: <20240626094232.2432891-1-xiujianfeng@huawei.com> <10b948cd-5fbf-78e7-c3e8-6867661fa50b@huawei.com> From: xiujianfeng In-Reply-To: Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.67.110.112] X-ClientProxiedBy: dggems706-chm.china.huawei.com (10.3.19.183) To dggpeml500023.china.huawei.com (7.185.36.114) X-Rspam-User: X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: C5CEE1A000A X-Stat-Signature: jeogru75e55u54i5pazard5behkyjojs X-HE-Tag: 1719541230-690662 X-HE-Meta: U2FsdGVkX19roiPxugF3vMCeucx+VkwMoFpW2PuKE0J58lGajZNVG6wNC5bVGe7z4GCLy/fmcwJVefImyvvadqAmfCtBmyQRtxV/tCr2Il3G6wtJTy73kvc6HD35N+Twmi4cTs1YlNRbhk7qyLW6DXhuj1/KfhlEEGGoUY1V56OvNs3x6AHOgDujsaKye5f/RsMHv2vmP/E5DHeGROzgmI9kx5XcOYuseDAUFiVkHftjdGoZqKSriD6U7xOoPlF8D34xlITQjQpM47OQK3m2C0sRdOlSEBw7r929M7ugX15hSvo63Lv0DfH9N1uW7res5CRow5GbmXKgR2+2spEx8oD0Mw0LJFoBdcNvbZBDz6ssMojRzOZMGigwM1iI0mdX/fQwjWwVTMNPA1X6+sas5T5zgfq1bxE7o9UuAkCIsruRH4NLHEqYYkMyFeeedDCaD30K9u1iQwL6vWMSBjZqOjqHZDX1NgCb6PGk+U3yLFfok5hmhnBcQz6fHfZDNApapm3H1YDzLOzS6T72yhuNxqra0cJpPxPvSxfMjA+lIC74Dans4ooPj0C4ormzualnFc9WEBDLZUlHthj28AqY6egF5bx4Gs9XCPioAKJKyWsEubCSS312lQ8gUrV4p/7CQfn6tGsCpbhmnlA/Fa28JczfgbouoddFe1yV8fbbvdCGWU5k+tixIlvP39Ft1MJyO4MpnMv40Cs92XzxMEpePOHbAKIuPebYzfRSIdYBDTv88Cd24+wpKVYhPN/ILnGVLcXZYgvHkGnYhEa/ovnw/kEoHzRRB7fUnOi6urOsXcyNTdhjv4+JRfh0qAMZuPwuHDo0RBUXQkh0DHUpUgdLvthLVIYWYQd2c8x4vh8GcMPdD/sY3oL8ybswQ6Tcd3Vwghq5RVX2FnJvQa2u60ERiulxPLCYcvC4k9UPnqivwVXCAdrtI0qP5yDCeNWjPzrltdMRWR1EeaD4y7zK9+C xyWj3MU9 v1tnQ3EAe/xx3MeOFr3aher+o3mNj4mbrVs9SZjOR35qTnyPqL8qq93i/RhGproG6rrgSTw68Bc+fTf4ExAfF0DAOQAWnmmcEFMbQTLTD/5poo+V3GNSzlVZM2R/cg8OuDNHOJSEQvaH7u4vk+/cYy4wrY/c6X77RnJ33xEJTy381kXufO8aTefO4BA== 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: List-Subscribe: List-Unsubscribe: On 2024/6/27 19:54, Michal Hocko wrote: > On Thu 27-06-24 19:43:06, xiujianfeng wrote: >> >> >> On 2024/6/27 19:20, Michal Hocko wrote: >>> On Thu 27-06-24 16:33:00, xiujianfeng wrote: >>>> >>>> >>>> On 2024/6/27 15:13, Michal Hocko wrote: >>>>> On Wed 26-06-24 09:42:32, Xiu Jianfeng wrote: >>>>>> Both the end of memory_stat_format() and memcg_stat_format() will call >>>>>> WARN_ON_ONCE(seq_buf_has_overflowed()). However, memory_stat_format() >>>>>> is the only caller of memcg_stat_format(), when memcg is on the default >>>>>> hierarchy, seq_buf_has_overflowed() will be executed twice, so remove >>>>>> the reduntant one. >>>>> >>>>> Shouldn't we rather remove both? Are they giving us anything useful >>>>> actually? Would a simpl pr_warn be sufficient? Afterall all we care >>>>> about is to learn that we need to grow the buffer size because our stats >>>>> do not fit anymore. It is not really important whether that is an OOM or >>>>> cgroupfs interface path. >>>> >>>> I did a test, when I removed both of them and added a lot of prints in >>>> memcg_stat_format() to make the seq_buf overflow, and then cat >>>> memory.stat in user mode, no OOM occurred, and there were no warning >>>> logs in the kernel. >>> >>> The default buffer size is PAGE_SIZE. >> >> Hi Michal, >> >> I'm sorry, I didn't understand what you meant by this sentence. What I >> mean is that we can't remove both, otherwise, neither the kernel nor >> user space would be aware of a buffer overflow. From my test, there was >> no OOM or other exceptions when the overflow occurred; it just resulted >> in the displayed information being truncated. Therefore, we need to keep >> one. > > I've had this in mind > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 71fe2a95b8bd..3e17b9c3a27a 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -1845,9 +1845,6 @@ static void memcg_stat_format(struct mem_cgroup *memcg, struct seq_buf *s) > vm_event_name(memcg_vm_event_stat[i]), > memcg_events(memcg, memcg_vm_event_stat[i])); > } > - > - /* The above should easily fit into one page */ > - WARN_ON_ONCE(seq_buf_has_overflowed(s)); > } > > static void memcg1_stat_format(struct mem_cgroup *memcg, struct seq_buf *s); > @@ -1858,7 +1855,8 @@ static void memory_stat_format(struct mem_cgroup *memcg, struct seq_buf *s) > memcg_stat_format(memcg, s); > else > memcg1_stat_format(memcg, s); > - WARN_ON_ONCE(seq_buf_has_overflowed(s)); > + if (seq_buf_has_overflowed(s)) > + pr_warn("%s: Stat buffer insufficient please report\n", __FUNCTION__); I found that after the change, the effect is as follows: # dmesg [ 51.028327] memory_stat_format: Stat buffer insufficient please report with no keywords such as "Failed", "Warning" to draw attention to this printout. Should we change it to the following? if (seq_buf_has_overflowed(s)) pr_warn("%s: Warning, Stat buffer overflow, please report\n", __FUNCTION__); > } > > /** > > Because WARN_ON_ONCE doesn't buy us anything actually. It will dump > stack trace and it seems really mouthfull (and it will panic when > panic_on_warn is enabled which is likely not a great thing).