Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sun, 19 Jun 2022 12:47:39 -0700
From:   Roman Gushchin <roman.gushchin@linux.dev>
To:     Muchun Song <songmuchun@bytedance.com>
Cc:     hannes@cmpxchg.org, mhocko@kernel.org, shakeelb@google.com,
        akpm@linux-foundation.org, cgroups@vger.kernel.org,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        duanxiongchun@bytedance.com, longman@redhat.com
Subject: Re: [PATCH v5 08/11] mm: memcontrol: introduce memcg_reparent_ops
Message-ID: <Yq99W9ps1b/+XOwh@castle>
References: <20220530074919.46352-1-songmuchun@bytedance.com>
 <20220530074919.46352-9-songmuchun@bytedance.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220530074919.46352-9-songmuchun@bytedance.com>
Precedence: bulk

On Mon, May 30, 2022 at 03:49:16PM +0800, Muchun Song wrote:
> In the previous patch, we know how to make the lruvec lock safe when LRU
> pages are reparented. We should do something like following.
> 
>     memcg_reparent_objcgs(memcg)
>         1) lock
>         // lruvec belongs to memcg and lruvec_parent belongs to parent memcg.
>         spin_lock(&lruvec->lru_lock);
>         spin_lock(&lruvec_parent->lru_lock);
> 
>         2) relocate from current memcg to its parent
>         // Move all the pages from the lruvec list to the parent lruvec list.
> 
>         3) unlock
>         spin_unlock(&lruvec_parent->lru_lock);
>         spin_unlock(&lruvec->lru_lock);
> 
> Apart from the page lruvec lock, the deferred split queue lock (THP only)
> also needs to do something similar. So we extract the necessary three steps
> in the memcg_reparent_objcgs().
> 
>     memcg_reparent_objcgs(memcg)
>         1) lock
>         memcg_reparent_ops->lock(memcg, parent);
> 
>         2) relocate
>         memcg_reparent_ops->relocate(memcg, reparent);
> 
>         3) unlock
>         memcg_reparent_ops->unlock(memcg, reparent);
> 
> Now there are two different locks (e.g. lruvec lock and deferred split
> queue lock) need to use this infrastructure. In the next patch, we will
> use those APIs to make those locks safe when the LRU pages reparented.
> 
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>

I've mixed feelings about this: it looks nice, but maybe too nice. I wonder
if it's better to open-code it. Not very confident, I wonder what others are
thinking.

1) Because the lock callback is first called for all ops, then relocate, then
unlock, implicit lock dependencies are created. Now it depends on the order
of elements in the memcg_reparent_ops array, which isn't very obvious.
2) Unlikely there will be a lot of new ops added in the future.

The code looks correct though.

Thanks!