On Tue, Aug 19, 2025 at 03:46:42AM +0100, Matthew Wilcox wrote:
> On Mon, Aug 18, 2025 at 05:36:53PM -0700, Boris Burkov wrote:
> > +++ b/mm/filemap.c
> > @@ -960,15 +960,19 @@ int filemap_add_folio(struct address_space *mapping, struct folio *folio,
> > {
> > void *shadow = NULL;
> > int ret;
> > + bool charge_mem_cgroup = !test_bit(AS_UNCHARGED, &mapping->flags);
> >
> > - ret = mem_cgroup_charge(folio, NULL, gfp);
> > - if (ret)
> > - return ret;
> > + if (charge_mem_cgroup) {
> > + ret = mem_cgroup_charge(folio, NULL, gfp);
> > + if (ret)
> > + return ret;
> > + }
> >
> > __folio_set_locked(folio);
> > ret = __filemap_add_folio(mapping, folio, index, gfp, &shadow);
> > if (unlikely(ret)) {
> > - mem_cgroup_uncharge(folio);
> > + if (charge_mem_cgroup)
> > + mem_cgroup_uncharge(folio);
> > __folio_clear_locked(folio);
>
> This is unnecessarily complex; mem_cgroup_uncharge() is a no-op if
> the folio is not charged. Sure, it's a wasted function call, but
> this is a rare error path, so minimising size is more important than
> minimising time.
Makes sense, thanks.
>
> So you can drop the 'bool' as well:
>
> + if (!test_bit(AS_UNCHARGED, &mapping->flags)) {
I do use it once more for the stats, so if we do ultimately decide those
are worthwhile, should I keep the bool?
I can also put the checking in the stats function, in which case we
could drop it after all.
