Re: [PATCH v4 24/34] mm/memcontrol.c: Convert to printbuf

[Michal Hocko]

On Mon 20-06-22 11:13:56, Kent Overstreet wrote:
> On Mon, Jun 20, 2022 at 01:37:56PM +0200, Michal Hocko wrote:
> > 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 <kent.overstreet@xxxxxxxxx>
> > 
> > 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.
> 
> Do you want to tone down the hostility? Yeesh.

I have merely pointed out you have ignored my review feedback _twice_
already. Ignoring the review feedback and posting new versions without
questions being addressed is wasting other people's time.

> This patch is part of a wider series that deletes seq_buf, if you missed it here
> you go: https://lore.kernel.org/all/20220620004233.3805-1-kent.overstreet@xxxxxxxxx/

Each patch should have its justification. If the reasoning is that
seq_buf is going away then I can live with that. That is not obvious
from this patch which I care about because it falls into area I maintain
and review. Unlike the rest of the large patchset which I do not really
have time to review in its entirety.

> > 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.
> 
> Ok, _that's_ a technical point we can talk about and address. I'll add
> documentation to the printbuf code that the buffer must be freeable with
> kfree().

Hmm, wouldn't that be just too restrictive without any good reasons?
Maybe there are no seq_buf users currently but if there ever raises a
need for larger buffers then you might want to use kvmalloc for the
allocation and you will need to change all users to use kvfree
(potentially missing some). You could either start requiring kvfree
since the beginning or get rid of exposing internal buffer altogether
and use printbuf_exit in all cases.
-- 
Michal Hocko
SUSE Labs