All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5] mm: slub: move sysfs slab alloc/free interfaces to debugfs
@ 2021-05-06 10:22 Faiyaz Mohammed
  2021-05-06 14:29   ` kernel test robot
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Faiyaz Mohammed @ 2021-05-06 10:22 UTC (permalink / raw)
  To: cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, linux-mm,
	linux-kernel, glittao
  Cc: vinmenon, Faiyaz Mohammed

alloc_calls and free_calls implementation in sysfs have two issues,
one is PAGE_SIZE limitiation of sysfs and other is it does not adhere
to "one value per file" rule.

To overcome this issues, move the alloc_calls and free_calls implemeation
to debugfs.

Rename the alloc_calls/free_calls to alloc_traces/free_traces,
to be inline with what it does.

Signed-off-by: Faiyaz Mohammed <faiyazm@codeaurora.org>
---
 include/linux/slub_def.h |  10 ++
 mm/slab_common.c         |   9 ++
 mm/slub.c                | 362 ++++++++++++++++++++++++++++++++++++-----------
 3 files changed, 299 insertions(+), 82 deletions(-)

diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
index dcde82a..f8c268d 100644
--- a/include/linux/slub_def.h
+++ b/include/linux/slub_def.h
@@ -110,6 +110,9 @@ struct kmem_cache {
 #ifdef CONFIG_SYSFS
 	struct kobject kobj;	/* For sysfs */
 #endif
+#ifdef CONFIG_SLUB_DEBUG
+	struct dentry *slab_cache_dentry;
+#endif
 #ifdef CONFIG_SLAB_FREELIST_HARDENED
 	unsigned long random;
 #endif
@@ -159,6 +162,13 @@ static inline void sysfs_slab_release(struct kmem_cache *s)
 }
 #endif
 
+#ifdef CONFIG_DEBUG_FS
+void debugfs_slab_release(struct kmem_cache *);
+#else
+static inline void debugfs_slab_release(struct kmem_cache *s)
+{
+}
+#endif
 void object_err(struct kmem_cache *s, struct page *page,
 		u8 *object, char *reason);
 
diff --git a/mm/slab_common.c b/mm/slab_common.c
index f8833d3..f3afe6b 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -445,6 +445,9 @@ static void slab_caches_to_rcu_destroy_workfn(struct work_struct *work)
 #else
 		slab_kmem_cache_release(s);
 #endif
+#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_SLUB)
+		debugfs_slab_release(s);
+#endif
 	}
 }
 
@@ -462,6 +465,9 @@ static int shutdown_cache(struct kmem_cache *s)
 #ifdef SLAB_SUPPORTS_SYSFS
 		sysfs_slab_unlink(s);
 #endif
+#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_SLUB)
+		debugfs_slab_release(s);
+#endif
 		list_add_tail(&s->list, &slab_caches_to_rcu_destroy);
 		schedule_work(&slab_caches_to_rcu_destroy_work);
 	} else {
@@ -472,6 +478,9 @@ static int shutdown_cache(struct kmem_cache *s)
 #else
 		slab_kmem_cache_release(s);
 #endif
+#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_SLUB)
+		debugfs_slab_release(s);
+#endif
 	}
 
 	return 0;
diff --git a/mm/slub.c b/mm/slub.c
index 68123b2..a5347f1 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -36,6 +36,7 @@
 #include <linux/memcontrol.h>
 #include <linux/random.h>
 
+#include <linux/debugfs.h>
 #include <trace/events/kmem.h>
 
 #include "internal.h"
@@ -225,6 +226,15 @@ static inline int sysfs_slab_alias(struct kmem_cache *s, const char *p)
 							{ return 0; }
 #endif
 
+#ifdef CONFIG_DEBUG_FS
+static void debugfs_slab_add(struct kmem_cache *);
+static int debugfs_slab_alias(struct kmem_cache *, const char *);
+#else
+static inline void debugfs_slab_add(struct kmem_cache *s) { }
+static inline int debugfs_slab_alias(struct kmem_cache *s, const char *p)
+							{ return 0; }
+#endif
+
 static inline void stat(const struct kmem_cache *s, enum stat_item si)
 {
 #ifdef CONFIG_SLUB_STATS
@@ -4533,6 +4543,8 @@ __kmem_cache_alias(const char *name, unsigned int size, unsigned int align,
 			s->refcount--;
 			s = NULL;
 		}
+
+		debugfs_slab_alias(s, name);
 	}
 
 	return s;
@@ -4554,6 +4566,8 @@ int __kmem_cache_create(struct kmem_cache *s, slab_flags_t flags)
 	if (err)
 		__kmem_cache_release(s);
 
+	debugfs_slab_add(s);
+
 	return err;
 }
 
@@ -4694,6 +4708,8 @@ static long validate_slab_cache(struct kmem_cache *s)
 
 	return count;
 }
+
+#ifdef CONFIG_DEBUG_FS
 /*
  * Generate lists of code addresses where slabcache objects are allocated
  * and freed.
@@ -4717,6 +4733,9 @@ struct loc_track {
 	struct location *loc;
 };
 
+static struct dentry *slab_debugfs_root;
+struct loc_track t = { 0, 0, NULL };
+
 static void free_loc_track(struct loc_track *t)
 {
 	if (t->max)
@@ -4833,82 +4852,7 @@ static void process_slab(struct loc_track *t, struct kmem_cache *s,
 			add_location(t, s, get_track(s, p, alloc));
 	put_map(map);
 }
-
-static int list_locations(struct kmem_cache *s, char *buf,
-			  enum track_item alloc)
-{
-	int len = 0;
-	unsigned long i;
-	struct loc_track t = { 0, 0, NULL };
-	int node;
-	struct kmem_cache_node *n;
-
-	if (!alloc_loc_track(&t, PAGE_SIZE / sizeof(struct location),
-			     GFP_KERNEL)) {
-		return sysfs_emit(buf, "Out of memory\n");
-	}
-	/* Push back cpu slabs */
-	flush_all(s);
-
-	for_each_kmem_cache_node(s, node, n) {
-		unsigned long flags;
-		struct page *page;
-
-		if (!atomic_long_read(&n->nr_slabs))
-			continue;
-
-		spin_lock_irqsave(&n->list_lock, flags);
-		list_for_each_entry(page, &n->partial, slab_list)
-			process_slab(&t, s, page, alloc);
-		list_for_each_entry(page, &n->full, slab_list)
-			process_slab(&t, s, page, alloc);
-		spin_unlock_irqrestore(&n->list_lock, flags);
-	}
-
-	for (i = 0; i < t.count; i++) {
-		struct location *l = &t.loc[i];
-
-		len += sysfs_emit_at(buf, len, "%7ld ", l->count);
-
-		if (l->addr)
-			len += sysfs_emit_at(buf, len, "%pS", (void *)l->addr);
-		else
-			len += sysfs_emit_at(buf, len, "<not-available>");
-
-		if (l->sum_time != l->min_time)
-			len += sysfs_emit_at(buf, len, " age=%ld/%ld/%ld",
-					     l->min_time,
-					     (long)div_u64(l->sum_time,
-							   l->count),
-					     l->max_time);
-		else
-			len += sysfs_emit_at(buf, len, " age=%ld", l->min_time);
-
-		if (l->min_pid != l->max_pid)
-			len += sysfs_emit_at(buf, len, " pid=%ld-%ld",
-					     l->min_pid, l->max_pid);
-		else
-			len += sysfs_emit_at(buf, len, " pid=%ld",
-					     l->min_pid);
-
-		if (num_online_cpus() > 1 &&
-		    !cpumask_empty(to_cpumask(l->cpus)))
-			len += sysfs_emit_at(buf, len, " cpus=%*pbl",
-					     cpumask_pr_args(to_cpumask(l->cpus)));
-
-		if (nr_online_nodes > 1 && !nodes_empty(l->nodes))
-			len += sysfs_emit_at(buf, len, " nodes=%*pbl",
-					     nodemask_pr_args(&l->nodes));
-
-		len += sysfs_emit_at(buf, len, "\n");
-	}
-
-	free_loc_track(&t);
-	if (!t.count)
-		len += sysfs_emit_at(buf, len, "No data\n");
-
-	return len;
-}
+#endif  /* CONFIG_DEBUG_FS   */
 #endif	/* CONFIG_SLUB_DEBUG */
 
 #ifdef SLUB_RESILIENCY_TEST
@@ -5360,17 +5304,25 @@ SLAB_ATTR(validate);
 
 static ssize_t alloc_calls_show(struct kmem_cache *s, char *buf)
 {
-	if (!(s->flags & SLAB_STORE_USER))
-		return -ENOSYS;
-	return list_locations(s, buf, TRACK_ALLOC);
+	int len = 0;
+
+	len += sprintf(buf + len, "Deprecated, use the equvalent under\n");
+	len += sprintf(buf + len, "/sys/kernel/debug/slab/%s/alloc_traces\n",
+			s->name);
+
+	return len;
 }
 SLAB_ATTR_RO(alloc_calls);
 
 static ssize_t free_calls_show(struct kmem_cache *s, char *buf)
 {
-	if (!(s->flags & SLAB_STORE_USER))
-		return -ENOSYS;
-	return list_locations(s, buf, TRACK_FREE);
+	int len = 0;
+
+	len += sprintf(buf + len, "Deprecated, use the equvalent under\n");
+	len += sprintf(buf + len, "/sys/kernel/debug/slab/%s/free_traces\n",
+			s->name);
+
+	return len;
 }
 SLAB_ATTR_RO(free_calls);
 #endif /* CONFIG_SLUB_DEBUG */
@@ -5826,6 +5778,252 @@ static int __init slab_sysfs_init(void)
 __initcall(slab_sysfs_init);
 #endif /* CONFIG_SYSFS */
 
+#if defined(CONFIG_SLUB_DEBUG) && defined(CONFIG_DEBUG_FS)
+static int debugfs_slab_alias(struct kmem_cache *s, const char *name)
+{
+	struct saved_alias *al;
+
+	if (slab_state == FULL) {
+		/*
+		 * If we have a leftover link then remove it.
+		 */
+		debugfs_remove(s->slab_cache_dentry);
+		s->slab_cache_dentry = debugfs_create_symlink(name, slab_debugfs_root, NULL);
+		return IS_ERR(s->slab_cache_dentry);
+	}
+
+	al = kmalloc(sizeof(struct saved_alias), GFP_KERNEL);
+	if (!al)
+		return -ENOMEM;
+
+	al->s = s;
+	al->name = name;
+	al->next = alias_list;
+	alias_list = al;
+	return 0;
+}
+
+static int slab_debugfs_show(struct seq_file *seq, void *v)
+{
+
+	struct location *l;
+	unsigned int idx = *(unsigned int *)v;
+
+	if (idx < t.count) {
+		l = &t.loc[idx];
+
+		seq_printf(seq, "%7ld ", l->count);
+
+		if (l->addr)
+			seq_printf(seq, "%pS", (void *)l->addr);
+		else
+			seq_puts(seq, "<not-available>");
+
+		if (l->sum_time != l->min_time) {
+			seq_printf(seq, " age=%ld/%ld/%ld",
+				l->min_time,
+				(long)div_u64(l->sum_time, l->count),
+				l->max_time);
+		} else
+			seq_printf(seq, " age=%ld",
+				l->min_time);
+
+		if (l->min_pid != l->max_pid)
+			seq_printf(seq, " pid=%ld-%ld",
+				l->min_pid, l->max_pid);
+		else
+			seq_printf(seq, " pid=%ld",
+				l->min_pid);
+
+		if (num_online_cpus() > 1 &&
+				!cpumask_empty(to_cpumask(l->cpus)))
+			seq_printf(seq, " cpus=%*pbl",
+				 cpumask_pr_args(to_cpumask(l->cpus)));
+
+		if (nr_online_nodes > 1 && !nodes_empty(l->nodes))
+			seq_printf(seq, " nodes=%*pbl",
+				 nodemask_pr_args(&l->nodes));
+
+		seq_puts(seq, "\n");
+	}
+
+	if (t.count == 0)
+		seq_puts(seq, "No data\n");
+
+	return 0;
+}
+
+static void slab_debugfs_stop(struct seq_file *seq, void *v)
+{
+	if (!v && t.max) {
+		free_loc_track(&t);
+		t.max = 0;
+	}
+}
+
+static void *slab_debugfs_next(struct seq_file *seq, void *v, loff_t *ppos)
+{
+	loff_t *spos = v;
+
+	if (*ppos < t.count) {
+		*spos = *spos + 1;
+		*ppos = *spos;
+		return spos;
+	}
+
+	return NULL;
+}
+
+static void *slab_debugfs_start(struct seq_file *seq, loff_t *ppos)
+{
+	struct kmem_cache_node *n;
+	struct kmem_cache *s;
+	enum track_item alloc;
+	int node;
+	loff_t *spos = kmalloc(sizeof(loff_t), GFP_KERNEL);
+
+	s = seq->file->f_inode->i_private;
+
+	if (!spos)
+		return NULL;
+
+	if (!(s->flags & SLAB_STORE_USER))
+		return ERR_PTR(-EOPNOTSUPP);
+
+	if (*ppos == 0) {
+
+		t.count = 0;
+		t.max = 0;
+		t.loc = NULL;
+		if (strcmp(seq->file->f_path.dentry->d_name.name, "alloc_traces") == 0)
+			alloc =  TRACK_ALLOC;
+		else
+			alloc =  TRACK_FREE;
+
+		if (!alloc_loc_track(&t, PAGE_SIZE / sizeof(struct location),
+			     GFP_KERNEL)) {
+			seq_puts(seq, "Out of memory\n");
+			return ERR_PTR(-ENOMEM);
+		}
+		/* Push back cpu slabs */
+		flush_all(s);
+
+		for_each_kmem_cache_node(s, node, n) {
+			unsigned long flags;
+			struct page *page;
+
+			if (!atomic_long_read(&n->nr_slabs))
+				continue;
+
+			spin_lock_irqsave(&n->list_lock, flags);
+			list_for_each_entry(page, &n->partial, slab_list)
+				process_slab(&t, s, page, alloc);
+			list_for_each_entry(page, &n->full, slab_list)
+				process_slab(&t, s, page, alloc);
+			spin_unlock_irqrestore(&n->list_lock, flags);
+		}
+	}
+
+	if (*ppos < t.count) {
+		*spos = *ppos;
+		return spos;
+	}
+
+	kfree(spos);
+	return NULL;
+}
+
+static const struct seq_operations slab_debugfs_sops = {
+	.start  = slab_debugfs_start,
+	.next   = slab_debugfs_next,
+	.stop   = slab_debugfs_stop,
+	.show   = slab_debugfs_show
+};
+DEFINE_SEQ_ATTRIBUTE(slab_debugfs);
+
+static void debugfs_slab_add(struct kmem_cache *s)
+{
+	const char *name;
+	int unmergeable = slab_unmergeable(s);
+
+	if (unlikely(!slab_debugfs_root))
+		return;
+
+	if (!unmergeable && disable_higher_order_debug &&
+			(slub_debug & DEBUG_METADATA_FLAGS))
+		unmergeable = 1;
+
+	if (unmergeable) {
+		/*
+		 * Slabcache can never be merged so we can use the name proper.
+		 * This is typically the case for debug situations. In that
+		 * case we can catch duplicate names easily.
+		 */
+		debugfs_remove_recursive(s->slab_cache_dentry);
+		name = s->name;
+	} else {
+		/*
+		 * Create a unique name for the slab as a target
+		 * for the symlinks.
+		 */
+		name = create_unique_id(s);
+	}
+
+	s->slab_cache_dentry = debugfs_create_dir(name, slab_debugfs_root);
+	if (!IS_ERR(s->slab_cache_dentry)) {
+		debugfs_create_file("alloc_traces", 0400,
+			s->slab_cache_dentry, s, &slab_debugfs_fops);
+
+		debugfs_create_file("free_traces", 0400,
+			s->slab_cache_dentry, s, &slab_debugfs_fops);
+	}
+
+	if (!unmergeable) {
+		/* Setup first alias */
+		debugfs_slab_alias(s, s->name);
+	}
+}
+
+void debugfs_slab_release(struct kmem_cache *s)
+{
+	if (slab_state >= FULL)
+		debugfs_remove_recursive(s->slab_cache_dentry);
+}
+
+static int __init slab_debugfs_init(void)
+{
+	struct kmem_cache *s;
+	int err;
+
+	slab_debugfs_root = debugfs_create_dir("slab", NULL);
+	if (!IS_ERR(slab_debugfs_root)) {
+
+		slab_state = FULL;
+
+		list_for_each_entry(s, &slab_caches, list)
+			debugfs_slab_add(s);
+	} else {
+		pr_err("Cannot create slab debugfs.\n");
+		return IS_ERR(slab_debugfs_root);
+	}
+
+	while (alias_list) {
+		struct saved_alias *al = alias_list;
+
+		alias_list = alias_list->next;
+
+		err = debugfs_slab_alias(al->s, al->name);
+		if (err)
+			pr_err("SLUB: Unable to add boot slab alias %s to debugfs\n",
+			       al->name);
+		kfree(al);
+	}
+
+	return 0;
+
+}
+__initcall(slab_debugfs_init);
+#endif
 /*
  * The /proc/slabinfo ABI
  */
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
member of the Code Aurora Forum, hosted by The Linux Foundation


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] mm: slub: move sysfs slab alloc/free interfaces to debugfs
  2021-05-06 10:22 [PATCH v5] mm: slub: move sysfs slab alloc/free interfaces to debugfs Faiyaz Mohammed
@ 2021-05-06 14:29   ` kernel test robot
  2021-05-06 14:32   ` kernel test robot
  2021-05-07 11:48 ` Greg KH
  2 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2021-05-06 14:29 UTC (permalink / raw)
  To: Faiyaz Mohammed, cl, penberg, rientjes, iamjoonsoo.kim, akpm,
	vbabka, linux-mm, linux-kernel, glittao
  Cc: kbuild-all, vinmenon

