Re: [PATCH 2/7] mm: shrinker: Add a .to_text() method for shrinkers

[Qi Zheng]

On 2023/11/28 11:53, Kent Overstreet wrote:
> On Tue, Nov 28, 2023 at 11:27:11AM +0800, Muchun Song wrote:
> > > On Nov 25, 2023, at 08:30, Kent Overstreet <kent.overstreet@xxxxxxxxx> wrote:
> > >
> > > On Fri, Nov 24, 2023 at 11:08:11AM +0800, Qi Zheng wrote:
> > > > Hi Kent,
> > > >
> > > > On 2023/11/24 05:24, Kent Overstreet wrote:
> > > > > On Thu, Nov 23, 2023 at 11:32:59AM +0800, Qi Zheng wrote:
> > > > > > > + void (*to_text)(struct seq_buf *, struct shrinker *);
> > > > > >
> > > > > > The "to_text" looks a little strange, how about naming it
> > > > > > "stat_objects"?
> > > > >
> > > > > The convention I've been using heavily in bcachefs is
> > > > > typename_to_text(), or type.to_text(), for debug reports. The
> > > >
> > > > OK.
> > > >
> > > > > consistency is nice.
> > > >
> > > > However, this is inconsistent with the name style of other
> > > > shrinker callbacks. Please use the "objects" suffix. As for
> > > > bcachefs's own callback function, you can use typename_to_text()
> > > > to ensure consistency.
> > >
> > > That would be inconsistent with introducing a convention to the wider
> > > kernel.
> >
> > I don not think .to_text is a good name. I really do not know what it means
> > when I first look at this name. I knew you want to report the objects of
> > shrinks, so why not use .report_objects or stat_objects proposed by Qi.
> > Although .to_text is only used by bcachefs now, shrinker is a general module
> > which is not only serving the bcachefs itself. I think it should be better
> > to use a more straightforward name.
>
> No, .report_objects or .stat_objects would be wrong; this isn't
> generating a report on the objects owned by the shrinker, it's just a
> report on the statistics of the shrinker itself.

Now I think adding this method might not be a good idea. If we allow
shrinkers to report thier own private information, OOM logs may become
cluttered. Most people only care about some general information when
troubleshooting OOM problem, but not the private information of a
shrinker.

So I thought maybe we could add some general statistics to the shrinker,
but adding private ".to_text" method is not necessary.

Also +CC Michal, who is reviwing OOM related patches recently.

Thanks,
Qi

> That's why the convention is typename_to_text() - generate a text
> representation of an object of that type.