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