[-- Attachment #1: Type: text/plain, Size: 2404 bytes --]

Hi Faiyaz,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.12]
[cannot apply to hnaz-linux-mm/master next-20210506]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Faiyaz-Mohammed/mm-slub-move-sysfs-slab-alloc-free-interfaces-to-debugfs/20210506-182420
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 8404c9fbc84b741f66cff7d4934a25dd2c344452
config: mips-randconfig-r005-20210506 (attached as .config)
compiler: mipsel-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/88e180b99466c1298b9454c7cf40d571cc92b7cc
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Faiyaz-Mohammed/mm-slub-move-sysfs-slab-alloc-free-interfaces-to-debugfs/20210506-182420
        git checkout 88e180b99466c1298b9454c7cf40d571cc92b7cc
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   mipsel-linux-ld: mm/slab_common.o: in function `slab_caches_to_rcu_destroy_workfn':
>> slab_common.c:(.text.slab_caches_to_rcu_destroy_workfn+0xac): undefined reference to `debugfs_slab_release'
   mipsel-linux-ld: mm/slab_common.o: in function `kmem_cache_destroy':
>> (.text.kmem_cache_destroy+0xd4): undefined reference to `debugfs_slab_release'
>> mipsel-linux-ld: (.text.kmem_cache_destroy+0x138): undefined reference to `debugfs_slab_release'
   mipsel-linux-ld: mm/slub.o: in function `__kmem_cache_alias':
>> (.text.__kmem_cache_alias+0xac): undefined reference to `debugfs_slab_alias'
   mipsel-linux-ld: mm/slub.o: in function `__kmem_cache_create':
>> (.text.__kmem_cache_create+0x4dc): undefined reference to `debugfs_slab_add'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26191 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] mm: slub: move sysfs slab alloc/free interfaces to debugfs
@ 2021-05-06 14:29   ` kernel test robot
  0 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2021-05-06 14:29 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 2449 bytes --]

Hi Faiyaz,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.12]
[cannot apply to hnaz-linux-mm/master next-20210506]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Faiyaz-Mohammed/mm-slub-move-sysfs-slab-alloc-free-interfaces-to-debugfs/20210506-182420
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 8404c9fbc84b741f66cff7d4934a25dd2c344452
config: mips-randconfig-r005-20210506 (attached as .config)
compiler: mipsel-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/88e180b99466c1298b9454c7cf40d571cc92b7cc
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Faiyaz-Mohammed/mm-slub-move-sysfs-slab-alloc-free-interfaces-to-debugfs/20210506-182420
        git checkout 88e180b99466c1298b9454c7cf40d571cc92b7cc
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=mips 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   mipsel-linux-ld: mm/slab_common.o: in function `slab_caches_to_rcu_destroy_workfn':
>> slab_common.c:(.text.slab_caches_to_rcu_destroy_workfn+0xac): undefined reference to `debugfs_slab_release'
   mipsel-linux-ld: mm/slab_common.o: in function `kmem_cache_destroy':
>> (.text.kmem_cache_destroy+0xd4): undefined reference to `debugfs_slab_release'
>> mipsel-linux-ld: (.text.kmem_cache_destroy+0x138): undefined reference to `debugfs_slab_release'
   mipsel-linux-ld: mm/slub.o: in function `__kmem_cache_alias':
>> (.text.__kmem_cache_alias+0xac): undefined reference to `debugfs_slab_alias'
   mipsel-linux-ld: mm/slub.o: in function `__kmem_cache_create':
>> (.text.__kmem_cache_create+0x4dc): undefined reference to `debugfs_slab_add'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 26191 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] mm: slub: move sysfs slab alloc/free interfaces to debugfs
  2021-05-06 10:22 [PATCH v5] mm: slub: move sysfs slab alloc/free interfaces to debugfs Faiyaz Mohammed
@ 2021-05-06 14:32   ` kernel test robot
  2021-05-06 14:32   ` kernel test robot
  2021-05-07 11:48 ` Greg KH
  2 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2021-05-06 14:32 UTC (permalink / raw)
  To: Faiyaz Mohammed, cl, penberg, rientjes, iamjoonsoo.kim, akpm,
	vbabka, linux-mm, linux-kernel, glittao
  Cc: kbuild-all, vinmenon

