Re: [PATCH v6 07/10] mm: synchronize access to kmem_cache dying flag using a spinlock

[Roman Gushchin]

On Wed, Jun 05, 2019 at 03:02:03PM -0700, Roman Gushchin wrote:
> On Wed, Jun 05, 2019 at 12:56:16PM -0400, Johannes Weiner wrote:
> > On Tue, Jun 04, 2019 at 07:44:51PM -0700, Roman Gushchin wrote:
> > > Currently the memcg_params.dying flag and the corresponding
> > > workqueue used for the asynchronous deactivation of kmem_caches
> > > is synchronized using the slab_mutex.
> > > 
> > > It makes impossible to check this flag from the irq context,
> > > which will be required in order to implement asynchronous release
> > > of kmem_caches.
> > > 
> > > So let's switch over to the irq-save flavor of the spinlock-based
> > > synchronization.
> > > 
> > > Signed-off-by: Roman Gushchin <guro@xxxxxx>
> > > ---
> > >  mm/slab_common.c | 19 +++++++++++++++----
> > >  1 file changed, 15 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/mm/slab_common.c b/mm/slab_common.c
> > > index 09b26673b63f..2914a8f0aa85 100644
> > > --- a/mm/slab_common.c
> > > +++ b/mm/slab_common.c
> > > @@ -130,6 +130,7 @@ int __kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t nr,
> > >  #ifdef CONFIG_MEMCG_KMEM
> > >  
> > >  LIST_HEAD(slab_root_caches);
> > > +static DEFINE_SPINLOCK(memcg_kmem_wq_lock);
> > >  
> > >  void slab_init_memcg_params(struct kmem_cache *s)
> > >  {
> > > @@ -629,6 +630,7 @@ void memcg_create_kmem_cache(struct mem_cgroup *memcg,
> > >  	struct memcg_cache_array *arr;
> > >  	struct kmem_cache *s = NULL;
> > >  	char *cache_name;
> > > +	bool dying;
> > >  	int idx;
> > >  
> > >  	get_online_cpus();
> > > @@ -640,7 +642,13 @@ void memcg_create_kmem_cache(struct mem_cgroup *memcg,
> > >  	 * The memory cgroup could have been offlined while the cache
> > >  	 * creation work was pending.
> > >  	 */
> > > -	if (memcg->kmem_state != KMEM_ONLINE || root_cache->memcg_params.dying)
> > > +	if (memcg->kmem_state != KMEM_ONLINE)
> > > +		goto out_unlock;
> > > +
> > > +	spin_lock_irq(&memcg_kmem_wq_lock);
> > > +	dying = root_cache->memcg_params.dying;
> > > +	spin_unlock_irq(&memcg_kmem_wq_lock);
> > > +	if (dying)
> > >  		goto out_unlock;
> > 
> > What does this lock protect? The dying flag could get set right after
> > the unlock.
> >
> 
> Hi Johannes!
> 
> Here is my logic:
> 
> 1) flush_memcg_workqueue() must guarantee that no new memcg kmem_caches
> will be created, and there are no works queued, which will touch
> the root kmem_cache, so it can be released
> 2) so it sets the dying flag, waits for an rcu grace period and flushes
> the workqueue (that means for all in-flight works)
> 3) dying flag in checked in kmemcg_cache_shutdown() and
> kmemcg_cache_deactivate(), so that if it set, no new works/rcu tasks
> will be queued. corresponding queue_work()/call_rcu() are all under
> memcg_kmem_wq_lock lock.
> 4) memcg_schedule_kmem_cache_create() doesn't check the dying flag
> (probably to avoid taking locks on a hot path), but it does
> memcg_create_kmem_cache(), which is part of the scheduled work.
> And it does it at the very beginning, so even if new kmem_caches
> are scheduled to be created, the root kmem_cache won't be touched.
> 
> Previously the flag was checked under slab_mutex, but now we set it
> under memcg_kmem_wq_lock lock. So I'm not sure we can read it without
> taking this lock.
> 
> If the flag will be set after unlock, it's fine. It means that the
> work has already been scheduled, and flush_workqueue() in
> flush_memcg_workqueue() will wait for it. The only problem is if we
> don't see the flag after flush_workqueue() is called, but I don't
> see how it's possible.
> 
> Does it makes sense? I'm sure there are ways to make it more obvious.
> Please, let me know if you've any ideas.

Hm, after some thoughts, I've found that the problem is that we check
the dying flag of the root cache. But it's the same in the existing code.

So currently (without my patches):
1) we do set the dying flag under slab_mutex
2) waiting for the workqueue to flush
3) grabbing the slab_mutex and going to release the root kmem_cache

a concurrent memcg_kmem_cache_create_func() can be scheduled after 2),
grab the slab_mutex after 3) and check the kmem_cache->memcg_params.dying
flag of already released kmem_cache.

The reason why it's not a real problem is that it's expected from a user
that kmem_cache will not be used for new allocations after calling
kmem_cache_destroy(). It means no new memcg kmem_cache creation will be
scheduled, and we can avoid checking the dying flag at all.

Does this makes sense?

Thanks!