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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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.




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux