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:   Tue, 6 Jun 2023 08:56:37 -0700
From:   Roman Gushchin <roman.gushchin@linux.dev>
To:     Dave Chinner <david@fromorbit.com>
Cc:     Kirill Tkhai <tkhai@ya.ru>, akpm@linux-foundation.org,
        vbabka@suse.cz, viro@zeniv.linux.org.uk, brauner@kernel.org,
        djwong@kernel.org, hughd@google.com, paulmck@kernel.org,
        muchun.song@linux.dev, linux-mm@kvack.org,
        linux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org,
        linux-kernel@vger.kernel.org, zhengqi.arch@bytedance.com
Subject: Re: [PATCH v2 3/3] fs: Use delayed shrinker unregistration
Message-ID: <ZH9XNUGTW3+sEANc@P9FQF9L96D.corp.robot.car>
References: <168599103578.70911.9402374667983518835.stgit@pro.pro>
 <168599180526.70911.14606767590861123431.stgit@pro.pro>
 <ZH6AA72wOd4HKTKE@P9FQF9L96D>
 <ZH6K0McWBeCjaf16@dread.disaster.area>
 <ZH6ge3yiGAotYRR9@P9FQF9L96D>
 <ZH7XfD/pBcWzhHcc@dread.disaster.area>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZH7XfD/pBcWzhHcc@dread.disaster.area>
Precedence: bulk

On Tue, Jun 06, 2023 at 04:51:40PM +1000, Dave Chinner wrote:
> On Mon, Jun 05, 2023 at 07:56:59PM -0700, Roman Gushchin wrote:
> > On Tue, Jun 06, 2023 at 11:24:32AM +1000, Dave Chinner wrote:
> > > On Mon, Jun 05, 2023 at 05:38:27PM -0700, Roman Gushchin wrote:
> > > > Isn't it possible to hide it from a user and call the second part from a work
> > > > context automatically?
> > > 
> > > Nope, because it has to be done before the struct shrinker is freed.
> > > Those are embedded into other structures rather than being
> > > dynamically allocated objects.
> > 
> > This part we might consider to revisit, if it helps to solve other problems.
> > Having an extra memory allocation (or two) per mount-point doesn't look
> > that expensive. Again, iff it helps with more important problems.
> 
> Ah, I guess if you're concerned about memory allocation overhead
> during register_shrinker() calls then you really aren't familiar
> with what register_shrinker() does on memcg and numa aware
> shrinkers?

What a nice way to agree with an idea :)

> 
> Let's ignore the fact that we could roll the shrinker structure
> allocation into the existing shrinker->nr_deferred array allocation
> (so it's effectively a zero cost modification), and just look at
> what a memcg enabled shrinker must initialise if it expands the
> shrinker info array because the index returned from idr_alloc()
> is larger than the current array:
> 
> 	for each memcg {
> 		for_each_node {
> 			info = kvmalloc_node();
> 			rcu_assign_pointer(memcg->nodeinfo[nid]->shrinker_info, info);
> 		}
> 	}
> 
> Hmmmm?
> 
> So, there really isn't any additional cost, it completely decouples
> the shrinker infrastructure from the subsystem shrinker
> implementations, it enables the shrinker to control infrastructure
> teardown independently of the subsystem that registered the
> shrinker, and it still gives guarantees that the shrinker is never
> run after unregister_shrinker() completes. What's not to like?

Yep, this sounds like a good idea.

Thanks.