Re: [PATCH v3 1/4] mm/filemap: add AS_UNCHARGED

[Boris Burkov <boris@bur.io>]

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.