Re: [PATCH v1 1/5] mm/shmem: support deterministic charging of tmpfs

[Dave Chinner <david@fromorbit.com>]

On Mon, Nov 08, 2021 at 01:19:55PM -0800, Mina Almasry wrote:
> Add memcg= option to shmem mount.
> 
> Users can specify this option at mount time and all data page charges
> will be charged to the memcg supplied.
.....
> +/*
> + * Returns the memcg to charge for inode pages.  If non-NULL is returned, caller
> + * must drop reference with css_put().  NULL indicates that the inode does not
> + * have a memcg to charge, so the default process based policy should be used.
> + */
> +static struct mem_cgroup *
> +mem_cgroup_mapping_get_charge_target(struct address_space *mapping)
> +{
> +	struct mem_cgroup *memcg;
> +
> +	if (!mapping)
> +		return NULL;
> +
> +	rcu_read_lock();
> +	memcg = rcu_dereference(mapping->host->i_sb->s_memcg_to_charge);

Anything doing pointer chasing to obtain static, unchanging
superblock state is poorly implemented. The s_memcg_to_charge value never
changes, so this code should associate the memcg to charge directly
on the mapping when the mapping is first initialised by the
filesystem. We already do this with things like attaching address
space ops and mapping specific gfp masks (i.e
mapping_set_gfp_mask()), so this association should be set up that
way, too (e.g. mapping_set_memcg_to_charge()).

And because this memcg pointer is static and unchanging for the entire
life of the superblock, the superblock *must* pin the memcg into
memory and that means we can elide the rcu locking altogether in the
fast path for all filesystems that don't support this functionality.
i.e. we can simply do:

	if (!mapping || !mapping->memcg_to_charge)
		return NULL;

And then only if there is a memcg to charge, we do the slow path
locking and lookup stuff...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com