Message-ID: <CAJxJ_jjGS39doSXGn7rxX_OmfLni=5YFwbucV7UxK-KMH937uQ@mail.gmail.com>
Date: Fri, 23 Jan 2026 21:24:26 +0800
From: Jianyue Wu <wujianyue000@...il.com>
To: JP Kobryn <inwardvessel@...il.com>
Cc: akpm@...ux-foundation.org, shakeel.butt@...ux.dev, hannes@...xchg.org, 
	mhocko@...nel.org, roman.gushchin@...ux.dev, muchun.song@...ux.dev, 
	linux-mm@...ck.org, cgroups@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] mm: optimize stat output for 11% sys time reduce

On Fri, Jan 23, 2026 at 4:14 PM JP Kobryn <inwardvessel@...il.com> wrote:
>
> On 1/22/26 3:42 AM, Jianyue Wu wrote:
> > Replace seq_printf/seq_buf_printf with lightweight helpers to avoid
> > printf parsing in memcg stats output.
>
> Hi Jianyue,
> I gave this patch a run and can confirm the perf gain. I left comments
> on reducing the amount of added lines so that it better resembles the
> existing code.
>
> Tested-by: JP Kobryn <inwardvessel@...il.com>
>
Great, and thanks for the detailed review!

>
> There's a recurring pattern of 1) put name, 2) put separator, 3) put
> value. Instead of adding so many new lines, I wonder if you could use a
> function or macro that accepts: char *name, char sep, u64 val. You could
> then use it as a replacement for seq_printf() and avoid the extra added
> lines here and throughout this patch.
>
That's a good idea! Yes, if we use sep, then many places can use the
same function.

> > +     memory_limit = (u64)memory * PAGE_SIZE;
> > +     memsw_limit = (u64)memsw * PAGE_SIZE;
>
> I don't think in this case these new local variables are improving
> readability.
>
Agree, will remove it. Previously I wanted to keep them inside 80 columns,
as a hack:) Indent is not so easy to change.

> > +             seq_buf_puts(s, "total_");
> > +             memcg_seq_buf_put_name_val(s, memcg1_stat_names[i], (u64)nr);
>
> I would try and combine these 2 calls into 1 if possible. If the diff
> has close to a -1:+1 line change in places where seq_buf_printf() is
> replaced with some helper, it would reduce the noisiness. This applies
> to other areas where a prefix is put before calling a new helper.
>
Agree, I think we can add a prefix param here 0) prefix, 1) put name,
2) put separator, 3) put value. So we can use a common API.

> > +     u64 oom_kill;
> > +
> > +     memcg_seq_put_name_val(sf, "oom_kill_disable",
> > +                            READ_ONCE(memcg->oom_kill_disable));
> > +     memcg_seq_put_name_val(sf, "under_oom", (bool)memcg->under_oom);
> >
> > -     seq_printf(sf, "oom_kill_disable %d\n", READ_ONCE(memcg->oom_kill_disable));
> > -     seq_printf(sf, "under_oom %d\n", (bool)memcg->under_oom);
> > -     seq_printf(sf, "oom_kill %lu\n",
> > -                atomic_long_read(&memcg->memory_events[MEMCG_OOM_KILL]));
> > +     oom_kill = atomic_long_read(&memcg->memory_events[MEMCG_OOM_KILL]);
> > +     memcg_seq_put_name_val(sf, "oom_kill", oom_kill);
>
> New local variable just adding extra lines.
>
Agree, will remove it.

> > +void memcg_seq_put_name_val(struct seq_file *m, const char *name, u64 val)
> > +{
> > +     seq_puts(m, name);
> > +     /* need a space between name and value */
> > +     seq_put_decimal_ull(m, " ", val);
> > +     seq_putc(m, '\n');
>
> I think seq_put* calls normally don't imply a newline. Maybe change the
> name to reflect, like something with "print"? Also, it's not really
> memcg specific.
>
> This function has a space as a separator. Earlier in your diff you were
> using '='. A separator parameter could allow this func to be used
> elsewhere, but you'd have to manage the newline somehow. Maybe a newline
> wrapper?
I think a newline wrapper API might be an extra complexity, maybe this time
I'll keep changes only for which has a newline, places which still
don't have newline,
like memory.numa_stats print still kept using seq_printf() API.
But not sure how perf gain will be, I will test in the next version.

> > +void memcg_seq_buf_put_name_val(struct seq_buf *s, const char *name, u64 val)
> > +{
> > +     char num_buf[MEMCG_DEC_U64_MAX_LEN];
> > +     int num_len;
> > +
> > +     num_len = num_to_str(num_buf, sizeof(num_buf), val, 0);
> > +     if (num_len <= 0)
> > +             return;
> > +
> > +     if (seq_buf_puts(s, name))
> > +             return;
> > +     if (seq_buf_putc(s, ' '))
> > +             return;
>
> Can num_buf[0] just be ' '? The length would have to be extended though.
> Not sure if saving a few seq_buf_putc() calls make a difference.
>
> > +     if (seq_buf_putmem(s, num_buf, num_len))
> > +             return;
> > +     seq_buf_putc(s, '\n');
>
> Similary, though I'm not sure if it even performs better, this call
> could be removed and can do num_buf[num_len+1] = '\n' (extend buf
> again).
>
> If you make the two changes above you can call seq_buf_putmem() last.
Good catch~ Embedding the separator and newline in num_buf reduces
function calls from 4 to 2, perfect!

> > +}
> > +
> >   static void memcg_stat_format(struct mem_cgroup *memcg, struct seq_buf *s)
> >   {
> >       int i;
> > +     u64 pgscan, pgsteal;
> >
> More extra local variables. You can save the lines instead.
>
Agree, will remove it.

> > @@ -4247,7 +4315,8 @@ static int peak_show(struct seq_file *sf, void *v, struct page_counter *pc)
> >       else
> >               peak = max(fd_peak, READ_ONCE(pc->local_watermark));
> >
> > -     seq_printf(sf, "%llu\n", peak * PAGE_SIZE);
> > +     seq_put_decimal_ull(sf, "", peak * PAGE_SIZE);
> > +     seq_putc(sf, '\n');
>
> Your benchmark mentions reading memory and numa stat files, but this
> function is not reached in those cases. Is this a hot path for you? If
> not, maybe just leave this and any others like it alone.
>
This path is not touched by memory.stat. Agree, will remove the change.

> > +     u64 low, high, max, oom, oom_kill;
> > +     u64 oom_group_kill, sock_throttled;
> > +
> Same, more new locals.
Will remove them.

> > +     u64 swap_high, swap_max, swap_fail;
> > +
> > +     swap_high = atomic_long_read(&memcg->memory_events[MEMCG_SWAP_HIGH]);
> > +     swap_max = atomic_long_read(&memcg->memory_events[MEMCG_SWAP_MAX]);
> > +     swap_fail = atomic_long_read(&memcg->memory_events[MEMCG_SWAP_FAIL]);
>
> Same, new local variables.
Will remove them.

Powered by blists - more mailing lists