On Fri, Jun 21, 2019 at 01:30:05PM -0400, Waiman Long wrote:
> With Roman's kmem cache reparent patch, multiple kmem caches of the same
> type can be seen attached to the same memcg id. All of them, except
> maybe one, are reparent'ed kmem caches. It can be useful to tag those
> reparented caches by adding a new slab flag "SLAB_DEACTIVATED" to those
> kmem caches that will be reparent'ed if it cannot be destroyed completely.
>
> For the reparent'ed memcg kmem caches, the tag ":deact" will now be
> shown in <debugfs>/memcg_slabinfo.
>
> Signed-off-by: Waiman Long <longman@xxxxxxxxxx>
Hi Waiman!
Sorry for the late reply. The patch overall looks good to me,
except one nit. Please feel free to use my ack:
Acked-by: Roman Gushchin <guro@xxxxxx>
> ---
> include/linux/slab.h | 4 ++++
> mm/slab.c | 1 +
> mm/slab_common.c | 14 ++++++++------
> mm/slub.c | 1 +
> 4 files changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index fecf40b7be69..19ab1380f875 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -116,6 +116,10 @@
> /* Objects are reclaimable */
> #define SLAB_RECLAIM_ACCOUNT ((slab_flags_t __force)0x00020000U)
> #define SLAB_TEMPORARY SLAB_RECLAIM_ACCOUNT /* Objects are short-lived */
> +
> +/* Slab deactivation flag */
> +#define SLAB_DEACTIVATED ((slab_flags_t __force)0x10000000U)
> +
> /*
> * ZERO_SIZE_PTR will be returned for zero sized kmalloc requests.
> *
> diff --git a/mm/slab.c b/mm/slab.c
> index a2e93adf1df0..e8c7743fc283 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2245,6 +2245,7 @@ int __kmem_cache_shrink(struct kmem_cache *cachep)
> #ifdef CONFIG_MEMCG
> void __kmemcg_cache_deactivate(struct kmem_cache *cachep)
> {
> + cachep->flags |= SLAB_DEACTIVATED;
A nit: it can be done from kmemcg_cache_deactivate() instead,
and then you don't have to do it in slab and slub separately.
Since it's not slab- or slub-specific code, it'd be better, IMO,
to put it into slab_common.c.
Thanks!
> __kmem_cache_shrink(cachep);
> }
>
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index 146d8eaa639c..85cf0c374303 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -1533,7 +1533,7 @@ static int memcg_slabinfo_show(struct seq_file *m, void *unused)
> struct slabinfo sinfo;
>
> mutex_lock(&slab_mutex);
> - seq_puts(m, "# <name> <css_id[:dead]> <active_objs> <num_objs>");
> + seq_puts(m, "# <name> <css_id[:dead|deact]> <active_objs> <num_objs>");
> seq_puts(m, " <active_slabs> <num_slabs>\n");
> list_for_each_entry(s, &slab_root_caches, root_caches_node) {
> /*
> @@ -1544,22 +1544,24 @@ static int memcg_slabinfo_show(struct seq_file *m, void *unused)
>
> memset(&sinfo, 0, sizeof(sinfo));
> get_slabinfo(s, &sinfo);
> - seq_printf(m, "%-17s root %6lu %6lu %6lu %6lu\n",
> + seq_printf(m, "%-17s root %6lu %6lu %6lu %6lu\n",
> cache_name(s), sinfo.active_objs, sinfo.num_objs,
> sinfo.active_slabs, sinfo.num_slabs);
>
> for_each_memcg_cache(c, s) {
> struct cgroup_subsys_state *css;
> - char *dead = "";
> + char *status = "";
>
> css = &c->memcg_params.memcg->css;
> if (!(css->flags & CSS_ONLINE))
> - dead = ":dead";
> + status = ":dead";
> + else if (c->flags & SLAB_DEACTIVATED)
> + status = ":deact";
>
> memset(&sinfo, 0, sizeof(sinfo));
> get_slabinfo(c, &sinfo);
> - seq_printf(m, "%-17s %4d%5s %6lu %6lu %6lu %6lu\n",
> - cache_name(c), css->id, dead,
> + seq_printf(m, "%-17s %4d%-6s %6lu %6lu %6lu %6lu\n",
> + cache_name(c), css->id, status,
> sinfo.active_objs, sinfo.num_objs,
> sinfo.active_slabs, sinfo.num_slabs);
> }
> diff --git a/mm/slub.c b/mm/slub.c
> index a384228ff6d3..c965b4413658 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -4057,6 +4057,7 @@ void __kmemcg_cache_deactivate(struct kmem_cache *s)
> */
> slub_set_cpu_partial(s, 0);
> s->min_partial = 0;
> + s->flags |= SLAB_DEACTIVATED;
> }
> #endif /* CONFIG_MEMCG */
>
> --
> 2.18.1
>
