Re: [PATCH] mm/memcg: bail out early when !memcg in mem_cgroup_lruvec

[Andrew Morton]

On Fri, 27 Nov 2020 11:08:35 +0800 Alex Shi <alex.shi@xxxxxxxxxxxxxxxxx> wrote:

> Sometime, we use NULL memcg in mem_cgroup_lruvec(memcg, pgdat)
> so we could get out early in the situation to avoid useless checking.
> 
> Also warning if both parameter are NULL.

Why do you think a warning is needed here?

> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -613,14 +613,13 @@ static inline struct lruvec *mem_cgroup_lruvec(struct mem_cgroup *memcg,
>  	struct mem_cgroup_per_node *mz;
>  	struct lruvec *lruvec;
>  
> -	if (mem_cgroup_disabled()) {
> +	VM_WARN_ON_ONCE(!memcg && !pgdat);
> +
> +	if (mem_cgroup_disabled() || !memcg) {
>  		lruvec = &pgdat->__lruvec;
>  		goto out;
>  	}
>  
> -	if (!memcg)
> -		memcg = root_mem_cgroup;
> -

This change isn't obviously equivalent, is it?

>  	mz = mem_cgroup_nodeinfo(memcg, pgdat->node_id);
>  	lruvec = &mz->lruvec;
>  out:

And the resulting code is awkward:

	if (mem_cgroup_disabled() || !memcg) {
		lruvec = &pgdat->__lruvec;
		goto out;
	}

	mz = mem_cgroup_nodeinfo(memcg, pgdat->node_id);
	lruvec = &mz->lruvec;
out:


could be

	if (mem_cgroup_disabled() || !memcg) {
		lruvec = &pgdat->__lruvec;
	} else {
		mem_cgroup_per_node mz;

		mz = mem_cgroup_nodeinfo(memcg, pgdat->node_id);
		lruvec = &mz->lruvec;
	}