[-- Attachment #1: Type: text/plain, Size: 2328 bytes --]

Hi Faiyaz,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.12]
[cannot apply to hnaz-linux-mm/master next-20210506]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Faiyaz-Mohammed/mm-slub-move-sysfs-slab-alloc-free-interfaces-to-debugfs/20210506-182420
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 8404c9fbc84b741f66cff7d4934a25dd2c344452
config: ia64-randconfig-r015-20210506 (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/88e180b99466c1298b9454c7cf40d571cc92b7cc
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Faiyaz-Mohammed/mm-slub-move-sysfs-slab-alloc-free-interfaces-to-debugfs/20210506-182420
        git checkout 88e180b99466c1298b9454c7cf40d571cc92b7cc
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=ia64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ia64-linux-ld: mm/slab_common.o: in function `slab_caches_to_rcu_destroy_workfn':
>> slab_common.c:(.text+0x172): undefined reference to `debugfs_slab_release'
   ia64-linux-ld: mm/slab_common.o: in function `kmem_cache_destroy':
   slab_common.c:(.text+0xec2): undefined reference to `debugfs_slab_release'
>> ia64-linux-ld: slab_common.c:(.text+0xf82): undefined reference to `debugfs_slab_release'
   ia64-linux-ld: mm/slub.o: in function `__kmem_cache_alias':
>> slub.c:(.text+0x8a42): undefined reference to `debugfs_slab_alias'
   ia64-linux-ld: mm/slub.o: in function `__kmem_cache_create':
>> slub.c:(.text+0x9a82): undefined reference to `debugfs_slab_add'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 22431 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] mm: slub: move sysfs slab alloc/free interfaces to debugfs
@ 2021-05-06 14:32   ` kernel test robot
  0 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2021-05-06 14:32 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 2373 bytes --]

Hi Faiyaz,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.12]
[cannot apply to hnaz-linux-mm/master next-20210506]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Faiyaz-Mohammed/mm-slub-move-sysfs-slab-alloc-free-interfaces-to-debugfs/20210506-182420
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 8404c9fbc84b741f66cff7d4934a25dd2c344452
config: ia64-randconfig-r015-20210506 (attached as .config)
compiler: ia64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/88e180b99466c1298b9454c7cf40d571cc92b7cc
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Faiyaz-Mohammed/mm-slub-move-sysfs-slab-alloc-free-interfaces-to-debugfs/20210506-182420
        git checkout 88e180b99466c1298b9454c7cf40d571cc92b7cc
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=ia64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ia64-linux-ld: mm/slab_common.o: in function `slab_caches_to_rcu_destroy_workfn':
>> slab_common.c:(.text+0x172): undefined reference to `debugfs_slab_release'
   ia64-linux-ld: mm/slab_common.o: in function `kmem_cache_destroy':
   slab_common.c:(.text+0xec2): undefined reference to `debugfs_slab_release'
>> ia64-linux-ld: slab_common.c:(.text+0xf82): undefined reference to `debugfs_slab_release'
   ia64-linux-ld: mm/slub.o: in function `__kmem_cache_alias':
>> slub.c:(.text+0x8a42): undefined reference to `debugfs_slab_alias'
   ia64-linux-ld: mm/slub.o: in function `__kmem_cache_create':
>> slub.c:(.text+0x9a82): undefined reference to `debugfs_slab_add'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 22431 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] mm: slub: move sysfs slab alloc/free interfaces to debugfs
  2021-05-06 10:22 [PATCH v5] mm: slub: move sysfs slab alloc/free interfaces to debugfs Faiyaz Mohammed
  2021-05-06 14:29   ` kernel test robot
@ 2021-05-07  5:21 ` Dan Carpenter
  2021-05-07 11:48 ` Greg KH
  2 siblings, 0 replies; 13+ messages in thread
From: kernel test robot @ 2021-05-06 18:40 UTC (permalink / raw)
  To: kbuild

[-- Attachment #1: Type: text/plain, Size: 5986 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <1620296523-21922-1-git-send-email-faiyazm@codeaurora.org>
References: <1620296523-21922-1-git-send-email-faiyazm@codeaurora.org>
TO: Faiyaz Mohammed <faiyazm@codeaurora.org>
TO: cl(a)linux.com
TO: penberg(a)kernel.org
TO: rientjes(a)google.com
TO: iamjoonsoo.kim(a)lge.com
TO: akpm(a)linux-foundation.org
TO: vbabka(a)suse.cz
TO: linux-mm(a)kvack.org
TO: linux-kernel(a)vger.kernel.org
TO: glittao(a)gmail.com
CC: vinmenon(a)codeaurora.org

Hi Faiyaz,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.12]
[cannot apply to hnaz-linux-mm/master next-20210506]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Faiyaz-Mohammed/mm-slub-move-sysfs-slab-alloc-free-interfaces-to-debugfs/20210506-182420
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 8404c9fbc84b741f66cff7d4934a25dd2c344452
:::::: branch date: 8 hours ago
:::::: commit date: 8 hours ago
config: i386-randconfig-m021-20210506 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
mm/slub.c:5891 slab_debugfs_start() warn: possible memory leak of 'spos'

vim +/spos +5891 mm/slub.c

88e180b99466c1 Faiyaz Mohammed 2021-05-06  5876  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5877  static void *slab_debugfs_start(struct seq_file *seq, loff_t *ppos)
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5878  {
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5879  	struct kmem_cache_node *n;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5880  	struct kmem_cache *s;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5881  	enum track_item alloc;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5882  	int node;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5883  	loff_t *spos = kmalloc(sizeof(loff_t), GFP_KERNEL);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5884  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5885  	s = seq->file->f_inode->i_private;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5886  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5887  	if (!spos)
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5888  		return NULL;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5889  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5890  	if (!(s->flags & SLAB_STORE_USER))
88e180b99466c1 Faiyaz Mohammed 2021-05-06 @5891  		return ERR_PTR(-EOPNOTSUPP);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5892  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5893  	if (*ppos == 0) {
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5894  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5895  		t.count = 0;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5896  		t.max = 0;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5897  		t.loc = NULL;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5898  		if (strcmp(seq->file->f_path.dentry->d_name.name, "alloc_traces") == 0)
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5899  			alloc =  TRACK_ALLOC;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5900  		else
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5901  			alloc =  TRACK_FREE;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5902  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5903  		if (!alloc_loc_track(&t, PAGE_SIZE / sizeof(struct location),
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5904  			     GFP_KERNEL)) {
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5905  			seq_puts(seq, "Out of memory\n");
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5906  			return ERR_PTR(-ENOMEM);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5907  		}
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5908  		/* Push back cpu slabs */
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5909  		flush_all(s);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5910  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5911  		for_each_kmem_cache_node(s, node, n) {
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5912  			unsigned long flags;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5913  			struct page *page;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5914  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5915  			if (!atomic_long_read(&n->nr_slabs))
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5916  				continue;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5917  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5918  			spin_lock_irqsave(&n->list_lock, flags);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5919  			list_for_each_entry(page, &n->partial, slab_list)
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5920  				process_slab(&t, s, page, alloc);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5921  			list_for_each_entry(page, &n->full, slab_list)
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5922  				process_slab(&t, s, page, alloc);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5923  			spin_unlock_irqrestore(&n->list_lock, flags);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5924  		}
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5925  	}
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5926  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5927  	if (*ppos < t.count) {
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5928  		*spos = *ppos;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5929  		return spos;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5930  	}
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5931  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5932  	kfree(spos);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5933  	return NULL;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5934  }
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5935  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 35377 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] mm: slub: move sysfs slab alloc/free interfaces to debugfs
@ 2021-05-07  5:21 ` Dan Carpenter
  0 siblings, 0 replies; 13+ messages in thread
From: Dan Carpenter @ 2021-05-07  5:21 UTC (permalink / raw)
  To: kbuild, Faiyaz Mohammed, cl, penberg, rientjes, iamjoonsoo.kim,
	akpm, vbabka, linux-mm, linux-kernel, glittao
  Cc: lkp, kbuild-all, vinmenon

Hi Faiyaz,

url:    https://github.com/0day-ci/linux/commits/Faiyaz-Mohammed/mm-slub-move-sysfs-slab-alloc-free-interfaces-to-debugfs/20210506-182420
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 8404c9fbc84b741f66cff7d4934a25dd2c344452
config: i386-randconfig-m021-20210506 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
mm/slub.c:5891 slab_debugfs_start() warn: possible memory leak of 'spos'

vim +/spos +5891 mm/slub.c

88e180b99466c1 Faiyaz Mohammed 2021-05-06  5877  static void *slab_debugfs_start(struct seq_file *seq, loff_t *ppos)
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5878  {
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5879  	struct kmem_cache_node *n;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5880  	struct kmem_cache *s;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5881  	enum track_item alloc;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5882  	int node;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5883  	loff_t *spos = kmalloc(sizeof(loff_t), GFP_KERNEL);
                                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Generally avoid putting functions which can fail in the declaration
block.  Allocations inside the declaration block are a tiny percent of
declarations over all but they are an outsize source of static checker
bugs.

88e180b99466c1 Faiyaz Mohammed 2021-05-06  5884  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5885  	s = seq->file->f_inode->i_private;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5886  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5887  	if (!spos)
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5888  		return NULL;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5889  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5890  	if (!(s->flags & SLAB_STORE_USER))
88e180b99466c1 Faiyaz Mohammed 2021-05-06 @5891  		return ERR_PTR(-EOPNOTSUPP);

"spos" is leaked.

88e180b99466c1 Faiyaz Mohammed 2021-05-06  5892  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5893  	if (*ppos == 0) {
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5894  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5895  		t.count = 0;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5896  		t.max = 0;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5897  		t.loc = NULL;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5898  		if (strcmp(seq->file->f_path.dentry->d_name.name, "alloc_traces") == 0)
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5899  			alloc =  TRACK_ALLOC;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5900  		else
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5901  			alloc =  TRACK_FREE;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5902  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5903  		if (!alloc_loc_track(&t, PAGE_SIZE / sizeof(struct location),
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5904  			     GFP_KERNEL)) {
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5905  			seq_puts(seq, "Out of memory\n");
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5906  			return ERR_PTR(-ENOMEM);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5907  		}
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5908  		/* Push back cpu slabs */
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5909  		flush_all(s);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5910  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5911  		for_each_kmem_cache_node(s, node, n) {
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5912  			unsigned long flags;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5913  			struct page *page;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5914  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5915  			if (!atomic_long_read(&n->nr_slabs))
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5916  				continue;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5917  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5918  			spin_lock_irqsave(&n->list_lock, flags);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5919  			list_for_each_entry(page, &n->partial, slab_list)
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5920  				process_slab(&t, s, page, alloc);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5921  			list_for_each_entry(page, &n->full, slab_list)
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5922  				process_slab(&t, s, page, alloc);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5923  			spin_unlock_irqrestore(&n->list_lock, flags);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5924  		}
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5925  	}
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5926  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5927  	if (*ppos < t.count) {
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5928  		*spos = *ppos;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5929  		return spos;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5930  	}
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5931  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5932  	kfree(spos);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5933  	return NULL;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5934  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] mm: slub: move sysfs slab alloc/free interfaces to debugfs
@ 2021-05-07  5:21 ` Dan Carpenter
  0 siblings, 0 replies; 13+ messages in thread
From: Dan Carpenter @ 2021-05-07  5:21 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 5290 bytes --]

Hi Faiyaz,

