From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 85BB4C433B4 for ; Mon, 19 Apr 2021 12:12:19 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id CA06061057 for ; Mon, 19 Apr 2021 12:12:18 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CA06061057 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 5BEA46B0036; Mon, 19 Apr 2021 08:12:18 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 547966B006E; Mon, 19 Apr 2021 08:12:18 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 39C276B0070; Mon, 19 Apr 2021 08:12:18 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0191.hostedemail.com [216.40.44.191]) by kanga.kvack.org (Postfix) with ESMTP id 1E9516B0036 for ; Mon, 19 Apr 2021 08:12:18 -0400 (EDT) Received: from smtpin11.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id BAB28181AEF3C for ; Mon, 19 Apr 2021 12:12:17 +0000 (UTC) X-FDA: 78049003914.11.28DB906 Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by imf16.hostedemail.com (Postfix) with ESMTP id 5CECF80192F3 for ; Mon, 19 Apr 2021 12:12:16 +0000 (UTC) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id 9FB71B2F0; Mon, 19 Apr 2021 12:12:15 +0000 (UTC) To: Faiyaz Mohammed , cl@linux.com, penberg@kernel.org, rientjes@google.com, iamjoonsoo.kim@lge.com, akpm@linux-foundation.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org Cc: vinmenon@codeaurora.org, Oliver Glitta References: <1618583239-18124-1-git-send-email-faiyazm@codeaurora.org> From: Vlastimil Babka Subject: Re: [PATCH v4] mm: slub: move sysfs slab alloc/free interfaces to debugfs Message-ID: <8a4a06f1-f72f-6145-71da-ac458c16312d@suse.cz> Date: Mon, 19 Apr 2021 14:12:05 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 MIME-Version: 1.0 In-Reply-To: <1618583239-18124-1-git-send-email-faiyazm@codeaurora.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: 5CECF80192F3 X-Stat-Signature: 1qwa9etac98tpnkwj7juiruppjqf6fwt Received-SPF: none (suse.cz>: No applicable sender policy available) receiver=imf16; identity=mailfrom; envelope-from=""; helo=mx2.suse.de; client-ip=195.135.220.15 X-HE-DKIM-Result: none/none X-HE-Tag: 1618834336-455908 Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 4/16/21 4:27 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. >=20 > To overcome this issues, move the alloc_calls and free_calls implemeati= on > to debugfs. >=20 > Signed-off-by: Faiyaz Mohammed > --- > include/linux/slub_def.h | 10 +++ > mm/slab_common.c | 9 +++ > mm/slub.c | 202 +++++++++++++++++++++++++++++++++++++++= +++----- > 3 files changed, 200 insertions(+), 21 deletions(-) The patch doesn't compile, as the bots noticed. Had to add a missing #end= if after list_locations(). > 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 > =20 > +#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); > =20 > diff --git a/mm/slab_common.c b/mm/slab_common.c > index 88e8339..fb28328 100644 > --- a/mm/slab_common.c > +++ b/mm/slab_common.c > @@ -437,6 +437,9 @@ static void slab_caches_to_rcu_destroy_workfn(struc= t work_struct *work) > #else > slab_kmem_cache_release(s); > #endif > +#ifdef CONFIG_DEBUG_FS > + debugfs_slab_release(s); > +#endif > } > } > =20 > @@ -454,6 +457,9 @@ static int shutdown_cache(struct kmem_cache *s) > #ifdef SLAB_SUPPORTS_SYSFS > sysfs_slab_unlink(s); > #endif > +#ifdef CONFIG_DEBUG_FS > + debugfs_slab_release(s); > +#endif > list_add_tail(&s->list, &slab_caches_to_rcu_destroy); > schedule_work(&slab_caches_to_rcu_destroy_work); > } else { > @@ -464,6 +470,9 @@ static int shutdown_cache(struct kmem_cache *s) > #else > slab_kmem_cache_release(s); > #endif > +#ifdef CONFIG_DEBUG_FS > + debugfs_slab_release(s); > +#endif > } > =20 > return 0; > diff --git a/mm/slub.c b/mm/slub.c > index 3021ce9..ab7a0d3 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -36,6 +36,7 @@ > #include > #include > =20 > +#include > #include > =20 > #include "internal.h" > @@ -225,6 +226,15 @@ static inline int sysfs_slab_alias(struct kmem_cac= he *s, const char *p) > { return 0; } > #endif > =20 > +#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 > @@ -4521,6 +4531,8 @@ __kmem_cache_alias(const char *name, unsigned int= size, unsigned int align, > s->refcount--; > s =3D NULL; > } > + > + debugfs_slab_alias(s, name); > } > =20 > return s; > @@ -4542,6 +4554,8 @@ int __kmem_cache_create(struct kmem_cache *s, sla= b_flags_t flags) > if (err) > __kmem_cache_release(s); > =20 > + debugfs_slab_add(s); > + > return err; > } > =20 > @@ -4682,6 +4696,8 @@ static long validate_slab_cache(struct kmem_cache= *s) > =20 > return count; > } > + > +#ifdef CONFIG_DEBUG_FS > /* > * Generate lists of code addresses where slabcache objects are alloca= ted > * and freed. > @@ -4705,6 +4721,8 @@ struct loc_track { > struct location *loc; > }; > =20 > +static struct dentry *slab_debugfs_root; > + > static void free_loc_track(struct loc_track *t) > { > if (t->max) > @@ -4822,10 +4840,9 @@ static void process_slab(struct loc_track *t, st= ruct kmem_cache *s, > put_map(map); > } > =20 > -static int list_locations(struct kmem_cache *s, char *buf, > +static int list_locations(struct seq_file *seq, struct kmem_cache *s, > enum track_item alloc) > { > - int len =3D 0; > unsigned long i; > struct loc_track t =3D { 0, 0, NULL }; > int node; > @@ -4833,7 +4850,8 @@ static int list_locations(struct kmem_cache *s, c= har *buf, > =20 > if (!alloc_loc_track(&t, PAGE_SIZE / sizeof(struct location), > GFP_KERNEL)) { > - return sysfs_emit(buf, "Out of memory\n"); > + seq_puts(seq, "Out of memory\n"); > + return -ENOMEM; > } > /* Push back cpu slabs */ > flush_all(s); > @@ -4856,46 +4874,46 @@ static int list_locations(struct kmem_cache *s,= char *buf, > for (i =3D 0; i < t.count; i++) { > struct location *l =3D &t.loc[i]; > =20 > - len +=3D sysfs_emit_at(buf, len, "%7ld ", l->count); > + seq_printf(seq, "%7ld ", l->count); > =20 > if (l->addr) > - len +=3D sysfs_emit_at(buf, len, "%pS", (void *)l->addr); > + seq_printf(seq, "%pS", (void *)l->addr); > else > - len +=3D sysfs_emit_at(buf, len, ""); > + seq_puts(seq, ""); > =20 > if (l->sum_time !=3D l->min_time) > - len +=3D sysfs_emit_at(buf, len, " age=3D%ld/%ld/%ld", > + seq_printf(seq, " age=3D%ld/%ld/%ld", > l->min_time, > (long)div_u64(l->sum_time, > l->count), > l->max_time); > else > - len +=3D sysfs_emit_at(buf, len, " age=3D%ld", l->min_time); > + seq_printf(seq, " age=3D%ld", l->min_time); > =20 > if (l->min_pid !=3D l->max_pid) > - len +=3D sysfs_emit_at(buf, len, " pid=3D%ld-%ld", > + seq_printf(seq, " pid=3D%ld-%ld", > l->min_pid, l->max_pid); > else > - len +=3D sysfs_emit_at(buf, len, " pid=3D%ld", > + seq_printf(seq, " pid=3D%ld", > l->min_pid); > =20 > if (num_online_cpus() > 1 && > !cpumask_empty(to_cpumask(l->cpus))) > - len +=3D sysfs_emit_at(buf, len, " cpus=3D%*pbl", > + seq_printf(seq, " cpus=3D%*pbl", > cpumask_pr_args(to_cpumask(l->cpus))); > =20 > if (nr_online_nodes > 1 && !nodes_empty(l->nodes)) > - len +=3D sysfs_emit_at(buf, len, " nodes=3D%*pbl", > + seq_printf(seq, " nodes=3D%*pbl", > nodemask_pr_args(&l->nodes)); > =20 > - len +=3D sysfs_emit_at(buf, len, "\n"); > + seq_puts(seq, "\n"); > } > =20 > free_loc_track(&t); > if (!t.count) > - len +=3D sysfs_emit_at(buf, len, "No data\n"); > + seq_puts(seq, "No data\n"); > =20 > - return len; > + return 0; > } > #endif /* CONFIG_SLUB_DEBUG */ > =20 > @@ -5348,17 +5366,23 @@ SLAB_ATTR(validate); > =20 > 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 =3D 0; > + > + len +=3D sprintf(buf, "Deprecated, use the equvalent under"); > + len +=3D sprintf(buf, "/sys/kernel/debug/slab/%s/alloc_calls", s->nam= e); This doesn't work properly, the second printf() overwrites the first one,= also it's not newline-terminated. Also debugfs does not have to be always mounted as /sys/kernel/debug. But= maybe it's common enough that the suggestion will work most of the time, so we = can probably leave it. > + return len; > } > SLAB_ATTR_RO(alloc_calls); > =20 > 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 =3D 0; > + > + len +=3D sprintf(buf, "Deprecated, use the equvalent under"); > + len +=3D sprintf(buf, "/sys/kernel/debug/slab/%s/free_calls", s->name= ); > + > + return len; > } > SLAB_ATTR_RO(free_calls); > #endif /* CONFIG_SLUB_DEBUG */ > @@ -5814,6 +5838,142 @@ static int __init slab_sysfs_init(void) > __initcall(slab_sysfs_init); > #endif /* CONFIG_SYSFS */ > =20 > +#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 =3D=3D FULL) { > + /* > + * If we have a leftover link then remove it. > + */ > + debugfs_remove(s->slab_cache_dentry); > + s->slab_cache_dentry =3D debugfs_create_symlink(name, slab_debugfs_r= oot, NULL); > + return IS_ERR(s->slab_cache_dentry); > + } > + > + al =3D kmalloc(sizeof(struct saved_alias), GFP_KERNEL); > + if (!al) > + return -ENOMEM; > + > + al->s =3D s; > + al->name =3D name; > + al->next =3D alias_list; > + alias_list =3D al; > + return 0; > +} > + > +static int slab_debug_trace(struct seq_file *seq, void *ignored) > +{ > + struct kmem_cache *s =3D seq->private; > + > + if (!(s->flags & SLAB_STORE_USER)) > + return 0; > + > + if (strcmp(seq->file->f_path.dentry->d_name.name, "alloc_trace") =3D=3D= 0) > + return list_locations(seq, s, TRACK_ALLOC); > + else > + return list_locations(seq, s, TRACK_FREE); > + > + return 0; > +} > + > +static int slab_debug_trace_open(struct inode *inode, struct file *fil= p) > +{ > + return single_open(filp, slab_debug_trace, > + file_inode(filp)->i_private); So using single_open() makes things simple, yeah. But IMHO it's still abu= sing the seq_file interface. We do iterate over the loc_track objects so we sh= ould better use the proper iteration approach as described in Documentation/filesystems/seq_file.rst and used e.g. in fs/proc/task_mmu.c for /proc/pid/smaps etc. Right now this relies on the implementation of traverse() which starts by allocating a PAGE_SIZE buffer and if the output doesn't fit, it keeps reallocating twice the size and repeating the output, thus the call to list_location(). That's suboptimal. Also we should probably mimic the return -ENOSYS approach of the sysfs va= riant if tracking is not enabled for the cache. > +} > + > +static const struct file_operations slab_debug_fops =3D { > + .open =3D slab_debug_trace_open, > + .read =3D seq_read, > + .llseek =3D seq_lseek, > + .release =3D single_release, > +}; > + > +static void debugfs_slab_add(struct kmem_cache *s) > +{ > + const char *name; > + int unmergeable =3D slab_unmergeable(s); > + > + if (unlikely(!slab_debugfs_root)) > + return; > + > + if (!unmergeable && disable_higher_order_debug && > + (slub_debug & DEBUG_METADATA_FLAGS)) > + unmergeable =3D 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(s->slab_cache_dentry); > + name =3D s->name; > + } else { > + /* > + * Create a unique name for the slab as a target > + * for the symlinks. > + */ > + name =3D create_unique_id(s); You don't seem to be actually using the unique name? When testing your patch, I don't see the hierarchy of /sys/kernel/slab mi= rrored in /sys/kernel/debug/slab. There are no directories with the unique names= and symlinks from normal names, it seems all the caches that are aliases are = just missing completely from /sys/kernel/debug/slab > + } > + > + s->slab_cache_dentry =3D debugfs_create_dir(s->name, slab_debugfs_roo= t); > + if (!IS_ERR(s->slab_cache_dentry)) { > + debugfs_create_file("alloc_trace", 0400, > + s->slab_cache_dentry, s, &slab_debug_fops); > + > + debugfs_create_file("free_trace", 0400, > + s->slab_cache_dentry, s, &slab_debug_fops); Was it intentional to rename alloc_calls and free_calls to alloc_trace an= d free_trace? You should say in the changelog why. I would agree with the new names as Oliver is in fact working on a patch = to add actual stack traces to the file instead of just immediate callers. And yo= ur patch to move these files to debugfs to allow longer output is a required prerequisity for that. But IMHO it would be more correct to call the file= s "alloc_traces" and "free_traces". > + } > + > + if (!unmergeable) { > + /* Setup first alias */ > + debugfs_slab_alias(s, s->name); > + } > +} > + > +void debugfs_slab_release(struct kmem_cache *s) > +{ > + if (slab_state >=3D FULL) > + debugfs_remove_recursive(s->slab_cache_dentry); > +} > + > +static int __init slab_debugfs_init(void) > +{ > + struct kmem_cache *s; > + int err; > + > + slab_debugfs_root =3D debugfs_create_dir("slab", NULL); > + if (!IS_ERR(slab_debugfs_root)) { > + > + slab_state =3D 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 =3D alias_list; > + > + alias_list =3D alias_list->next; > + > + err =3D 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 > */ >=20