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 88CF9C433EF for ; Mon, 20 Jun 2022 11:38:00 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 215D28E0001; Mon, 20 Jun 2022 07:38:00 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 19F4B6B0073; Mon, 20 Jun 2022 07:38:00 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 03E4E8E0001; Mon, 20 Jun 2022 07:37:59 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id E4FF76B0071 for ; Mon, 20 Jun 2022 07:37:59 -0400 (EDT) Received: from smtpin29.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id A3B57E2B for ; Mon, 20 Jun 2022 11:37:59 +0000 (UTC) X-FDA: 79598415078.29.B627471 Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by imf16.hostedemail.com (Postfix) with ESMTP id 1E074180091 for ; Mon, 20 Jun 2022 11:37:58 +0000 (UTC) Received: from relay2.suse.de (relay2.suse.de [149.44.160.134]) by smtp-out2.suse.de (Postfix) with ESMTP id 900E11F8CF; Mon, 20 Jun 2022 11:37:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1655725077; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=gk50856yvyikasm7bS0OG56qAWkZTO8FSBBzDtB/dnw=; b=bS8dARtczrr/thaYtGaFWbzUCIuUilnu3VvA0Hpy5VRddQLxTtV/UzyYVHby5b5eVALWt2 OUHlUARH37jKTqa5bZ4JN/B+mmeX94bQ0vGWumv9rcjJWwFokGvQ43MwWvWlUCWu2GjuQJ UkYqvYl/ibJH9qefIcEv5R3tCWEj0os= Received: from suse.cz (unknown [10.100.201.86]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay2.suse.de (Postfix) with ESMTPS id 1705C2C16B; Mon, 20 Jun 2022 11:37:57 +0000 (UTC) Date: Mon, 20 Jun 2022 13:37:56 +0200 From: Michal Hocko To: Kent Overstreet Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, pmladek@suse.com, rostedt@goodmis.org, enozhatsky@chromium.org, linux@rasmusvillemoes.dk, willy@infradead.org Subject: Re: [PATCH v4 24/34] mm/memcontrol.c: Convert to printbuf Message-ID: References: <20220620004233.3805-1-kent.overstreet@gmail.com> <20220620004233.3805-25-kent.overstreet@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220620004233.3805-25-kent.overstreet@gmail.com> ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1655725079; 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: in-reply-to:in-reply-to:references:references:dkim-signature; bh=gk50856yvyikasm7bS0OG56qAWkZTO8FSBBzDtB/dnw=; b=Gop/K9UPihibTIHytPy7IAcgZlGFrxXVygppzl5wYnQsba98FfnjvKoe+8RWEOq5EAR4sD qbZW5tW61/DhWnwjt4SL1j4aFbe4M2RDrlJOtFY1jXWoURnveD9rSqVmIl5Hbfz5ZNQKFj 7l8JzpMU3n5Gxzm03p5PEYtX/RrPWI4= ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1655725079; a=rsa-sha256; cv=none; b=hjEbZt8wmGTxczUS5qU1FTIrL4SKVBwgMaKfJQ3633XofmUC0J6bopUoU4DJHHoRI3I0MJ kOLfghdryQfvhND9+VTKwazQ6rEnlVyNStSuPQGf3UXnagiiFYRGH7jw8s+cIY+GHEpOYl UVmdcGjJLhBBAVpbR1tO7b80yYI/R0c= ARC-Authentication-Results: i=1; imf16.hostedemail.com; dkim=pass header.d=suse.com header.s=susede1 header.b=bS8dARtc; spf=pass (imf16.hostedemail.com: domain of mhocko@suse.com designates 195.135.220.29 as permitted sender) smtp.mailfrom=mhocko@suse.com; dmarc=pass (policy=quarantine) header.from=suse.com X-Stat-Signature: oq7bqebuy53xgmo7hhpwze7ndmrzafhi Authentication-Results: imf16.hostedemail.com; dkim=pass header.d=suse.com header.s=susede1 header.b=bS8dARtc; spf=pass (imf16.hostedemail.com: domain of mhocko@suse.com designates 195.135.220.29 as permitted sender) smtp.mailfrom=mhocko@suse.com; dmarc=pass (policy=quarantine) header.from=suse.com X-Rspamd-Queue-Id: 1E074180091 X-Rspamd-Server: rspam02 X-Rspam-User: X-HE-Tag: 1655725078-972535 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 Sun 19-06-22 20:42:23, Kent Overstreet wrote: > This converts memory_stat_format() from seq_buf to printbuf. Printbuf is > simalar to seq_buf except that it heap allocates the string buffer: > here, we were already heap allocating the buffer with kmalloc() so the > conversion is trivial. > > Signed-off-by: Kent Overstreet I have asked for a justification two times already not hearing anything. Please drop this patch. I do not see any actual advantage of the conversion. The primary downside of the existing code is that an internal buffer is exposed to the caller which is error prone and ugly. The conversion doesn't really address that part. Moreover there is an inconsistency between failrure case where the printbuf is destroyed by a docummented way (printbuf_exit) and when the caller frees the buffer directly. If printbuf_exit evers need to do more than just kfree (e.g. kvfree) then this is a subtle bug hard to find. > --- > mm/memcontrol.c | 68 ++++++++++++++++++++++++------------------------- > 1 file changed, 33 insertions(+), 35 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 598fece89e..57861dc9fe 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -62,7 +62,7 @@ > #include > #include > #include > -#include > +#include > #include "internal.h" > #include > #include > @@ -1461,13 +1461,9 @@ static inline unsigned long memcg_page_state_output(struct mem_cgroup *memcg, > > static char *memory_stat_format(struct mem_cgroup *memcg) > { > - struct seq_buf s; > + struct printbuf buf = PRINTBUF; > int i; > > - seq_buf_init(&s, kmalloc(PAGE_SIZE, GFP_KERNEL), PAGE_SIZE); > - if (!s.buffer) > - return NULL; > - > /* > * Provide statistics on the state of the memory subsystem as > * well as cumulative event counters that show past behavior. > @@ -1484,49 +1480,51 @@ static char *memory_stat_format(struct mem_cgroup *memcg) > u64 size; > > size = memcg_page_state_output(memcg, memory_stats[i].idx); > - seq_buf_printf(&s, "%s %llu\n", memory_stats[i].name, size); > + prt_printf(&buf, "%s %llu\n", memory_stats[i].name, size); > > if (unlikely(memory_stats[i].idx == NR_SLAB_UNRECLAIMABLE_B)) { > size += memcg_page_state_output(memcg, > NR_SLAB_RECLAIMABLE_B); > - seq_buf_printf(&s, "slab %llu\n", size); > + prt_printf(&buf, "slab %llu\n", size); > } > } > > /* Accumulated memory events */ > > - seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGFAULT), > - memcg_events(memcg, PGFAULT)); > - seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGMAJFAULT), > - memcg_events(memcg, PGMAJFAULT)); > - seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGREFILL), > - memcg_events(memcg, PGREFILL)); > - seq_buf_printf(&s, "pgscan %lu\n", > - memcg_events(memcg, PGSCAN_KSWAPD) + > - memcg_events(memcg, PGSCAN_DIRECT)); > - seq_buf_printf(&s, "pgsteal %lu\n", > - memcg_events(memcg, PGSTEAL_KSWAPD) + > - memcg_events(memcg, PGSTEAL_DIRECT)); > - seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGACTIVATE), > - memcg_events(memcg, PGACTIVATE)); > - seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGDEACTIVATE), > - memcg_events(memcg, PGDEACTIVATE)); > - seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGLAZYFREE), > - memcg_events(memcg, PGLAZYFREE)); > - seq_buf_printf(&s, "%s %lu\n", vm_event_name(PGLAZYFREED), > - memcg_events(memcg, PGLAZYFREED)); > + prt_printf(&buf, "%s %lu\n", vm_event_name(PGFAULT), > + memcg_events(memcg, PGFAULT)); > + prt_printf(&buf, "%s %lu\n", vm_event_name(PGMAJFAULT), > + memcg_events(memcg, PGMAJFAULT)); > + prt_printf(&buf, "%s %lu\n", vm_event_name(PGREFILL), > + memcg_events(memcg, PGREFILL)); > + prt_printf(&buf, "pgscan %lu\n", > + memcg_events(memcg, PGSCAN_KSWAPD) + > + memcg_events(memcg, PGSCAN_DIRECT)); > + prt_printf(&buf, "pgsteal %lu\n", > + memcg_events(memcg, PGSTEAL_KSWAPD) + > + memcg_events(memcg, PGSTEAL_DIRECT)); > + prt_printf(&buf, "%s %lu\n", vm_event_name(PGACTIVATE), > + memcg_events(memcg, PGACTIVATE)); > + prt_printf(&buf, "%s %lu\n", vm_event_name(PGDEACTIVATE), > + memcg_events(memcg, PGDEACTIVATE)); > + prt_printf(&buf, "%s %lu\n", vm_event_name(PGLAZYFREE), > + memcg_events(memcg, PGLAZYFREE)); > + prt_printf(&buf, "%s %lu\n", vm_event_name(PGLAZYFREED), > + memcg_events(memcg, PGLAZYFREED)); > > #ifdef CONFIG_TRANSPARENT_HUGEPAGE > - seq_buf_printf(&s, "%s %lu\n", vm_event_name(THP_FAULT_ALLOC), > - memcg_events(memcg, THP_FAULT_ALLOC)); > - seq_buf_printf(&s, "%s %lu\n", vm_event_name(THP_COLLAPSE_ALLOC), > - memcg_events(memcg, THP_COLLAPSE_ALLOC)); > + prt_printf(&buf, "%s %lu\n", vm_event_name(THP_FAULT_ALLOC), > + memcg_events(memcg, THP_FAULT_ALLOC)); > + prt_printf(&buf, "%s %lu\n", vm_event_name(THP_COLLAPSE_ALLOC), > + memcg_events(memcg, THP_COLLAPSE_ALLOC)); > #endif /* CONFIG_TRANSPARENT_HUGEPAGE */ > > - /* The above should easily fit into one page */ > - WARN_ON_ONCE(seq_buf_has_overflowed(&s)); > + if (buf.allocation_failure) { > + printbuf_exit(&buf); > + return NULL; > + } > > - return s.buffer; > + return buf.buf; > } > > #define K(x) ((x) << (PAGE_SHIFT-10)) > -- > 2.36.1 -- Michal Hocko SUSE Labs