url:    https://github.com/0day-ci/linux/commits/Faiyaz-Mohammed/mm-slub-move-sysfs-slab-alloc-free-interfaces-to-debugfs/20210506-182420
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 8404c9fbc84b741f66cff7d4934a25dd2c344452
config: i386-randconfig-m021-20210506 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
mm/slub.c:5891 slab_debugfs_start() warn: possible memory leak of 'spos'

vim +/spos +5891 mm/slub.c

88e180b99466c1 Faiyaz Mohammed 2021-05-06  5877  static void *slab_debugfs_start(struct seq_file *seq, loff_t *ppos)
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5878  {
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5879  	struct kmem_cache_node *n;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5880  	struct kmem_cache *s;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5881  	enum track_item alloc;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5882  	int node;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5883  	loff_t *spos = kmalloc(sizeof(loff_t), GFP_KERNEL);
                                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Generally avoid putting functions which can fail in the declaration
block.  Allocations inside the declaration block are a tiny percent of
declarations over all but they are an outsize source of static checker
bugs.

88e180b99466c1 Faiyaz Mohammed 2021-05-06  5884  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5885  	s = seq->file->f_inode->i_private;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5886  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5887  	if (!spos)
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5888  		return NULL;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5889  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5890  	if (!(s->flags & SLAB_STORE_USER))
88e180b99466c1 Faiyaz Mohammed 2021-05-06 @5891  		return ERR_PTR(-EOPNOTSUPP);

"spos" is leaked.

88e180b99466c1 Faiyaz Mohammed 2021-05-06  5892  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5893  	if (*ppos == 0) {
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5894  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5895  		t.count = 0;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5896  		t.max = 0;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5897  		t.loc = NULL;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5898  		if (strcmp(seq->file->f_path.dentry->d_name.name, "alloc_traces") == 0)
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5899  			alloc =  TRACK_ALLOC;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5900  		else
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5901  			alloc =  TRACK_FREE;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5902  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5903  		if (!alloc_loc_track(&t, PAGE_SIZE / sizeof(struct location),
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5904  			     GFP_KERNEL)) {
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5905  			seq_puts(seq, "Out of memory\n");
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5906  			return ERR_PTR(-ENOMEM);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5907  		}
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5908  		/* Push back cpu slabs */
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5909  		flush_all(s);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5910  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5911  		for_each_kmem_cache_node(s, node, n) {
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5912  			unsigned long flags;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5913  			struct page *page;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5914  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5915  			if (!atomic_long_read(&n->nr_slabs))
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5916  				continue;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5917  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5918  			spin_lock_irqsave(&n->list_lock, flags);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5919  			list_for_each_entry(page, &n->partial, slab_list)
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5920  				process_slab(&t, s, page, alloc);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5921  			list_for_each_entry(page, &n->full, slab_list)
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5922  				process_slab(&t, s, page, alloc);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5923  			spin_unlock_irqrestore(&n->list_lock, flags);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5924  		}
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5925  	}
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5926  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5927  	if (*ppos < t.count) {
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5928  		*spos = *ppos;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5929  		return spos;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5930  	}
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5931  
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5932  	kfree(spos);
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5933  	return NULL;
88e180b99466c1 Faiyaz Mohammed 2021-05-06  5934  }

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] mm: slub: move sysfs slab alloc/free interfaces to debugfs
  2021-05-06 10:22 [PATCH v5] mm: slub: move sysfs slab alloc/free interfaces to debugfs Faiyaz Mohammed
  2021-05-06 14:29   ` kernel test robot
  2021-05-06 14:32   ` kernel test robot
@ 2021-05-07 11:48 ` Greg KH
  2021-05-18 12:54   ` Faiyaz Mohammed
  2 siblings, 1 reply; 13+ messages in thread
From: Greg KH @ 2021-05-07 11:48 UTC (permalink / raw)
  To: Faiyaz Mohammed
  Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, linux-mm,
	linux-kernel, glittao, vinmenon

On Thu, May 06, 2021 at 03:52:03PM +0530, Faiyaz Mohammed wrote:
> alloc_calls and free_calls implementation in sysfs have two issues,
> one is PAGE_SIZE limitiation of sysfs and other is it does not adhere
> to "one value per file" rule.
> 
> To overcome this issues, move the alloc_calls and free_calls implemeation
> to debugfs.
> 
> Rename the alloc_calls/free_calls to alloc_traces/free_traces,
> to be inline with what it does.
> 
> Signed-off-by: Faiyaz Mohammed <faiyazm@codeaurora.org>
> ---
>  include/linux/slub_def.h |  10 ++
>  mm/slab_common.c         |   9 ++
>  mm/slub.c                | 362 ++++++++++++++++++++++++++++++++++++-----------
>  3 files changed, 299 insertions(+), 82 deletions(-)
> 
> diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
> index dcde82a..f8c268d 100644
> --- a/include/linux/slub_def.h
> +++ b/include/linux/slub_def.h
> @@ -110,6 +110,9 @@ struct kmem_cache {
>  #ifdef CONFIG_SYSFS
>  	struct kobject kobj;	/* For sysfs */
>  #endif
> +#ifdef CONFIG_SLUB_DEBUG
> +	struct dentry *slab_cache_dentry;
> +#endif
>  #ifdef CONFIG_SLAB_FREELIST_HARDENED
>  	unsigned long random;
>  #endif
> @@ -159,6 +162,13 @@ static inline void sysfs_slab_release(struct kmem_cache *s)
>  }
>  #endif
>  
> +#ifdef CONFIG_DEBUG_FS
> +void debugfs_slab_release(struct kmem_cache *);
> +#else
> +static inline void debugfs_slab_release(struct kmem_cache *s)
> +{
> +}
> +#endif
>  void object_err(struct kmem_cache *s, struct page *page,
>  		u8 *object, char *reason);
>  
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index f8833d3..f3afe6b 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -445,6 +445,9 @@ static void slab_caches_to_rcu_destroy_workfn(struct work_struct *work)
>  #else
>  		slab_kmem_cache_release(s);
>  #endif
> +#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_SLUB)
> +		debugfs_slab_release(s);
> +#endif

If you write your .h files correctly, no need for #ifdef in a .c file.

Please fix up.

