On Wed 18-12-13 17:16:54, Vladimir Davydov wrote: > First, in memcg_create_kmem_cache() we should issue the write barrier > after the kmem_cache is initialized, but before storing the pointer to > it in its parent's memcg_params. > > Second, we should always issue the read barrier after > cache_from_memcg_idx() to conform with the write barrier. > > Third, its better to use smp_* versions of barriers, because we don't > need them on UP systems.
Please be (much) more verbose on Why. Barriers are tricky and should be documented accordingly. So if you say that we should issue a barrier always be specific why we should do it. > Signed-off-by: Vladimir Davydov <[email protected]> > Cc: Michal Hocko <[email protected]> > Cc: Johannes Weiner <[email protected]> > Cc: Glauber Costa <[email protected]> > Cc: Christoph Lameter <[email protected]> > Cc: Pekka Enberg <[email protected]> > Cc: Andrew Morton <[email protected]> > --- > mm/memcontrol.c | 24 ++++++++++-------------- > mm/slab.h | 6 +++++- > 2 files changed, 15 insertions(+), 15 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index e6ad6ff..e37fdb5 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -3429,12 +3429,14 @@ static struct kmem_cache > *memcg_create_kmem_cache(struct mem_cgroup *memcg, > > atomic_set(&new_cachep->memcg_params->nr_pages , 0); > > - cachep->memcg_params->memcg_caches[idx] = new_cachep; > /* > - * the readers won't lock, make sure everybody sees the updated value, > - * so they won't put stuff in the queue again for no reason > + * Since readers won't lock (see cache_from_memcg_idx()), we need a > + * barrier here to ensure nobody will see the kmem_cache partially > + * initialized. > */ > - wmb(); > + smp_wmb(); > + > + cachep->memcg_params->memcg_caches[idx] = new_cachep; > out: > mutex_unlock(&memcg_cache_mutex); > return new_cachep; > @@ -3573,7 +3575,7 @@ struct kmem_cache *__memcg_kmem_get_cache(struct > kmem_cache *cachep, > gfp_t gfp) > { > struct mem_cgroup *memcg; > - int idx; > + struct kmem_cache *memcg_cachep; > > VM_BUG_ON(!cachep->memcg_params); > VM_BUG_ON(!cachep->memcg_params->is_root_cache); > @@ -3587,15 +3589,9 @@ struct kmem_cache *__memcg_kmem_get_cache(struct > kmem_cache *cachep, > if (!memcg_can_account_kmem(memcg)) > goto out; > > - idx = memcg_cache_id(memcg); > - > - /* > - * barrier to mare sure we're always seeing the up to date value. The > - * code updating memcg_caches will issue a write barrier to match this. > - */ > - read_barrier_depends(); > - if (likely(cache_from_memcg_idx(cachep, idx))) { > - cachep = cache_from_memcg_idx(cachep, idx); > + memcg_cachep = cache_from_memcg_idx(cachep, memcg_cache_id(memcg)); > + if (likely(memcg_cachep)) { > + cachep = memcg_cachep; > goto out; > } > > diff --git a/mm/slab.h b/mm/slab.h > index 0859c42..1d8b53f 100644 > --- a/mm/slab.h > +++ b/mm/slab.h > @@ -163,9 +163,13 @@ static inline const char *cache_name(struct kmem_cache > *s) > static inline struct kmem_cache * > cache_from_memcg_idx(struct kmem_cache *s, int idx) > { > + struct kmem_cache *cachep; > + > if (!s->memcg_params) > return NULL; > - return s->memcg_params->memcg_caches[idx]; > + cachep = s->memcg_params->memcg_caches[idx]; > + smp_read_barrier_depends(); /* see memcg_register_cache() */ > + return cachep; > } > > static inline struct kmem_cache *memcg_root_cache(struct kmem_cache *s) > -- > 1.7.10.4 > -- Michal Hocko SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
