On Sun, Jun 19, 2022 at 12:47:39PM -0700, Roman Gushchin wrote: > 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@xxxxxxxxxxxxx> > > 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. > I also thought about this. Open-code is not simplified than this since memcg_reparent_ops can be used for 3 locks which simplifies code a lot. I also want to hear others' thoughts on this. > 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. Maybe we can add some comments explaining the lock dependency depends on the element order in array. > 2) Unlikely there will be a lot of new ops added in the future. > Yep. I think so. Thanks. > The code looks correct though. > > Thanks! >