>  	}
>  }
>  
> @@ -462,6 +465,9 @@ static int shutdown_cache(struct kmem_cache *s)
>  #ifdef SLAB_SUPPORTS_SYSFS
>  		sysfs_slab_unlink(s);
>  #endif
> +#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_SLUB)
> +		debugfs_slab_release(s);
> +#endif
>  		list_add_tail(&s->list, &slab_caches_to_rcu_destroy);
>  		schedule_work(&slab_caches_to_rcu_destroy_work);
>  	} else {
> @@ -472,6 +478,9 @@ static int shutdown_cache(struct kmem_cache *s)
>  #else
>  		slab_kmem_cache_release(s);
>  #endif
> +#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_SLUB)
> +		debugfs_slab_release(s);
> +#endif
>  	}
>  
>  	return 0;
> diff --git a/mm/slub.c b/mm/slub.c
> index 68123b2..a5347f1 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -36,6 +36,7 @@
>  #include <linux/memcontrol.h>
>  #include <linux/random.h>
>  
> +#include <linux/debugfs.h>
>  #include <trace/events/kmem.h>
>  
>  #include "internal.h"
> @@ -225,6 +226,15 @@ static inline int sysfs_slab_alias(struct kmem_cache *s, const char *p)
>  							{ return 0; }
>  #endif
>  
> +#ifdef CONFIG_DEBUG_FS
> +static void debugfs_slab_add(struct kmem_cache *);
> +static int debugfs_slab_alias(struct kmem_cache *, const char *);
> +#else
> +static inline void debugfs_slab_add(struct kmem_cache *s) { }
> +static inline int debugfs_slab_alias(struct kmem_cache *s, const char *p)
> +							{ return 0; }
> +#endif
> +
>  static inline void stat(const struct kmem_cache *s, enum stat_item si)
>  {
>  #ifdef CONFIG_SLUB_STATS
> @@ -4533,6 +4543,8 @@ __kmem_cache_alias(const char *name, unsigned int size, unsigned int align,
>  			s->refcount--;
>  			s = NULL;
>  		}
> +
> +		debugfs_slab_alias(s, name);
>  	}
>  
>  	return s;
> @@ -4554,6 +4566,8 @@ int __kmem_cache_create(struct kmem_cache *s, slab_flags_t flags)
>  	if (err)
>  		__kmem_cache_release(s);
>  
> +	debugfs_slab_add(s);
> +
>  	return err;
>  }
>  
> @@ -4694,6 +4708,8 @@ static long validate_slab_cache(struct kmem_cache *s)
>  
>  	return count;
>  }
> +
> +#ifdef CONFIG_DEBUG_FS
>  /*
>   * Generate lists of code addresses where slabcache objects are allocated
>   * and freed.
> @@ -4717,6 +4733,9 @@ struct loc_track {
>  	struct location *loc;
>  };
>  
> +static struct dentry *slab_debugfs_root;
> +struct loc_track t = { 0, 0, NULL };
> +
>  static void free_loc_track(struct loc_track *t)
>  {
>  	if (t->max)
> @@ -4833,82 +4852,7 @@ static void process_slab(struct loc_track *t, struct kmem_cache *s,
>  			add_location(t, s, get_track(s, p, alloc));
>  	put_map(map);
>  }
> -
> -static int list_locations(struct kmem_cache *s, char *buf,
> -			  enum track_item alloc)
> -{
> -	int len = 0;
> -	unsigned long i;
> -	struct loc_track t = { 0, 0, NULL };
> -	int node;
> -	struct kmem_cache_node *n;
> -
> -	if (!alloc_loc_track(&t, PAGE_SIZE / sizeof(struct location),
> -			     GFP_KERNEL)) {
> -		return sysfs_emit(buf, "Out of memory\n");
> -	}
> -	/* Push back cpu slabs */
> -	flush_all(s);
> -
> -	for_each_kmem_cache_node(s, node, n) {
> -		unsigned long flags;
> -		struct page *page;
> -
> -		if (!atomic_long_read(&n->nr_slabs))
> -			continue;
> -
> -		spin_lock_irqsave(&n->list_lock, flags);
> -		list_for_each_entry(page, &n->partial, slab_list)
> -			process_slab(&t, s, page, alloc);
> -		list_for_each_entry(page, &n->full, slab_list)
> -			process_slab(&t, s, page, alloc);
> -		spin_unlock_irqrestore(&n->list_lock, flags);
> -	}
> -
> -	for (i = 0; i < t.count; i++) {
> -		struct location *l = &t.loc[i];
> -
> -		len += sysfs_emit_at(buf, len, "%7ld ", l->count);
> -
> -		if (l->addr)
> -			len += sysfs_emit_at(buf, len, "%pS", (void *)l->addr);
> -		else
> -			len += sysfs_emit_at(buf, len, "<not-available>");
> -
> -		if (l->sum_time != l->min_time)
> -			len += sysfs_emit_at(buf, len, " age=%ld/%ld/%ld",
> -					     l->min_time,
> -					     (long)div_u64(l->sum_time,
> -							   l->count),
> -					     l->max_time);
> -		else
> -			len += sysfs_emit_at(buf, len, " age=%ld", l->min_time);
> -
> -		if (l->min_pid != l->max_pid)
> -			len += sysfs_emit_at(buf, len, " pid=%ld-%ld",
> -					     l->min_pid, l->max_pid);
> -		else
> -			len += sysfs_emit_at(buf, len, " pid=%ld",
> -					     l->min_pid);
> -
> -		if (num_online_cpus() > 1 &&
> -		    !cpumask_empty(to_cpumask(l->cpus)))
> -			len += sysfs_emit_at(buf, len, " cpus=%*pbl",
> -					     cpumask_pr_args(to_cpumask(l->cpus)));
> -
> -		if (nr_online_nodes > 1 && !nodes_empty(l->nodes))
> -			len += sysfs_emit_at(buf, len, " nodes=%*pbl",
> -					     nodemask_pr_args(&l->nodes));
> -
> -		len += sysfs_emit_at(buf, len, "\n");
> -	}
> -
> -	free_loc_track(&t);
> -	if (!t.count)
> -		len += sysfs_emit_at(buf, len, "No data\n");
> -
> -	return len;
> -}
> +#endif  /* CONFIG_DEBUG_FS   */
>  #endif	/* CONFIG_SLUB_DEBUG */
>  
>  #ifdef SLUB_RESILIENCY_TEST
> @@ -5360,17 +5304,25 @@ SLAB_ATTR(validate);
>  
>  static ssize_t alloc_calls_show(struct kmem_cache *s, char *buf)
>  {
> -	if (!(s->flags & SLAB_STORE_USER))
> -		return -ENOSYS;
> -	return list_locations(s, buf, TRACK_ALLOC);
> +	int len = 0;
> +
> +	len += sprintf(buf + len, "Deprecated, use the equvalent under\n");
> +	len += sprintf(buf + len, "/sys/kernel/debug/slab/%s/alloc_traces\n",
> +			s->name);
> +
> +	return len;
>  }
>  SLAB_ATTR_RO(alloc_calls);
>  
>  static ssize_t free_calls_show(struct kmem_cache *s, char *buf)
>  {
> -	if (!(s->flags & SLAB_STORE_USER))
> -		return -ENOSYS;
> -	return list_locations(s, buf, TRACK_FREE);
> +	int len = 0;
> +
> +	len += sprintf(buf + len, "Deprecated, use the equvalent under\n");
> +	len += sprintf(buf + len, "/sys/kernel/debug/slab/%s/free_traces\n",
> +			s->name);
> +
> +	return len;
>  }
>  SLAB_ATTR_RO(free_calls);
>  #endif /* CONFIG_SLUB_DEBUG */
> @@ -5826,6 +5778,252 @@ static int __init slab_sysfs_init(void)
>  __initcall(slab_sysfs_init);
>  #endif /* CONFIG_SYSFS */
>  
> +#if defined(CONFIG_SLUB_DEBUG) && defined(CONFIG_DEBUG_FS)
> +static int debugfs_slab_alias(struct kmem_cache *s, const char *name)
> +{
> +	struct saved_alias *al;
> +
> +	if (slab_state == FULL) {
> +		/*
> +		 * If we have a leftover link then remove it.
> +		 */
> +		debugfs_remove(s->slab_cache_dentry);
> +		s->slab_cache_dentry = debugfs_create_symlink(name, slab_debugfs_root, NULL);
> +		return IS_ERR(s->slab_cache_dentry);

Why do you care if this returned an error or not?  No normal code should
care, please just keep on going.

And are you sure you have to save the dentry?  Why not just look it up
when you want to remove it?  That way you save the memory, which for
slabs, can be a lot, right?



> +	}
> +
> +	al = kmalloc(sizeof(struct saved_alias), GFP_KERNEL);
> +	if (!al)
> +		return -ENOMEM;
> +
> +	al->s = s;
> +	al->name = name;
> +	al->next = alias_list;
> +	alias_list = al;
> +	return 0;
> +}
> +
> +static int slab_debugfs_show(struct seq_file *seq, void *v)
> +{
> +
> +	struct location *l;
> +	unsigned int idx = *(unsigned int *)v;
> +
> +	if (idx < t.count) {
> +		l = &t.loc[idx];
> +
> +		seq_printf(seq, "%7ld ", l->count);
> +
> +		if (l->addr)
> +			seq_printf(seq, "%pS", (void *)l->addr);
> +		else
> +			seq_puts(seq, "<not-available>");
> +
> +		if (l->sum_time != l->min_time) {
> +			seq_printf(seq, " age=%ld/%ld/%ld",
> +				l->min_time,
> +				(long)div_u64(l->sum_time, l->count),
> +				l->max_time);
> +		} else
> +			seq_printf(seq, " age=%ld",
> +				l->min_time);
> +
> +		if (l->min_pid != l->max_pid)
> +			seq_printf(seq, " pid=%ld-%ld",
> +				l->min_pid, l->max_pid);
> +		else
> +			seq_printf(seq, " pid=%ld",
> +				l->min_pid);
> +
> +		if (num_online_cpus() > 1 &&
> +				!cpumask_empty(to_cpumask(l->cpus)))
> +			seq_printf(seq, " cpus=%*pbl",
> +				 cpumask_pr_args(to_cpumask(l->cpus)));
> +
> +		if (nr_online_nodes > 1 && !nodes_empty(l->nodes))
> +			seq_printf(seq, " nodes=%*pbl",
> +				 nodemask_pr_args(&l->nodes));
> +
> +		seq_puts(seq, "\n");
> +	}
> +
> +	if (t.count == 0)
> +		seq_puts(seq, "No data\n");
> +
> +	return 0;
> +}
> +
> +static void slab_debugfs_stop(struct seq_file *seq, void *v)
> +{
> +	if (!v && t.max) {
> +		free_loc_track(&t);
> +		t.max = 0;
> +	}
> +}
> +
> +static void *slab_debugfs_next(struct seq_file *seq, void *v, loff_t *ppos)
> +{
> +	loff_t *spos = v;
> +
> +	if (*ppos < t.count) {
> +		*spos = *spos + 1;
> +		*ppos = *spos;
> +		return spos;
> +	}
> +
> +	return NULL;
> +}
> +
> +static void *slab_debugfs_start(struct seq_file *seq, loff_t *ppos)
> +{
> +	struct kmem_cache_node *n;
> +	struct kmem_cache *s;
> +	enum track_item alloc;
> +	int node;
> +	loff_t *spos = kmalloc(sizeof(loff_t), GFP_KERNEL);
> +
> +	s = seq->file->f_inode->i_private;
> +
> +	if (!spos)
> +		return NULL;
> +
> +	if (!(s->flags & SLAB_STORE_USER))
> +		return ERR_PTR(-EOPNOTSUPP);
> +
> +	if (*ppos == 0) {
> +
> +		t.count = 0;
> +		t.max = 0;
> +		t.loc = NULL;
> +		if (strcmp(seq->file->f_path.dentry->d_name.name, "alloc_traces") == 0)
> +			alloc =  TRACK_ALLOC;
> +		else
> +			alloc =  TRACK_FREE;
> +
> +		if (!alloc_loc_track(&t, PAGE_SIZE / sizeof(struct location),
> +			     GFP_KERNEL)) {
> +			seq_puts(seq, "Out of memory\n");
> +			return ERR_PTR(-ENOMEM);
> +		}
> +		/* Push back cpu slabs */
> +		flush_all(s);
> +
> +		for_each_kmem_cache_node(s, node, n) {
> +			unsigned long flags;
> +			struct page *page;
> +
> +			if (!atomic_long_read(&n->nr_slabs))
> +				continue;
> +
> +			spin_lock_irqsave(&n->list_lock, flags);
> +			list_for_each_entry(page, &n->partial, slab_list)
> +				process_slab(&t, s, page, alloc);
> +			list_for_each_entry(page, &n->full, slab_list)
> +				process_slab(&t, s, page, alloc);
> +			spin_unlock_irqrestore(&n->list_lock, flags);
> +		}
> +	}
> +
> +	if (*ppos < t.count) {
> +		*spos = *ppos;
> +		return spos;
> +	}
> +
> +	kfree(spos);
> +	return NULL;
> +}
> +
> +static const struct seq_operations slab_debugfs_sops = {
> +	.start  = slab_debugfs_start,
> +	.next   = slab_debugfs_next,
> +	.stop   = slab_debugfs_stop,
> +	.show   = slab_debugfs_show
> +};
> +DEFINE_SEQ_ATTRIBUTE(slab_debugfs);
> +
> +static void debugfs_slab_add(struct kmem_cache *s)
> +{
> +	const char *name;
> +	int unmergeable = slab_unmergeable(s);
> +
> +	if (unlikely(!slab_debugfs_root))
> +		return;
> +
> +	if (!unmergeable && disable_higher_order_debug &&
> +			(slub_debug & DEBUG_METADATA_FLAGS))
> +		unmergeable = 1;
> +
> +	if (unmergeable) {
> +		/*
> +		 * Slabcache can never be merged so we can use the name proper.
> +		 * This is typically the case for debug situations. In that
> +		 * case we can catch duplicate names easily.
> +		 */
> +		debugfs_remove_recursive(s->slab_cache_dentry);
> +		name = s->name;
> +	} else {
> +		/*
> +		 * Create a unique name for the slab as a target
> +		 * for the symlinks.
> +		 */
> +		name = create_unique_id(s);
> +	}
> +
> +	s->slab_cache_dentry = debugfs_create_dir(name, slab_debugfs_root);
> +	if (!IS_ERR(s->slab_cache_dentry)) {

No need for you to care here either, just call debugfs and keep on
going.

> +		debugfs_create_file("alloc_traces", 0400,
> +			s->slab_cache_dentry, s, &slab_debugfs_fops);
> +
> +		debugfs_create_file("free_traces", 0400,
> +			s->slab_cache_dentry, s, &slab_debugfs_fops);
> +	}
> +
> +	if (!unmergeable) {
> +		/* Setup first alias */
> +		debugfs_slab_alias(s, s->name);
> +	}

Do you really need the { }?

> +}
> +
> +void debugfs_slab_release(struct kmem_cache *s)
> +{
> +	if (slab_state >= FULL)
> +		debugfs_remove_recursive(s->slab_cache_dentry);

Why does the state matter?

And again, can't you just look up the debugfs dentry here?

> +}
> +
> +static int __init slab_debugfs_init(void)
> +{
> +	struct kmem_cache *s;
> +	int err;
> +
> +	slab_debugfs_root = debugfs_create_dir("slab", NULL);
> +	if (!IS_ERR(slab_debugfs_root)) {
> +
> +		slab_state = FULL;

Again, no need to check.

> +
> +		list_for_each_entry(s, &slab_caches, list)
> +			debugfs_slab_add(s);
> +	} else {
> +		pr_err("Cannot create slab debugfs.\n");
> +		return IS_ERR(slab_debugfs_root);
> +	}
> +
> +	while (alias_list) {
> +		struct saved_alias *al = alias_list;
> +
> +		alias_list = alias_list->next;
> +
> +		err = debugfs_slab_alias(al->s, al->name);
> +		if (err)
> +			pr_err("SLUB: Unable to add boot slab alias %s to debugfs\n",
> +			       al->name);

Why does this matter?

> +		kfree(al);
> +	}
> +
> +	return 0;

See, you always return 0, no need to check the above stuff :)

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] mm: slub: move sysfs slab alloc/free interfaces to debugfs
  2021-05-07  5:21 ` Dan Carpenter
@ 2021-05-18 12:49   ` Faiyaz Mohammed
  -1 siblings, 0 replies; 13+ messages in thread
