| Message ID | 1622996045-25826-1-git-send-email-faiyazm@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v10] mm: slub: move sysfs slab alloc/free interfaces to debugfs | expand |
Hi Faiyaz,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on v5.13-rc4]
[cannot apply to hnaz-linux-mm/master next-20210604]
[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/20210607-001659
base: 8124c8a6b35386f73523d27eacb71b5364a68c4c
config: sparc-randconfig-r003-20210606 (attached as .config)
compiler: sparc64-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/f7385626eb41e6154132519bd9b836f91eb7f93c
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/20210607-001659
git checkout f7385626eb41e6154132519bd9b836f91eb7f93c
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sparc
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from include/linux/preempt.h:11,
from include/linux/spinlock.h:51,
from include/linux/mmzone.h:8,
from include/linux/gfp.h:6,
from include/linux/mm.h:10,
from mm/slub.c:13:
mm/slub.c: In function 'slab_debug_trace_open':
>> include/linux/list.h:628:2: warning: this 'for' clause does not guard... [-Wmisleading-indentation]
628 | for (pos = list_first_entry(head, typeof(*pos), member); \
| ^~~
mm/slub.c:5863:3: note: in expansion of macro 'list_for_each_entry'
5863 | list_for_each_entry(page, &n->full, slab_list)
| ^~~~~~~~~~~~~~~~~~~
mm/slub.c:5865:4: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'for'
5865 | spin_unlock_irqrestore(&n->list_lock, flags);
| ^~~~~~~~~~~~~~~~~~~~~~
vim +/for +628 include/linux/list.h
6d7581e62f8be4 Jiri Pirko 2013-05-29 548
008208c6b26f21 Oleg Nesterov 2013-11-12 549 /**
008208c6b26f21 Oleg Nesterov 2013-11-12 550 * list_next_entry - get the next element in list
008208c6b26f21 Oleg Nesterov 2013-11-12 551 * @pos: the type * to cursor
3943f42c11896c Andrey Utkin 2014-11-14 552 * @member: the name of the list_head within the struct.
008208c6b26f21 Oleg Nesterov 2013-11-12 553 */
008208c6b26f21 Oleg Nesterov 2013-11-12 554 #define list_next_entry(pos, member) \
008208c6b26f21 Oleg Nesterov 2013-11-12 555 list_entry((pos)->member.next, typeof(*(pos)), member)
008208c6b26f21 Oleg Nesterov 2013-11-12 556
008208c6b26f21 Oleg Nesterov 2013-11-12 557 /**
008208c6b26f21 Oleg Nesterov 2013-11-12 558 * list_prev_entry - get the prev element in list
008208c6b26f21 Oleg Nesterov 2013-11-12 559 * @pos: the type * to cursor
3943f42c11896c Andrey Utkin 2014-11-14 560 * @member: the name of the list_head within the struct.
008208c6b26f21 Oleg Nesterov 2013-11-12 561 */
008208c6b26f21 Oleg Nesterov 2013-11-12 562 #define list_prev_entry(pos, member) \
008208c6b26f21 Oleg Nesterov 2013-11-12 563 list_entry((pos)->member.prev, typeof(*(pos)), member)
008208c6b26f21 Oleg Nesterov 2013-11-12 564
^1da177e4c3f41 Linus Torvalds 2005-04-16 565 /**
^1da177e4c3f41 Linus Torvalds 2005-04-16 566 * list_for_each - iterate over a list
8e3a67a99231f9 Randy Dunlap 2006-06-25 567 * @pos: the &struct list_head to use as a loop cursor.
^1da177e4c3f41 Linus Torvalds 2005-04-16 568 * @head: the head for your list.
^1da177e4c3f41 Linus Torvalds 2005-04-16 569 */
^1da177e4c3f41 Linus Torvalds 2005-04-16 570 #define list_for_each(pos, head) \
e66eed651fd18a Linus Torvalds 2011-05-19 571 for (pos = (head)->next; pos != (head); pos = pos->next)
^1da177e4c3f41 Linus Torvalds 2005-04-16 572
28ca0d6d39ab1d Pavel Begunkov 2019-11-29 573 /**
28ca0d6d39ab1d Pavel Begunkov 2019-11-29 574 * list_for_each_continue - continue iteration over a list
28ca0d6d39ab1d Pavel Begunkov 2019-11-29 575 * @pos: the &struct list_head to use as a loop cursor.
28ca0d6d39ab1d Pavel Begunkov 2019-11-29 576 * @head: the head for your list.
28ca0d6d39ab1d Pavel Begunkov 2019-11-29 577 *
28ca0d6d39ab1d Pavel Begunkov 2019-11-29 578 * Continue to iterate over a list, continuing after the current position.
28ca0d6d39ab1d Pavel Begunkov 2019-11-29 579 */
28ca0d6d39ab1d Pavel Begunkov 2019-11-29 580 #define list_for_each_continue(pos, head) \
28ca0d6d39ab1d Pavel Begunkov 2019-11-29 581 for (pos = pos->next; pos != (head); pos = pos->next)
28ca0d6d39ab1d Pavel Begunkov 2019-11-29 582
^1da177e4c3f41 Linus Torvalds 2005-04-16 583 /**
^1da177e4c3f41 Linus Torvalds 2005-04-16 584 * list_for_each_prev - iterate over a list backwards
8e3a67a99231f9 Randy Dunlap 2006-06-25 585 * @pos: the &struct list_head to use as a loop cursor.
^1da177e4c3f41 Linus Torvalds 2005-04-16 586 * @head: the head for your list.
^1da177e4c3f41 Linus Torvalds 2005-04-16 587 */
^1da177e4c3f41 Linus Torvalds 2005-04-16 588 #define list_for_each_prev(pos, head) \
e66eed651fd18a Linus Torvalds 2011-05-19 589 for (pos = (head)->prev; pos != (head); pos = pos->prev)
^1da177e4c3f41 Linus Torvalds 2005-04-16 590
^1da177e4c3f41 Linus Torvalds 2005-04-16 591 /**
^1da177e4c3f41 Linus Torvalds 2005-04-16 592 * list_for_each_safe - iterate over a list safe against removal of list entry
8e3a67a99231f9 Randy Dunlap 2006-06-25 593 * @pos: the &struct list_head to use as a loop cursor.
^1da177e4c3f41 Linus Torvalds 2005-04-16 594 * @n: another &struct list_head to use as temporary storage
^1da177e4c3f41 Linus Torvalds 2005-04-16 595 * @head: the head for your list.
^1da177e4c3f41 Linus Torvalds 2005-04-16 596 */
^1da177e4c3f41 Linus Torvalds 2005-04-16 597 #define list_for_each_safe(pos, n, head) \
^1da177e4c3f41 Linus Torvalds 2005-04-16 598 for (pos = (head)->next, n = pos->next; pos != (head); \
^1da177e4c3f41 Linus Torvalds 2005-04-16 599 pos = n, n = pos->next)
^1da177e4c3f41 Linus Torvalds 2005-04-16 600
37c42524d60906 Denis V. Lunev 2007-10-16 601 /**
8f731f7d83d6c6 Randy Dunlap 2007-10-18 602 * list_for_each_prev_safe - iterate over a list backwards safe against removal of list entry
37c42524d60906 Denis V. Lunev 2007-10-16 603 * @pos: the &struct list_head to use as a loop cursor.
37c42524d60906 Denis V. Lunev 2007-10-16 604 * @n: another &struct list_head to use as temporary storage
37c42524d60906 Denis V. Lunev 2007-10-16 605 * @head: the head for your list.
37c42524d60906 Denis V. Lunev 2007-10-16 606 */
37c42524d60906 Denis V. Lunev 2007-10-16 607 #define list_for_each_prev_safe(pos, n, head) \
37c42524d60906 Denis V. Lunev 2007-10-16 608 for (pos = (head)->prev, n = pos->prev; \
e66eed651fd18a Linus Torvalds 2011-05-19 609 pos != (head); \
37c42524d60906 Denis V. Lunev 2007-10-16 610 pos = n, n = pos->prev)
37c42524d60906 Denis V. Lunev 2007-10-16 611
e130816164e244 Andy Shevchenko 2020-10-15 612 /**
e130816164e244 Andy Shevchenko 2020-10-15 613 * list_entry_is_head - test if the entry points to the head of the list
e130816164e244 Andy Shevchenko 2020-10-15 614 * @pos: the type * to cursor
e130816164e244 Andy Shevchenko 2020-10-15 615 * @head: the head for your list.
e130816164e244 Andy Shevchenko 2020-10-15 616 * @member: the name of the list_head within the struct.
e130816164e244 Andy Shevchenko 2020-10-15 617 */
e130816164e244 Andy Shevchenko 2020-10-15 618 #define list_entry_is_head(pos, head, member) \
e130816164e244 Andy Shevchenko 2020-10-15 619 (&pos->member == (head))
e130816164e244 Andy Shevchenko 2020-10-15 620
^1da177e4c3f41 Linus Torvalds 2005-04-16 621 /**
^1da177e4c3f41 Linus Torvalds 2005-04-16 622 * list_for_each_entry - iterate over list of given type
8e3a67a99231f9 Randy Dunlap 2006-06-25 623 * @pos: the type * to use as a loop cursor.
^1da177e4c3f41 Linus Torvalds 2005-04-16 624 * @head: the head for your list.
3943f42c11896c Andrey Utkin 2014-11-14 625 * @member: the name of the list_head within the struct.
^1da177e4c3f41 Linus Torvalds 2005-04-16 626 */
^1da177e4c3f41 Linus Torvalds 2005-04-16 627 #define list_for_each_entry(pos, head, member) \
93be3c2eb3371f Oleg Nesterov 2013-11-12 @628 for (pos = list_first_entry(head, typeof(*pos), member); \
e130816164e244 Andy Shevchenko 2020-10-15 629 !list_entry_is_head(pos, head, member); \
8120e2e5141a42 Oleg Nesterov 2013-11-12 630 pos = list_next_entry(pos, member))
^1da177e4c3f41 Linus Torvalds 2005-04-16 631
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Sun, Jun 6, 2021 at 7:16 PM Faiyaz Mohammed <faiyazm@codeaurora.org> 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 limitation > to "one value per file" rule. > > To overcome this issues, move the alloc_calls and free_calls implemeation implementation > to debugfs. > > Debugfs cache will be created if SLAB_STORE_USER flag is set. > > Rename the alloc_calls/free_calls to alloc_traces/free_traces, > to be inline with what it does. ... > +#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_SLUB_DEBUG) > +void debugfs_slab_release(struct kmem_cache *); > +#else > +static inline void debugfs_slab_release(struct kmem_cache *s) > +{ > +} It can be one line. > +#endif ... > + 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), Hmm... Why is the cast needed here? > + l->max_time); > + } else > + seq_printf(seq, " age=%ld", > + l->min_time); ... > + if (num_online_cpus() > 1 && > + !cpumask_empty(to_cpumask(l->cpus))) One line? ... > +static const struct seq_operations slab_debugfs_sops = { > + .start = slab_debugfs_start, > + .next = slab_debugfs_next, > + .stop = slab_debugfs_stop, > + .show = slab_debugfs_show Leave a comma here. It might not be the last one in the future. > +}; + blank line? > +static int slab_debug_trace_open(struct inode *inode, struct file *filep) > +{ ... > +static const struct file_operations slab_debugfs_fops = { > + .open = slab_debug_trace_open, > + .read = seq_read, > + .llseek = seq_lseek, > + .release = slab_debug_trace_release, > +}; > + > + One blank line is enough. ... > + debugfs_remove_recursive(debugfs_lookup(s->name, > + slab_debugfs_root)); One line?
On 6/6/2021 9:44 PM, 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. > > Debugfs cache will be created if SLAB_STORE_USER flag is set. > > 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> > --- > mm/slab.h | 8 ++ > mm/slab_common.c | 2 + > mm/slub.c | 292 +++++++++++++++++++++++++++++++++++++------------------ > 3 files changed, 209 insertions(+), 93 deletions(-) > > diff --git a/mm/slab.h b/mm/slab.h > index 18c1927..3b60925 100644 > --- a/mm/slab.h > +++ b/mm/slab.h > @@ -630,6 +630,14 @@ static inline bool slab_want_init_on_free(struct kmem_cache *c) > return false; > } > > +#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_SLUB_DEBUG) > +void debugfs_slab_release(struct kmem_cache *); > +#else > +static inline void debugfs_slab_release(struct kmem_cache *s) > +{ > +} > +#endif > + > #ifdef CONFIG_PRINTK > #define KS_ADDRS_COUNT 16 > struct kmem_obj_info { > diff --git a/mm/slab_common.c b/mm/slab_common.c > index a4a5714..ee5456f 100644 > --- a/mm/slab_common.c > +++ b/mm/slab_common.c > @@ -449,6 +449,7 @@ static void slab_caches_to_rcu_destroy_workfn(struct work_struct *work) > rcu_barrier(); > > list_for_each_entry_safe(s, s2, &to_destroy, list) { > + debugfs_slab_release(s); > kfence_shutdown_cache(s); > #ifdef SLAB_SUPPORTS_SYSFS > sysfs_slab_release(s); > @@ -476,6 +477,7 @@ static int shutdown_cache(struct kmem_cache *s) > schedule_work(&slab_caches_to_rcu_destroy_work); > } else { > kfence_shutdown_cache(s); > + debugfs_slab_release(s); > #ifdef SLAB_SUPPORTS_SYSFS > sysfs_slab_unlink(s); > sysfs_slab_release(s); > diff --git a/mm/slub.c b/mm/slub.c > index 3f96e09..974c42bd 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,12 @@ static inline int sysfs_slab_alias(struct kmem_cache *s, const char *p) > { return 0; } > #endif > > +#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_SLUB_DEBUG) > +static void debugfs_slab_add(struct kmem_cache *); > +#else > +static inline void debugfs_slab_add(struct kmem_cache *s) { } > +#endif > + > static inline void stat(const struct kmem_cache *s, enum stat_item si) > { > #ifdef CONFIG_SLUB_STATS > @@ -4546,6 +4553,9 @@ int __kmem_cache_create(struct kmem_cache *s, slab_flags_t flags) > if (err) > __kmem_cache_release(s); > > + if (s->flags & SLAB_STORE_USER) > + debugfs_slab_add(s); > + > return err; > } > > @@ -4686,6 +4696,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. > @@ -4709,6 +4721,8 @@ struct loc_track { > struct location *loc; > }; > > +static struct dentry *slab_debugfs_root; > + > static void free_loc_track(struct loc_track *t) > { > if (t->max) > @@ -4825,82 +4839,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 > @@ -5350,21 +5289,6 @@ static ssize_t validate_store(struct kmem_cache *s, > } > 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); > -} > -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); > -} > -SLAB_ATTR_RO(free_calls); > #endif /* CONFIG_SLUB_DEBUG */ > > #ifdef CONFIG_FAILSLAB > @@ -5528,8 +5452,6 @@ static struct attribute *slab_attrs[] = { > &poison_attr.attr, > &store_user_attr.attr, > &validate_attr.attr, > - &alloc_calls_attr.attr, > - &free_calls_attr.attr, > #endif > #ifdef CONFIG_ZONE_DMA > &cache_dma_attr.attr, > @@ -5818,6 +5740,190 @@ 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 slab_debugfs_show(struct seq_file *seq, void *v) > +{ > + > + struct location *l; > + unsigned int idx = *(unsigned int *)v; > + struct loc_track *t = seq->private; > + > + 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 (!idx && !t->count) > + seq_puts(seq, "No data\n"); > + > + return 0; > +} > + > +static void slab_debugfs_stop(struct seq_file *seq, void *v) > +{ > + kfree(v); > +} > + > +static void *slab_debugfs_next(struct seq_file *seq, void *v, loff_t *ppos) > +{ > + loff_t *spos = v; > + struct loc_track *t = seq->private; > + > + if (*ppos < t->count) { > + *ppos = ++*spos; > + return spos; > + } > + *ppos = ++*spos; > + return NULL; > +} > + > +static void *slab_debugfs_start(struct seq_file *seq, loff_t *ppos) > +{ > + loff_t *spos = kmalloc(sizeof(loff_t), GFP_KERNEL); > + > + if (!spos) > + return NULL; > + > + *spos = *ppos; > + return spos; > +} > + > +static const struct seq_operations slab_debugfs_sops = { > + .start = slab_debugfs_start, > + .next = slab_debugfs_next, > + .stop = slab_debugfs_stop, > + .show = slab_debugfs_show > +}; > +static int slab_debug_trace_open(struct inode *inode, struct file *filep) > +{ > + > + struct kmem_cache_node *n; > + enum track_item alloc; > + int node; > + struct loc_track *t = __seq_open_private(filep, &slab_debugfs_sops, > + sizeof(struct loc_track)); > + struct kmem_cache *s = file_inode(filep)->i_private; > + > + if (strcmp(filep->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)) { > + pr_err("Out of memory\n"); > + return -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); oh, due to extra tab it moved inside the list_for_ech_entry, will fixed in next patch version. > + } > + > + return 0; > +} > + > +static int slab_debug_trace_release(struct inode *inode, struct file *file) > +{ > + struct seq_file *seq = file->private_data; > + struct loc_track *t = seq->private; > + > + free_loc_track(t); > + kfree(seq->private); > + seq->private = NULL; > + return seq_release(inode, file); > +} > + > +static const struct file_operations slab_debugfs_fops = { > + .open = slab_debug_trace_open, > + .read = seq_read, > + .llseek = seq_lseek, > + .release = slab_debug_trace_release, > +}; > + > + > +static void debugfs_slab_add(struct kmem_cache *s) > +{ > + struct dentry *slab_cache_dir; > + > + if (unlikely(!slab_debugfs_root)) > + return; > + > + slab_cache_dir = debugfs_create_dir(s->name, slab_debugfs_root); > + > + debugfs_create_file("alloc_traces", 0400, > + slab_cache_dir, s, &slab_debugfs_fops); > + > + debugfs_create_file("free_traces", 0400, > + slab_cache_dir, s, &slab_debugfs_fops); > +} > + > +void debugfs_slab_release(struct kmem_cache *s) > +{ > + debugfs_remove_recursive(debugfs_lookup(s->name, > + slab_debugfs_root)); > +} > + > +static int __init slab_debugfs_init(void) > +{ > + struct kmem_cache *s; > + > + slab_debugfs_root = debugfs_create_dir("slab", NULL); > + > + list_for_each_entry(s, &slab_caches, list) > + if (s->flags & SLAB_STORE_USER) > + debugfs_slab_add(s); > + > + return 0; > + > +} > +__initcall(slab_debugfs_init); > +#endif > /* > * The /proc/slabinfo ABI > */ >
On 6/7/2021 2:01 AM, Andy Shevchenko wrote: > On Sun, Jun 6, 2021 at 7:16 PM Faiyaz Mohammed <faiyazm@codeaurora.org> 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 > > limitation > >> to "one value per file" rule. >> >> To overcome this issues, move the alloc_calls and free_calls implemeation > > implementation > >> to debugfs. >> >> Debugfs cache will be created if SLAB_STORE_USER flag is set. >> >> Rename the alloc_calls/free_calls to alloc_traces/free_traces, >> to be inline with what it does. > ... > >> +#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_SLUB_DEBUG) >> +void debugfs_slab_release(struct kmem_cache *); >> +#else > >> +static inline void debugfs_slab_release(struct kmem_cache *s) >> +{ >> +} > > It can be one line. > >> +#endif > > ... > > >> + 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), > > Hmm... Why is the cast needed here? > To avoid below warning while preparing build for arm/32 bit, "format ‘%ld’ expects argument of type ‘long int’, but argument 4 has type ‘u64 {aka long long unsigned int}" . >> + l->max_time); >> + } else >> + seq_printf(seq, " age=%ld", >> + l->min_time); > > ... > >> + if (num_online_cpus() > 1 && >> + !cpumask_empty(to_cpumask(l->cpus))) > > One line? > > ... > >> +static const struct seq_operations slab_debugfs_sops = { >> + .start = slab_debugfs_start, >> + .next = slab_debugfs_next, >> + .stop = slab_debugfs_stop, > >> + .show = slab_debugfs_show > > Leave a comma here. It might not be the last one in the future. > >> +}; > > + blank line? > >> +static int slab_debug_trace_open(struct inode *inode, struct file *filep) >> +{ > > ... > >> +static const struct file_operations slab_debugfs_fops = { >> + .open = slab_debug_trace_open, >> + .read = seq_read, >> + .llseek = seq_lseek, >> + .release = slab_debug_trace_release, >> +}; >> + >> + > > One blank line is enough. > > ... > >> + debugfs_remove_recursive(debugfs_lookup(s->name, >> + slab_debugfs_root)); > > One line? > Will update other comments in next patch. Thanks and regards, Mohammed Faiyaz
On Mon, Jun 7, 2021 at 5:40 AM Faiyaz Mohammed <faiyazm@codeaurora.org> wrote: > On 6/7/2021 2:01 AM, Andy Shevchenko wrote: > > On Sun, Jun 6, 2021 at 7:16 PM Faiyaz Mohammed <faiyazm@codeaurora.org> wrote: ... > >> + 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), > > > > Hmm... Why is the cast needed here? > > > To avoid below warning while preparing build for arm/32 bit, > "format ‘%ld’ expects argument of type ‘long int’, but argument 4 has > type ‘u64 {aka long long unsigned int}" . Perhaps use %llu? > >> + l->max_time); > >> + } else > >> + seq_printf(seq, " age=%ld", > >> + l->min_time);
On 6/6/21 6:14 PM, 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. > > Debugfs cache will be created if SLAB_STORE_USER flag is set. > > 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> > --- > mm/slab.h | 8 ++ > mm/slab_common.c | 2 + > mm/slub.c | 292 +++++++++++++++++++++++++++++++++++++------------------ > 3 files changed, 209 insertions(+), 93 deletions(-) > ... > +static int slab_debug_trace_open(struct inode *inode, struct file *filep) > +{ > + > + struct kmem_cache_node *n; > + enum track_item alloc; > + int node; > + struct loc_track *t = __seq_open_private(filep, &slab_debugfs_sops, > + sizeof(struct loc_track)); > + struct kmem_cache *s = file_inode(filep)->i_private; > + > + if (strcmp(filep->f_path.dentry->d_name.name, "alloc_traces") == 0) > + alloc = TRACK_ALLOC; ^ extra space here? > + else > + alloc = TRACK_FREE; same here > + > + if (!alloc_loc_track(t, PAGE_SIZE / sizeof(struct location), GFP_KERNEL)) { > + pr_err("Out of memory\n"); Hm I would remove this. It doesn't print any context, so it's not useful to let users know where/why we ran out of memory. Also if a GFP_KERNEL allocation fails, there will be a big warning including stacktrace from the page allocator anyway. > + return -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); At least this is not Python, so it's just a visual flaw :) > + } > + > + return 0; > +} > + > +static int slab_debug_trace_release(struct inode *inode, struct file *file) > +{ > + struct seq_file *seq = file->private_data; > + struct loc_track *t = seq->private; > + > + free_loc_track(t); > + kfree(seq->private); > + seq->private = NULL; > + return seq_release(inode, file); You can call seq_release_private() instead and deal just with free_loc_track here. Thanks! Vlastimil
On 6/7/2021 2:01 AM, Andy Shevchenko wrote: > On Sun, Jun 6, 2021 at 7:16 PM Faiyaz Mohammed <faiyazm@codeaurora.org> 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 > > limitation > >> to "one value per file" rule. >> >> To overcome this issues, move the alloc_calls and free_calls implemeation > > implementation > >> to debugfs. >> >> Debugfs cache will be created if SLAB_STORE_USER flag is set. >> >> Rename the alloc_calls/free_calls to alloc_traces/free_traces, >> to be inline with what it does. > > ... > >> +#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_SLUB_DEBUG) >> +void debugfs_slab_release(struct kmem_cache *); >> +#else > >> +static inline void debugfs_slab_release(struct kmem_cache *s) >> +{ >> +} > > It can be one line. > >> +#endif > > ... > > >> + 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), > > Hmm... Why is the cast needed here? > >> + l->max_time); >> + } else >> + seq_printf(seq, " age=%ld", >> + l->min_time); > > ... > >> + if (num_online_cpus() > 1 && >> + !cpumask_empty(to_cpumask(l->cpus))) > > One line? > I have split the line because it is crossing the 80 columns. In this case it's okay to cross 80 columns? > ... > >> +static const struct seq_operations slab_debugfs_sops = { >> + .start = slab_debugfs_start, >> + .next = slab_debugfs_next, >> + .stop = slab_debugfs_stop, > >> + .show = slab_debugfs_show > > Leave a comma here. It might not be the last one in the future. > >> +}; > > + blank line? > >> +static int slab_debug_trace_open(struct inode *inode, struct file *filep) >> +{ > > ... > >> +static const struct file_operations slab_debugfs_fops = { >> + .open = slab_debug_trace_open, >> + .read = seq_read, >> + .llseek = seq_lseek, >> + .release = slab_debug_trace_release, >> +}; >> + >> + > > One blank line is enough. > > ... > >> + debugfs_remove_recursive(debugfs_lookup(s->name, >> + slab_debugfs_root)); > > One line? > Same here!
On Mon, Jun 7, 2021 at 4:55 PM Faiyaz Mohammed <faiyazm@codeaurora.org> wrote: > On 6/7/2021 2:01 AM, Andy Shevchenko wrote: > > On Sun, Jun 6, 2021 at 7:16 PM Faiyaz Mohammed <faiyazm@codeaurora.org> wrote: ... > >> + if (num_online_cpus() > 1 && > >> + !cpumask_empty(to_cpumask(l->cpus))) > > > > One line? > > > I have split the line because it is crossing the 80 columns. In this > case it's okay to cross 80 columns? For how many characters? If it's 3 or 4 or so, it's fine to have it on one line. Use common sense here. ... > >> + debugfs_remove_recursive(debugfs_lookup(s->name, > >> + slab_debugfs_root)); > > > > One line? > > > Same here! Same here.
diff --git a/mm/slab.h b/mm/slab.h index 18c1927..3b60925 100644 --- a/mm/slab.h +++ b/mm/slab.h @@ -630,6 +630,14 @@ static inline bool slab_want_init_on_free(struct kmem_cache *c) return false; } +#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_SLUB_DEBUG) +void debugfs_slab_release(struct kmem_cache *); +#else +static inline void debugfs_slab_release(struct kmem_cache *s) +{ +} +#endif + #ifdef CONFIG_PRINTK #define KS_ADDRS_COUNT 16 struct kmem_obj_info { diff --git a/mm/slab_common.c b/mm/slab_common.c index a4a5714..ee5456f 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -449,6 +449,7 @@ static void slab_caches_to_rcu_destroy_workfn(struct work_struct *work) rcu_barrier(); list_for_each_entry_safe(s, s2, &to_destroy, list) { + debugfs_slab_release(s); kfence_shutdown_cache(s); #ifdef SLAB_SUPPORTS_SYSFS sysfs_slab_release(s); @@ -476,6 +477,7 @@ static int shutdown_cache(struct kmem_cache *s) schedule_work(&slab_caches_to_rcu_destroy_work); } else { kfence_shutdown_cache(s); + debugfs_slab_release(s); #ifdef SLAB_SUPPORTS_SYSFS sysfs_slab_unlink(s); sysfs_slab_release(s); diff --git a/mm/slub.c b/mm/slub.c index 3f96e09..974c42bd 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,12 @@ static inline int sysfs_slab_alias(struct kmem_cache *s, const char *p) { return 0; } #endif +#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_SLUB_DEBUG) +static void debugfs_slab_add(struct kmem_cache *); +#else +static inline void debugfs_slab_add(struct kmem_cache *s) { } +#endif + static inline void stat(const struct kmem_cache *s, enum stat_item si) { #ifdef CONFIG_SLUB_STATS @@ -4546,6 +4553,9 @@ int __kmem_cache_create(struct kmem_cache *s, slab_flags_t flags) if (err) __kmem_cache_release(s); + if (s->flags & SLAB_STORE_USER) + debugfs_slab_add(s); + return err; } @@ -4686,6 +4696,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. @@ -4709,6 +4721,8 @@ struct loc_track { struct location *loc; }; +static struct dentry *slab_debugfs_root; + static void free_loc_track(struct loc_track *t) { if (t->max) @@ -4825,82 +4839,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 @@ -5350,21 +5289,6 @@ static ssize_t validate_store(struct kmem_cache *s, } 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); -} -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); -} -SLAB_ATTR_RO(free_calls); #endif /* CONFIG_SLUB_DEBUG */ #ifdef CONFIG_FAILSLAB @@ -5528,8 +5452,6 @@ static struct attribute *slab_attrs[] = { &poison_attr.attr, &store_user_attr.attr, &validate_attr.attr, - &alloc_calls_attr.attr, - &free_calls_attr.attr, #endif #ifdef CONFIG_ZONE_DMA &cache_dma_attr.attr, @@ -5818,6 +5740,190 @@ 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 slab_debugfs_show(struct seq_file *seq, void *v) +{ + + struct location *l; + unsigned int idx = *(unsigned int *)v; + struct loc_track *t = seq->private; + + 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 (!idx && !t->count) + seq_puts(seq, "No data\n"); + + return 0; +} + +static void slab_debugfs_stop(struct seq_file *seq, void *v) +{ + kfree(v); +} + +static void *slab_debugfs_next(struct seq_file *seq, void *v, loff_t *ppos) +{ + loff_t *spos = v; + struct loc_track *t = seq->private; + + if (*ppos < t->count) { + *ppos = ++*spos; + return spos; + } + *ppos = ++*spos; + return NULL; +} + +static void *slab_debugfs_start(struct seq_file *seq, loff_t *ppos) +{ + loff_t *spos = kmalloc(sizeof(loff_t), GFP_KERNEL); + + if (!spos) + return NULL; + + *spos = *ppos; + return spos; +} + +static const struct seq_operations slab_debugfs_sops = { + .start = slab_debugfs_start, + .next = slab_debugfs_next, + .stop = slab_debugfs_stop, + .show = slab_debugfs_show +}; +static int slab_debug_trace_open(struct inode *inode, struct file *filep) +{ + + struct kmem_cache_node *n; + enum track_item alloc; + int node; + struct loc_track *t = __seq_open_private(filep, &slab_debugfs_sops, + sizeof(struct loc_track)); + struct kmem_cache *s = file_inode(filep)->i_private; + + if (strcmp(filep->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)) { + pr_err("Out of memory\n"); + return -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); + } + + return 0; +} + +static int slab_debug_trace_release(struct inode *inode, struct file *file) +{ + struct seq_file *seq = file->private_data; + struct loc_track *t = seq->private; + + free_loc_track(t); + kfree(seq->private); + seq->private = NULL; + return seq_release(inode, file); +} + +static const struct file_operations slab_debugfs_fops = { + .open = slab_debug_trace_open, + .read = seq_read, + .llseek = seq_lseek, + .release = slab_debug_trace_release, +}; + + +static void debugfs_slab_add(struct kmem_cache *s) +{ + struct dentry *slab_cache_dir; + + if (unlikely(!slab_debugfs_root)) + return; + + slab_cache_dir = debugfs_create_dir(s->name, slab_debugfs_root); + + debugfs_create_file("alloc_traces", 0400, + slab_cache_dir, s, &slab_debugfs_fops); + + debugfs_create_file("free_traces", 0400, + slab_cache_dir, s, &slab_debugfs_fops); +} + +void debugfs_slab_release(struct kmem_cache *s) +{ + debugfs_remove_recursive(debugfs_lookup(s->name, + slab_debugfs_root)); +} + +static int __init slab_debugfs_init(void) +{ + struct kmem_cache *s; + + slab_debugfs_root = debugfs_create_dir("slab", NULL); + + list_for_each_entry(s, &slab_caches, list) + if (s->flags & SLAB_STORE_USER) + debugfs_slab_add(s); + + return 0; + +} +__initcall(slab_debugfs_init); +#endif /* * The /proc/slabinfo ABI */
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. Debugfs cache will be created if SLAB_STORE_USER flag is set. 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> --- mm/slab.h | 8 ++ mm/slab_common.c | 2 + mm/slub.c | 292 +++++++++++++++++++++++++++++++++++++------------------ 3 files changed, 209 insertions(+), 93 deletions(-)