From: Faiyaz Mohammed @ 2021-05-18 12:49 UTC (permalink / raw)
  To: Dan Carpenter, kbuild, cl, penberg, rientjes, iamjoonsoo.kim,
	akpm, vbabka, linux-mm, linux-kernel, glittao
  Cc: lkp, kbuild-all, vinmenon

Hi,

Updated in the patch,
https://lore.kernel.org/linux-mm/1621341949-26762-1-git-send-email-faiyazm@codeaurora.org/T/#u

Thanks and regards,
Mohammed Faiyaz

On 5/7/2021 10:51 AM, Dan Carpenter wrote:
> Hi Faiyaz,
> 
> url:    https://github.com/0day-ci/linux/commits/Faiyaz-Mohammed/mm-slub-move-sysfs-slab-alloc-free-interfaces-to-debugfs/20210506-182420
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 8404c9fbc84b741f66cff7d4934a25dd2c344452
> config: i386-randconfig-m021-20210506 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> smatch warnings:
> mm/slub.c:5891 slab_debugfs_start() warn: possible memory leak of 'spos'
> 
> vim +/spos +5891 mm/slub.c
> 
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5877  static void *slab_debugfs_start(struct seq_file *seq, loff_t *ppos)
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5878  {
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5879  	struct kmem_cache_node *n;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5880  	struct kmem_cache *s;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5881  	enum track_item alloc;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5882  	int node;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5883  	loff_t *spos = kmalloc(sizeof(loff_t), GFP_KERNEL);
>                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Generally avoid putting functions which can fail in the declaration
> block.  Allocations inside the declaration block are a tiny percent of
> declarations over all but they are an outsize source of static checker
> bugs.
> 
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5884  
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5885  	s = seq->file->f_inode->i_private;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5886  
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5887  	if (!spos)
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5888  		return NULL;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5889  
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5890  	if (!(s->flags & SLAB_STORE_USER))
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06 @5891  		return ERR_PTR(-EOPNOTSUPP);
> 
> "spos" is leaked.
> 
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5892  
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5893  	if (*ppos == 0) {
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5894  
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5895  		t.count = 0;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5896  		t.max = 0;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5897  		t.loc = NULL;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5898  		if (strcmp(seq->file->f_path.dentry->d_name.name, "alloc_traces") == 0)
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5899  			alloc =  TRACK_ALLOC;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5900  		else
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5901  			alloc =  TRACK_FREE;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5902  
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5903  		if (!alloc_loc_track(&t, PAGE_SIZE / sizeof(struct location),
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5904  			     GFP_KERNEL)) {
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5905  			seq_puts(seq, "Out of memory\n");
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5906  			return ERR_PTR(-ENOMEM);
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5907  		}
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5908  		/* Push back cpu slabs */
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5909  		flush_all(s);
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5910  
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5911  		for_each_kmem_cache_node(s, node, n) {
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5912  			unsigned long flags;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5913  			struct page *page;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5914  
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5915  			if (!atomic_long_read(&n->nr_slabs))
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5916  				continue;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5917  
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5918  			spin_lock_irqsave(&n->list_lock, flags);
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5919  			list_for_each_entry(page, &n->partial, slab_list)
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5920  				process_slab(&t, s, page, alloc);
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5921  			list_for_each_entry(page, &n->full, slab_list)
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5922  				process_slab(&t, s, page, alloc);
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5923  			spin_unlock_irqrestore(&n->list_lock, flags);
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5924  		}
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5925  	}
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5926  
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5927  	if (*ppos < t.count) {
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5928  		*spos = *ppos;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5929  		return spos;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5930  	}
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5931  
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5932  	kfree(spos);
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5933  	return NULL;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5934  }
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] mm: slub: move sysfs slab alloc/free interfaces to debugfs
@ 2021-05-18 12:49   ` Faiyaz Mohammed
  0 siblings, 0 replies; 13+ messages in thread
From: Faiyaz Mohammed @ 2021-05-18 12:49 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 5682 bytes --]

Hi,

Updated in the patch,
https://lore.kernel.org/linux-mm/1621341949-26762-1-git-send-email-faiyazm(a)codeaurora.org/T/#u

Thanks and regards,
Mohammed Faiyaz

On 5/7/2021 10:51 AM, Dan Carpenter wrote:
> Hi Faiyaz,
> 
> url:    https://github.com/0day-ci/linux/commits/Faiyaz-Mohammed/mm-slub-move-sysfs-slab-alloc-free-interfaces-to-debugfs/20210506-182420
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 8404c9fbc84b741f66cff7d4934a25dd2c344452
> config: i386-randconfig-m021-20210506 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> 
> smatch warnings:
> mm/slub.c:5891 slab_debugfs_start() warn: possible memory leak of 'spos'
> 
> vim +/spos +5891 mm/slub.c
> 
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5877  static void *slab_debugfs_start(struct seq_file *seq, loff_t *ppos)
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5878  {
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5879  	struct kmem_cache_node *n;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5880  	struct kmem_cache *s;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5881  	enum track_item alloc;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5882  	int node;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5883  	loff_t *spos = kmalloc(sizeof(loff_t), GFP_KERNEL);
>                                                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Generally avoid putting functions which can fail in the declaration
> block.  Allocations inside the declaration block are a tiny percent of
> declarations over all but they are an outsize source of static checker
> bugs.
> 
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5884  
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5885  	s = seq->file->f_inode->i_private;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5886  
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5887  	if (!spos)
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5888  		return NULL;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5889  
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5890  	if (!(s->flags & SLAB_STORE_USER))
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06 @5891  		return ERR_PTR(-EOPNOTSUPP);
> 
> "spos" is leaked.
> 
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5892  
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5893  	if (*ppos == 0) {
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5894  
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5895  		t.count = 0;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5896  		t.max = 0;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5897  		t.loc = NULL;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5898  		if (strcmp(seq->file->f_path.dentry->d_name.name, "alloc_traces") == 0)
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5899  			alloc =  TRACK_ALLOC;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5900  		else
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5901  			alloc =  TRACK_FREE;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5902  
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5903  		if (!alloc_loc_track(&t, PAGE_SIZE / sizeof(struct location),
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5904  			     GFP_KERNEL)) {
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5905  			seq_puts(seq, "Out of memory\n");
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5906  			return ERR_PTR(-ENOMEM);
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5907  		}
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5908  		/* Push back cpu slabs */
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5909  		flush_all(s);
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5910  
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5911  		for_each_kmem_cache_node(s, node, n) {
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5912  			unsigned long flags;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5913  			struct page *page;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5914  
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5915  			if (!atomic_long_read(&n->nr_slabs))
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5916  				continue;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5917  
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5918  			spin_lock_irqsave(&n->list_lock, flags);
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5919  			list_for_each_entry(page, &n->partial, slab_list)
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5920  				process_slab(&t, s, page, alloc);
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5921  			list_for_each_entry(page, &n->full, slab_list)
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5922  				process_slab(&t, s, page, alloc);
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5923  			spin_unlock_irqrestore(&n->list_lock, flags);
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5924  		}
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5925  	}
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5926  
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5927  	if (*ppos < t.count) {
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5928  		*spos = *ppos;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5929  		return spos;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5930  	}
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5931  
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5932  	kfree(spos);
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5933  	return NULL;
> 88e180b99466c1 Faiyaz Mohammed 2021-05-06  5934  }
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] mm: slub: move sysfs slab alloc/free interfaces to debugfs
  2021-05-07 11:48 ` Greg KH
@ 2021-05-18 12:54   ` Faiyaz Mohammed
  2021-05-18 13:33     ` Greg KH
  0 siblings, 1 reply; 13+ messages in thread
From: Faiyaz Mohammed @ 2021-05-18 12:54 UTC (permalink / raw)
  To: Greg KH
  Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, linux-mm,
	linux-kernel, glittao, vinmenon



On 5/7/2021 5:18 PM, Greg KH wrote:
> On Thu, May 06, 2021 at 03:52:03PM +0530, Faiyaz Mohammed wrote:
>> alloc_calls and free_calls implementation in sysfs have two issues,
>> one is PAGE_SIZE limitiation of sysfs and other is it does not adhere
>> to "one value per file" rule.
>>
>> To overcome this issues, move the alloc_calls and free_calls implemeation
>> to debugfs.
>>
>> Rename the alloc_calls/free_calls to alloc_traces/free_traces,
>> to be inline with what it does.
>>
>> Signed-off-by: Faiyaz Mohammed <faiyazm@codeaurora.org>
>> ---
>>  include/linux/slub_def.h |  10 ++
>>  mm/slab_common.c         |   9 ++
>>  mm/slub.c                | 362 ++++++++++++++++++++++++++++++++++++-----------
>>  3 files changed, 299 insertions(+), 82 deletions(-)
>>
>> diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
>> index dcde82a..f8c268d 100644
>> --- a/include/linux/slub_def.h
>> +++ b/include/linux/slub_def.h
>> @@ -110,6 +110,9 @@ struct kmem_cache {
>>  #ifdef CONFIG_SYSFS
>>  	struct kobject kobj;	/* For sysfs */
>>  #endif
>> +#ifdef CONFIG_SLUB_DEBUG
>> +	struct dentry *slab_cache_dentry;
>> +#endif
>>  #ifdef CONFIG_SLAB_FREELIST_HARDENED
>>  	unsigned long random;
>>  #endif
>> @@ -159,6 +162,13 @@ static inline void sysfs_slab_release(struct kmem_cache *s)
>>  }
>>  #endif
>>  
>> +#ifdef CONFIG_DEBUG_FS
>> +void debugfs_slab_release(struct kmem_cache *);
>> +#else
>> +static inline void debugfs_slab_release(struct kmem_cache *s)
>> +{
>> +}
>> +#endif
>>  void object_err(struct kmem_cache *s, struct page *page,
>>  		u8 *object, char *reason);
>>  
>> diff --git a/mm/slab_common.c b/mm/slab_common.c
>> index f8833d3..f3afe6b 100644
>> --- a/mm/slab_common.c
>> +++ b/mm/slab_common.c
>> @@ -445,6 +445,9 @@ static void slab_caches_to_rcu_destroy_workfn(struct work_struct *work)
>>  #else
>>  		slab_kmem_cache_release(s);
>>  #endif
>> +#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_SLUB)
>> +		debugfs_slab_release(s);
>> +#endif
> 
> If you write your .h files correctly, no need for #ifdef in a .c file.
> 
> Please fix up.
> 
fixed in new patch and used the single #ifded because
debugfs_slab_release declaration is there in slub_def.h and slab_common
is used for both slab and slub.
Like SLAB_SUPPORTS_SYSFS, SLAB_SUPPORT_DEBUGFS will be not defined if
slab config is used.
>>  	}
>>  }
>>  
>> @@ -462,6 +465,9 @@ static int shutdown_cache(struct kmem_cache *s)
>>  #ifdef SLAB_SUPPORTS_SYSFS
>>  		sysfs_slab_unlink(s);
>>  #endif
>> +#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_SLUB)
>> +		debugfs_slab_release(s);
>> +#endif
>>  		list_add_tail(&s->list, &slab_caches_to_rcu_destroy);
>>  		schedule_work(&slab_caches_to_rcu_destroy_work);
>>  	} else {
>> @@ -472,6 +478,9 @@ static int shutdown_cache(struct kmem_cache *s)
>>  #else
>>  		slab_kmem_cache_release(s);
>>  #endif
>> +#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_SLUB)
>> +		debugfs_slab_release(s);
>> +#endif
>>  	}
>>  
>>  	return 0;
>> diff --git a/mm/slub.c b/mm/slub.c
>> index 68123b2..a5347f1 100644
>> --- a/mm/slub.c
>> +++ b/mm/slub.c
>> @@ -36,6 +36,7 @@
>>  #include <linux/memcontrol.h>
>>  #include <linux/random.h>
>>  
>> +#include <linux/debugfs.h>
>>  #include <trace/events/kmem.h>
>>  
>>  #include "internal.h"
>> @@ -225,6 +226,15 @@ static inline int sysfs_slab_alias(struct kmem_cache *s, const char *p)
>>  							{ return 0; }
>>  #endif
>>  
>> +#ifdef CONFIG_DEBUG_FS
>> +static void debugfs_slab_add(struct kmem_cache *);
>> +static int debugfs_slab_alias(struct kmem_cache *, const char *);
>> +#else
>> +static inline void debugfs_slab_add(struct kmem_cache *s) { }
>> +static inline int debugfs_slab_alias(struct kmem_cache *s, const char *p)
>> +							{ return 0; }
>> +#endif
>> +
>>  static inline void stat(const struct kmem_cache *s, enum stat_item si)
>>  {
>>  #ifdef CONFIG_SLUB_STATS
>> @@ -4533,6 +4543,8 @@ __kmem_cache_alias(const char *name, unsigned int size, unsigned int align,
>>  			s->refcount--;
>>  			s = NULL;
>>  		}
>> +
>> +		debugfs_slab_alias(s, name);
>>  	}
>>  
>>  	return s;
>> @@ -4554,6 +4566,8 @@ int __kmem_cache_create(struct kmem_cache *s, slab_flags_t flags)
>>  	if (err)
>>  		__kmem_cache_release(s);
>>  
>> +	debugfs_slab_add(s);
>> +
>>  	return err;
>>  }
>>  
>> @@ -4694,6 +4708,8 @@ static long validate_slab_cache(struct kmem_cache *s)
>>  
>>  	return count;
>>  }
>> +
>> +#ifdef CONFIG_DEBUG_FS
>>  /*
>>   * Generate lists of code addresses where slabcache objects are allocated
>>   * and freed.
>> @@ -4717,6 +4733,9 @@ struct loc_track {
>>  	struct location *loc;
>>  };
>>  
>> +static struct dentry *slab_debugfs_root;
>> +struct loc_track t = { 0, 0, NULL };
>> +
>>  static void free_loc_track(struct loc_track *t)
>>  {
>>  	if (t->max)
>> @@ -4833,82 +4852,7 @@ static void process_slab(struct loc_track *t, struct kmem_cache *s,
>>  			add_location(t, s, get_track(s, p, alloc));
>>  	put_map(map);
>>  }
>> -
>> -static int list_locations(struct kmem_cache *s, char *buf,
>> -			  enum track_item alloc)
>> -{
>> -	int len = 0;
>> -	unsigned long i;
>> -	struct loc_track t = { 0, 0, NULL };
>> -	int node;
>> -	struct kmem_cache_node *n;
>> -
>> -	if (!alloc_loc_track(&t, PAGE_SIZE / sizeof(struct location),
>> -			     GFP_KERNEL)) {
>> -		return sysfs_emit(buf, "Out of memory\n");
>> -	}
>> -	/* Push back cpu slabs */
>> -	flush_all(s);
>> -
>> -	for_each_kmem_cache_node(s, node, n) {
>> -		unsigned long flags;
>> -		struct page *page;
>> -
>> -		if (!atomic_long_read(&n->nr_slabs))
>> -			continue;
>> -
>> -		spin_lock_irqsave(&n->list_lock, flags);
>> -		list_for_each_entry(page, &n->partial, slab_list)
>> -			process_slab(&t, s, page, alloc);
>> -		list_for_each_entry(page, &n->full, slab_list)
>> -			process_slab(&t, s, page, alloc);
>> -		spin_unlock_irqrestore(&n->list_lock, flags);
>> -	}
>> -
>> -	for (i = 0; i < t.count; i++) {
>> -		struct location *l = &t.loc[i];
>> -
>> -		len += sysfs_emit_at(buf, len, "%7ld ", l->count);
>> -
>> -		if (l->addr)
>> -			len += sysfs_emit_at(buf, len, "%pS", (void *)l->addr);
>> -		else
>> -			len += sysfs_emit_at(buf, len, "<not-available>");
>> -
>> -		if (l->sum_time != l->min_time)
>> -			len += sysfs_emit_at(buf, len, " age=%ld/%ld/%ld",
>> -					     l->min_time,
>> -					     (long)div_u64(l->sum_time,
>> -							   l->count),
>> -					     l->max_time);
>> -		else
>> -			len += sysfs_emit_at(buf, len, " age=%ld", l->min_time);
>> -
>> -		if (l->min_pid != l->max_pid)
>> -			len += sysfs_emit_at(buf, len, " pid=%ld-%ld",
>> -					     l->min_pid, l->max_pid);
>> -		else
>> -			len += sysfs_emit_at(buf, len, " pid=%ld",
>> -					     l->min_pid);
>> -
>> -		if (num_online_cpus() > 1 &&
>> -		    !cpumask_empty(to_cpumask(l->cpus)))
>> -			len += sysfs_emit_at(buf, len, " cpus=%*pbl",
>> -					     cpumask_pr_args(to_cpumask(l->cpus)));
>> -
>> -		if (nr_online_nodes > 1 && !nodes_empty(l->nodes))
>> -			len += sysfs_emit_at(buf, len, " nodes=%*pbl",
>> -					     nodemask_pr_args(&l->nodes));
>> -
>> -		len += sysfs_emit_at(buf, len, "\n");
>> -	}
>> -
>> -	free_loc_track(&t);
>> -	if (!t.count)
>> -		len += sysfs_emit_at(buf, len, "No data\n");
>> -
>> -	return len;
>> -}
>> +#endif  /* CONFIG_DEBUG_FS   */
>>  #endif	/* CONFIG_SLUB_DEBUG */
>>  
>>  #ifdef SLUB_RESILIENCY_TEST
>> @@ -5360,17 +5304,25 @@ SLAB_ATTR(validate);
>>  
>>  static ssize_t alloc_calls_show(struct kmem_cache *s, char *buf)
>>  {
>> -	if (!(s->flags & SLAB_STORE_USER))
>> -		return -ENOSYS;
>> -	return list_locations(s, buf, TRACK_ALLOC);
>> +	int len = 0;
>> +
>> +	len += sprintf(buf + len, "Deprecated, use the equvalent under\n");
>> +	len += sprintf(buf + len, "/sys/kernel/debug/slab/%s/alloc_traces\n",
>> +			s->name);
>> +
>> +	return len;
>>  }
>>  SLAB_ATTR_RO(alloc_calls);
>>  
>>  static ssize_t free_calls_show(struct kmem_cache *s, char *buf)
>>  {
>> -	if (!(s->flags & SLAB_STORE_USER))
>> -		return -ENOSYS;
>> -	return list_locations(s, buf, TRACK_FREE);
>> +	int len = 0;
>> +
>> +	len += sprintf(buf + len, "Deprecated, use the equvalent under\n");
>> +	len += sprintf(buf + len, "/sys/kernel/debug/slab/%s/free_traces\n",
>> +			s->name);
>> +
>> +	return len;
>>  }
>>  SLAB_ATTR_RO(free_calls);
>>  #endif /* CONFIG_SLUB_DEBUG */
>> @@ -5826,6 +5778,252 @@ static int __init slab_sysfs_init(void)
>>  __initcall(slab_sysfs_init);
>>  #endif /* CONFIG_SYSFS */
>>  
>> +#if defined(CONFIG_SLUB_DEBUG) && defined(CONFIG_DEBUG_FS)
>> +static int debugfs_slab_alias(struct kmem_cache *s, const char *name)
>> +{
>> +	struct saved_alias *al;
>> +
>> +	if (slab_state == FULL) {
>> +		/*
>> +		 * If we have a leftover link then remove it.
>> +		 */
>> +		debugfs_remove(s->slab_cache_dentry);
>> +		s->slab_cache_dentry = debugfs_create_symlink(name, slab_debugfs_root, NULL);
>> +		return IS_ERR(s->slab_cache_dentry);
> 
> Why do you care if this returned an error or not?  No normal code should
> care, please just keep on going.
> 
> And are you sure you have to save the dentry?  Why not just look it up
> when you want to remove it?  That way you save the memory, which for
> slabs, can be a lot, right?
> 
> 
> 
Updated in new patch,
https://lore.kernel.org/linux-mm/1621341949-26762-1-git-send-email-faiyazm@codeaurora.org/T/#u
>> +	}
>> +
>> +	al = kmalloc(sizeof(struct saved_alias), GFP_KERNEL);
>> +	if (!al)
>> +		return -ENOMEM;
>> +
>> +	al->s = s;
>> +	al->name = name;
>> +	al->next = alias_list;
>> +	alias_list = al;
>> +	return 0;
>> +}
>> +
>> +static int slab_debugfs_show(struct seq_file *seq, void *v)
>> +{
>> +
>> +	struct location *l;
>> +	unsigned int idx = *(unsigned int *)v;
>> +
>> +	if (idx < t.count) {
>> +		l = &t.loc[idx];
>> +
>> +		seq_printf(seq, "%7ld ", l->count);
>> +
>> +		if (l->addr)
>> +			seq_printf(seq, "%pS", (void *)l->addr);
>> +		else
>> +			seq_puts(seq, "<not-available>");
>> +
>> +		if (l->sum_time != l->min_time) {
>> +			seq_printf(seq, " age=%ld/%ld/%ld",
>> +				l->min_time,
>> +				(long)div_u64(l->sum_time, l->count),
>> +				l->max_time);
>> +		} else
>> +			seq_printf(seq, " age=%ld",
>> +				l->min_time);
>> +
>> +		if (l->min_pid != l->max_pid)
>> +			seq_printf(seq, " pid=%ld-%ld",
>> +				l->min_pid, l->max_pid);
>> +		else
>> +			seq_printf(seq, " pid=%ld",
>> +				l->min_pid);
>> +
>> +		if (num_online_cpus() > 1 &&
>> +				!cpumask_empty(to_cpumask(l->cpus)))
>> +			seq_printf(seq, " cpus=%*pbl",
>> +				 cpumask_pr_args(to_cpumask(l->cpus)));
>> +
>> +		if (nr_online_nodes > 1 && !nodes_empty(l->nodes))
>> +			seq_printf(seq, " nodes=%*pbl",
>> +				 nodemask_pr_args(&l->nodes));
>> +
>> +		seq_puts(seq, "\n");
>> +	}
>> +
>> +	if (t.count == 0)
>> +		seq_puts(seq, "No data\n");
>> +
>> +	return 0;
>> +}
>> +
>> +static void slab_debugfs_stop(struct seq_file *seq, void *v)
>> +{
>> +	if (!v && t.max) {
>> +		free_loc_track(&t);
>> +		t.max = 0;
>> +	}
>> +}
>> +
>> +static void *slab_debugfs_next(struct seq_file *seq, void *v, loff_t *ppos)
>> +{
>> +	loff_t *spos = v;
>> +
>> +	if (*ppos < t.count) {
>> +		*spos = *spos + 1;
>> +		*ppos = *spos;
>> +		return spos;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +static void *slab_debugfs_start(struct seq_file *seq, loff_t *ppos)
>> +{
>> +	struct kmem_cache_node *n;
>> +	struct kmem_cache *s;
>> +	enum track_item alloc;
>> +	int node;
>> +	loff_t *spos = kmalloc(sizeof(loff_t), GFP_KERNEL);
>> +
>> +	s = seq->file->f_inode->i_private;
>> +
>> +	if (!spos)
>> +		return NULL;
>> +
>> +	if (!(s->flags & SLAB_STORE_USER))
>> +		return ERR_PTR(-EOPNOTSUPP);
>> +
>> +	if (*ppos == 0) {
>> +
>> +		t.count = 0;
>> +		t.max = 0;
>> +		t.loc = NULL;
>> +		if (strcmp(seq->file->f_path.dentry->d_name.name, "alloc_traces") == 0)
>> +			alloc =  TRACK_ALLOC;
>> +		else
>> +			alloc =  TRACK_FREE;
>> +
>> +		if (!alloc_loc_track(&t, PAGE_SIZE / sizeof(struct location),
>> +			     GFP_KERNEL)) {
>> +			seq_puts(seq, "Out of memory\n");
>> +			return ERR_PTR(-ENOMEM);
>> +		}
>> +		/* Push back cpu slabs */
>> +		flush_all(s);
>> +
>> +		for_each_kmem_cache_node(s, node, n) {
>> +			unsigned long flags;
>> +			struct page *page;
>> +
>> +			if (!atomic_long_read(&n->nr_slabs))
>> +				continue;
>> +
>> +			spin_lock_irqsave(&n->list_lock, flags);
>> +			list_for_each_entry(page, &n->partial, slab_list)
>> +				process_slab(&t, s, page, alloc);
>> +			list_for_each_entry(page, &n->full, slab_list)
>> +				process_slab(&t, s, page, alloc);
>> +			spin_unlock_irqrestore(&n->list_lock, flags);
>> +		}
>> +	}
>> +
>> +	if (*ppos < t.count) {
>> +		*spos = *ppos;
>> +		return spos;
>> +	}
>> +
>> +	kfree(spos);
>> +	return NULL;
>> +}
>> +
>> +static const struct seq_operations slab_debugfs_sops = {
>> +	.start  = slab_debugfs_start,
>> +	.next   = slab_debugfs_next,
>> +	.stop   = slab_debugfs_stop,
>> +	.show   = slab_debugfs_show
>> +};
>> +DEFINE_SEQ_ATTRIBUTE(slab_debugfs);
>> +
>> +static void debugfs_slab_add(struct kmem_cache *s)
>> +{
>> +	const char *name;
>> +	int unmergeable = slab_unmergeable(s);
>> +
>> +	if (unlikely(!slab_debugfs_root))
>> +		return;
>> +
>> +	if (!unmergeable && disable_higher_order_debug &&
>> +			(slub_debug & DEBUG_METADATA_FLAGS))
>> +		unmergeable = 1;
>> +
>> +	if (unmergeable) {
>> +		/*
>> +		 * Slabcache can never be merged so we can use the name proper.
>> +		 * This is typically the case for debug situations. In that
>> +		 * case we can catch duplicate names easily.
>> +		 */
>> +		debugfs_remove_recursive(s->slab_cache_dentry);
>> +		name = s->name;
>> +	} else {
>> +		/*
>> +		 * Create a unique name for the slab as a target
>> +		 * for the symlinks.
>> +		 */
>> +		name = create_unique_id(s);
>> +	}
>> +
>> +	s->slab_cache_dentry = debugfs_create_dir(name, slab_debugfs_root);
>> +	if (!IS_ERR(s->slab_cache_dentry)) {
> 
> No need for you to care here either, just call debugfs and keep on
> going.
> 
update in new patch,
https://lore.kernel.org/linux-mm/1621341949-26762-1-git-send-email-faiyazm@codeaurora.org/T/#u
>> +		debugfs_create_file("alloc_traces", 0400,
>> +			s->slab_cache_dentry, s, &slab_debugfs_fops);
>> +
>> +		debugfs_create_file("free_traces", 0400,
>> +			s->slab_cache_dentry, s, &slab_debugfs_fops);
>> +	}
>> +
>> +	if (!unmergeable) {
>> +		/* Setup first alias */
>> +		debugfs_slab_alias(s, s->name);
>> +	}
> 
> Do you really need the { }?
> 
>> +}
>> +
>> +void debugfs_slab_release(struct kmem_cache *s)
>> +{
>> +	if (slab_state >= FULL)
>> +		debugfs_remove_recursive(s->slab_cache_dentry);
> 
> Why does the state matter?
> 
> And again, can't you just look up the debugfs dentry here?
> 
>> +}
>> +
>> +static int __init slab_debugfs_init(void)
>> +{
>> +	struct kmem_cache *s;
>> +	int err;
>> +
>> +	slab_debugfs_root = debugfs_create_dir("slab", NULL);
>> +	if (!IS_ERR(slab_debugfs_root)) {
>> +
>> +		slab_state = FULL;
> 
> Again, no need to check.
> 
>> +
>> +		list_for_each_entry(s, &slab_caches, list)
>> +			debugfs_slab_add(s);
>> +	} else {
>> +		pr_err("Cannot create slab debugfs.\n");
>> +		return IS_ERR(slab_debugfs_root);
>> +	}
>> +
>> +	while (alias_list) {
>> +		struct saved_alias *al = alias_list;
>> +
>> +		alias_list = alias_list->next;
>> +
>> +		err = debugfs_slab_alias(al->s, al->name);
>> +		if (err)
>> +			pr_err("SLUB: Unable to add boot slab alias %s to debugfs\n",
>> +			       al->name);
> 
> Why does this matter?
> 
updated in new patch,
https://lore.kernel.org/linux-mm/1621341949-26762-1-git-send-email-faiyazm@codeaurora.org/T/#u
>> +		kfree(al);
>> +	}
>> +
>> +	return 0;
> 
> See, you always return 0, no need to check the above stuff :)
> 
> thanks,
> 
> greg k-h
> 


Thanks and regards,
Mohammed Faiyaz

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5] mm: slub: move sysfs slab alloc/free interfaces to debugfs
  2021-05-18 12:54   ` Faiyaz Mohammed
@ 2021-05-18 13:33     ` Greg KH
  0 siblings, 0 replies; 13+ messages in thread
From: Greg KH @ 2021-05-18 13:33 UTC (permalink / raw)
  To: Faiyaz Mohammed
  Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, linux-mm,
	linux-kernel, glittao, vinmenon

On Tue, May 18, 2021 at 06:24:48PM +0530, Faiyaz Mohammed wrote:
> 
> 
> On 5/7/2021 5:18 PM, Greg KH wrote:
> > On Thu, May 06, 2021 at 03:52:03PM +0530, Faiyaz Mohammed wrote:
> >> alloc_calls and free_calls implementation in sysfs have two issues,
> >> one is PAGE_SIZE limitiation of sysfs and other is it does not adhere
> >> to "one value per file" rule.
> >>
> >> To overcome this issues, move the alloc_calls and free_calls implemeation
> >> to debugfs.
> >>
> >> Rename the alloc_calls/free_calls to alloc_traces/free_traces,
> >> to be inline with what it does.
> >>
> >> Signed-off-by: Faiyaz Mohammed <faiyazm@codeaurora.org>
> >> ---
> >>  include/linux/slub_def.h |  10 ++
> >>  mm/slab_common.c         |   9 ++
> >>  mm/slub.c                | 362 ++++++++++++++++++++++++++++++++++++-----------
> >>  3 files changed, 299 insertions(+), 82 deletions(-)
> >>
> >> diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
> >> index dcde82a..f8c268d 100644
> >> --- a/include/linux/slub_def.h
> >> +++ b/include/linux/slub_def.h
> >> @@ -110,6 +110,9 @@ struct kmem_cache {
> >>  #ifdef CONFIG_SYSFS
> >>  	struct kobject kobj;	/* For sysfs */
> >>  #endif
> >> +#ifdef CONFIG_SLUB_DEBUG
> >> +	struct dentry *slab_cache_dentry;
> >> +#endif
> >>  #ifdef CONFIG_SLAB_FREELIST_HARDENED
> >>  	unsigned long random;
> >>  #endif
> >> @@ -159,6 +162,13 @@ static inline void sysfs_slab_release(struct kmem_cache *s)
> >>  }
> >>  #endif
> >>  
> >> +#ifdef CONFIG_DEBUG_FS
> >> +void debugfs_slab_release(struct kmem_cache *);
> >> +#else
> >> +static inline void debugfs_slab_release(struct kmem_cache *s)
> >> +{
> >> +}
> >> +#endif
> >>  void object_err(struct kmem_cache *s, struct page *page,
> >>  		u8 *object, char *reason);
> >>  
> >> diff --git a/mm/slab_common.c b/mm/slab_common.c
> >> index f8833d3..f3afe6b 100644
> >> --- a/mm/slab_common.c
> >> +++ b/mm/slab_common.c
> >> @@ -445,6 +445,9 @@ static void slab_caches_to_rcu_destroy_workfn(struct work_struct *work)
> >>  #else
> >>  		slab_kmem_cache_release(s);
> >>  #endif
> >> +#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_SLUB)
> >> +		debugfs_slab_release(s);
> >> +#endif
> > 
> > If you write your .h files correctly, no need for #ifdef in a .c file.
> > 
> > Please fix up.
> > 
> fixed in new patch and used the single #ifded because
> debugfs_slab_release declaration is there in slub_def.h and slab_common
> is used for both slab and slub.
> Like SLAB_SUPPORTS_SYSFS, SLAB_SUPPORT_DEBUGFS will be not defined if
> slab config is used.

No, you should have have any #ifdef at all, as I point out in that
review.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-05-18 13:33 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-06 10:22 [PATCH v5] mm: slub: move sysfs slab alloc/free interfaces to debugfs Faiyaz Mohammed
2021-05-06 14:29 ` kernel test robot
2021-05-06 14:29   ` kernel test robot
2021-05-06 14:32 ` kernel test robot
2021-05-06 14:32   ` kernel test robot
2021-05-07 11:48 ` Greg KH
2021-05-18 12:54   ` Faiyaz Mohammed
2021-05-18 13:33     ` Greg KH
2021-05-06 18:40 kernel test robot
2021-05-07  5:21 ` Dan Carpenter
2021-05-07  5:21 ` Dan Carpenter
2021-05-18 12:49 ` Faiyaz Mohammed
2021-05-18 12:49   ` Faiyaz Mohammed

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.