| Message ID | 20190201004242.7659-1-tobin@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/slab: Increase width of first /proc/slabinfo column | expand |
On Fri, Feb 01, 2019 at 11:42:42AM +1100, Tobin C. Harding wrote:
[snip]
This applies on top of Linus' tree
commit e74c98ca2d6a ('gfs2: Revert "Fix loop in gfs2_rbm_find"')
For this patch I doubt very much that it matters but for the record I
can't find mention in MAINTAINERS which tree to base work on for slab
patches. Are mm patches usually based of an mm tree or do you guys work
off linux-next?
thanks,
Tobin.
On Fri, 1 Feb 2019 11:58:38 +1100 "Tobin C. Harding" <me@tobin.cc> wrote: > On Fri, Feb 01, 2019 at 11:42:42AM +1100, Tobin C. Harding wrote: > [snip] > > This applies on top of Linus' tree > > commit e74c98ca2d6a ('gfs2: Revert "Fix loop in gfs2_rbm_find"') > > For this patch I doubt very much that it matters but for the record I > can't find mention in MAINTAINERS which tree to base work on for slab > patches. Are mm patches usually based of an mm tree or do you guys work > off linux-next? It's usually best to work off current mainline and I handle the integration stuff.
On Fri, 1 Feb 2019, Tobin C. Harding wrote: > Currently when displaying /proc/slabinfo if any cache names are too long > then the output columns are not aligned. We could do something fancy to > get the maximum length of any cache name in the system or we could just > increase the hardcoded width. Currently it is 17 characters. Monitors > are wide these days so lets just increase it to 30 characters. Hmm.. I wonder if there are any tools that depend on the field width here?
On Fri, Feb 01, 2019 at 11:42:42AM +1100, Tobin C. Harding wrote: > Currently when displaying /proc/slabinfo if any cache names are too long > then the output columns are not aligned. We could do something fancy to > get the maximum length of any cache name in the system or we could just > increase the hardcoded width. Currently it is 17 characters. Monitors > are wide these days so lets just increase it to 30 characters. I had a proposal some time ago to turn the slab name from being kmalloced to being an inline 16 bytes (with some fun hacks for cgroups). I think that's a better approach than permitting such long names. For example, ext4_allocation_context could be shortened to ext4_alloc_ctx without losing any expressivity. Let me know if you can't find that and I'll try to dig it up.
On Thu, Jan 31, 2019 at 05:13:06PM -0800, Andrew Morton wrote: > On Fri, 1 Feb 2019 11:58:38 +1100 "Tobin C. Harding" <me@tobin.cc> wrote: > > > On Fri, Feb 01, 2019 at 11:42:42AM +1100, Tobin C. Harding wrote: > > [snip] > > > > This applies on top of Linus' tree > > > > commit e74c98ca2d6a ('gfs2: Revert "Fix loop in gfs2_rbm_find"') > > > > For this patch I doubt very much that it matters but for the record I > > can't find mention in MAINTAINERS which tree to base work on for slab > > patches. Are mm patches usually based of an mm tree or do you guys work > > off linux-next? > > It's usually best to work off current mainline and I handle the > integration stuff. Awesome, thanks. Tobin
On Thu, Jan 31, 2019 at 06:43:10PM -0800, Matthew Wilcox wrote: > On Fri, Feb 01, 2019 at 11:42:42AM +1100, Tobin C. Harding wrote: > > Currently when displaying /proc/slabinfo if any cache names are too long > > then the output columns are not aligned. We could do something fancy to > > get the maximum length of any cache name in the system or we could just > > increase the hardcoded width. Currently it is 17 characters. Monitors > > are wide these days so lets just increase it to 30 characters. > > I had a proposal some time ago to turn the slab name from being kmalloced > to being an inline 16 bytes (with some fun hacks for cgroups). I think > that's a better approach than permitting such long names. For example, > ext4_allocation_context could be shortened to ext4_alloc_ctx without > losing any expressivity. > > Let me know if you can't find that and I'll try to dig it up. Thanks Willy, I'll try and find it and bring it back to life. Cheers, Tobin.
On Thu, 31 Jan 2019 18:43:10 -0800 Matthew Wilcox <willy@infradead.org> wrote: > On Fri, Feb 01, 2019 at 11:42:42AM +1100, Tobin C. Harding wrote: > > Currently when displaying /proc/slabinfo if any cache names are too long > > then the output columns are not aligned. We could do something fancy to > > get the maximum length of any cache name in the system or we could just > > increase the hardcoded width. Currently it is 17 characters. Monitors > > are wide these days so lets just increase it to 30 characters. > > I had a proposal some time ago to turn the slab name from being kmalloced > to being an inline 16 bytes (with some fun hacks for cgroups). I think > that's a better approach than permitting such long names. For example, > ext4_allocation_context could be shortened to ext4_alloc_ctx without > losing any expressivity. > There are some back-compatibility concerns here. And truncating long names might result in duplicates.
On Fri, 2019-02-01 at 11:42 +1100, Tobin C. Harding wrote: > Increase the width of the first column (cache name) in the output of > /proc/slabinfo from 17 to 30 characters. Do you care if this breaks any parsing of /proc/slabinfo? I don't but someone might.
Hi, On 01/02/2019 4.34, Christopher Lameter wrote: > On Fri, 1 Feb 2019, Tobin C. Harding wrote: > >> Currently when displaying /proc/slabinfo if any cache names are too long >> then the output columns are not aligned. We could do something fancy to >> get the maximum length of any cache name in the system or we could just >> increase the hardcoded width. Currently it is 17 characters. Monitors >> are wide these days so lets just increase it to 30 characters. > > Hmm.. I wonder if there are any tools that depend on the field width here? > It's possible, but it's more likely that userspace parses by whitespace because it's easier to write it that way. At least procps, which is used by slabtop, is prepared to parse a cache name of 128 characters. See the scanf() call in parse_slabinfo20() function in proc/slab.c of procps: http://procps.sourceforge.net/ Of course, testing with slabtop would make sense. - Pekka
On Fri, Feb 01, 2019 at 07:27:24PM -0800, Joe Perches wrote: > On Fri, 2019-02-01 at 11:42 +1100, Tobin C. Harding wrote: > > Increase the width of the first column (cache name) in the output of > > /proc/slabinfo from 17 to 30 characters. > > Do you care if this breaks any parsing of /proc/slabinfo? > > I don't but someone might. Thanks for looking at the patch Joe, Christoph pointed this out also. Solution is going to take a different approach and not touch the column width in /proc/slabinfo for the record, although it does not really matter now, I think that anyone parsing /proc/slabinfo would be using whitespace because the name field is already variable length. Anyways, new patch to come. thanks, Tobin.
On Thu, Jan 31, 2019 at 06:43:10PM -0800, Matthew Wilcox wrote: > On Fri, Feb 01, 2019 at 11:42:42AM +1100, Tobin C. Harding wrote: > > Currently when displaying /proc/slabinfo if any cache names are too long > > then the output columns are not aligned. We could do something fancy to > > get the maximum length of any cache name in the system or we could just > > increase the hardcoded width. Currently it is 17 characters. Monitors > > are wide these days so lets just increase it to 30 characters. > > I had a proposal some time ago to turn the slab name from being kmalloced > to being an inline 16 bytes (with some fun hacks for cgroups). I think > that's a better approach than permitting such long names. For example, > ext4_allocation_context could be shortened to ext4_alloc_ctx without > losing any expressivity. > > Let me know if you can't find that and I'll try to dig it up. Hi Willy, I haven't managed to find the patch, I grep'ed LKML using a bunch of search terms via the google group linux.kernel. Then I tried a bunch of different search strings in google prefixed with `site:kernel.org`. All to no avail. I have an idea how to fix it without making life less convenient for developers *or* for users, I know we don't discuss changes without a patch so I'll hack it up. I'm sure your solution contains things I don't understand yet (read: the cgroups bit) so I'd love to bring your patch back to life but am happy to work on another solution as well in the name of education. thanks, Tobin.
On Fri, Feb 01, 2019 at 02:03:46PM -0800, Andrew Morton wrote: > On Thu, 31 Jan 2019 18:43:10 -0800 Matthew Wilcox <willy@infradead.org> wrote: > > > On Fri, Feb 01, 2019 at 11:42:42AM +1100, Tobin C. Harding wrote: > > > Currently when displaying /proc/slabinfo if any cache names are too long > > > then the output columns are not aligned. We could do something fancy to > > > get the maximum length of any cache name in the system or we could just > > > increase the hardcoded width. Currently it is 17 characters. Monitors > > > are wide these days so lets just increase it to 30 characters. > > > > I had a proposal some time ago to turn the slab name from being kmalloced > > to being an inline 16 bytes (with some fun hacks for cgroups). I think > > that's a better approach than permitting such long names. For example, > > ext4_allocation_context could be shortened to ext4_alloc_ctx without > > losing any expressivity. > > > > There are some back-compatibility concerns here. I'm don't understand sorry what back-compatibility concerns (please see sentiment at end of email :) > And truncating long names might result in duplicates. So I thought I had a good idea - add a pr_warn() if cache name > 16 and patch all current intree calls to kmem_cache_create() called as such. This process very kindly lead me to the fact that this does *not* work because of the macro KMEM_CACHE (which uses the struct name as the cache name). So, back to the drawing board. I'm concerned that this may be a waste of peoples time, if so please say so and I'll move on to something else. thanks, Tobin.
diff --git a/mm/slab_common.c b/mm/slab_common.c index 81732d05e74a..a339f1361164 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -1365,7 +1365,7 @@ static void cache_show(struct kmem_cache *s, struct seq_file *m) memcg_accumulate_slabinfo(s, &sinfo); - seq_printf(m, "%-17s %6lu %6lu %6u %4u %4d", + seq_printf(m, "%-30s %6lu %6lu %6u %4u %4d", cache_name(s), sinfo.active_objs, sinfo.num_objs, s->size, sinfo.objects_per_slab, (1 << sinfo.cache_order));
Currently when displaying /proc/slabinfo if any cache names are too long then the output columns are not aligned. We could do something fancy to get the maximum length of any cache name in the system or we could just increase the hardcoded width. Currently it is 17 characters. Monitors are wide these days so lets just increase it to 30 characters. On one running kernel, with this choice of width, the increase is sufficient to align the columns and total line width is increased from 112 to 119 characters (excluding the heading row). Admittedly there may be cache names in the wild which are longer than the cache names on this machine, in which case the columns would still be unaligned. Increase the width of the first column (cache name) in the output of /proc/slabinfo from 17 to 30 characters. Signed-off-by: Tobin C. Harding <tobin@kernel.org> --- This patch does not touch the heading row, and discussion of column width excludes this row. Please note that the second column labeled by the heading row is now *not* above the second column. ### Before patch is applied sample output of `cat /proc/slabinfo` (max column width == 112): slabinfo - version: 2.1 # name <active_objs> <num_objs> <objsize> <objperslab> <pagesperslab> : tunables <limit> <batchcount> <sharedfactor> : slabdata <active_slabs> <num_slabs> <sharedavail> kcopyd_job 0 0 3312 9 8 : tunables 0 0 0 : slabdata 0 0 0 dm_uevent 0 0 2632 12 8 : tunables 0 0 0 : slabdata 0 0 0 fuse_request 60 60 392 20 2 : tunables 0 0 0 : slabdata 3 3 0 fuse_inode 21 21 768 21 4 : tunables 0 0 0 : slabdata 1 1 0 kvm_async_pf 90 90 136 30 1 : tunables 0 0 0 : slabdata 3 3 0 kvm_vcpu 4 4 24192 1 8 : tunables 0 0 0 : slabdata 4 4 0 kvm_mmu_page_header 100 150 160 25 1 : tunables 0 0 0 : slabdata 6 6 0 i915_request 100 100 640 25 4 : tunables 0 0 0 : slabdata 4 4 0 i915_vma 316 336 576 28 4 : tunables 0 0 0 : slabdata 12 12 0 fat_inode_cache 22 22 728 22 4 : tunables 0 0 0 : slabdata 1 1 0 fat_cache 0 0 40 102 1 : tunables 0 0 0 : slabdata 0 0 0 ext4_groupinfo_4k 3780 3780 144 28 1 : tunables 0 0 0 : slabdata 135 135 0 ext4_inode_cache 255633 258480 1080 30 8 : tunables 0 0 0 : slabdata 8616 8616 0 ext4_allocation_context 128 128 128 32 1 : tunables 0 0 0 : slabdata 4 4 0 ext4_io_end 256 256 64 64 1 : tunables 0 0 0 : slabdata 4 4 0 ext4_extent_status 197111 197778 40 102 1 : tunables 0 0 0 : slabdata 1939 1939 0 mbcache 294 584 56 73 1 : tunables 0 0 0 : slabdata 8 8 0 jbd2_journal_head 364 476 120 34 1 : tunables 0 0 0 : slabdata 14 14 0 jbd2_revoke_table_s 512 512 16 256 1 : tunables 0 0 0 : slabdata 2 2 0 fscrypt_info 512 1024 32 128 1 : tunables 0 0 0 : slabdata 8 8 0 ... ### With patch applied output of `cat /proc/slabinfo` (max column width == 119): slabinfo - version: 2.1 # name <active_objs> <num_objs> <objsize> <objperslab> <pagesperslab> : tunables <limit> <batchcount> <sharedfactor> : slabdata <active_slabs> <num_slabs> <share> PINGv6 0 0 1152 14 4 : tunables 0 0 0 : slabdata 0 0 0 RAWv6 14 14 1152 14 4 : tunables 0 0 0 : slabdata 1 1 0 UDPv6 0 0 1280 12 4 : tunables 0 0 0 : slabdata 0 0 0 tw_sock_TCPv6 0 0 240 17 1 : tunables 0 0 0 : slabdata 0 0 0 request_sock_TCPv6 0 0 304 13 1 : tunables 0 0 0 : slabdata 0 0 0 TCPv6 0 0 2304 14 8 : tunables 0 0 0 : slabdata 0 0 0 sgpool-128 8 8 4096 8 8 : tunables 0 0 0 : slabdata 1 1 0 bfq_io_cq 0 0 160 25 1 : tunables 0 0 0 : slabdata 0 0 0 bfq_queue 0 0 464 17 2 : tunables 0 0 0 : slabdata 0 0 0 mqueue_inode_cache 9 9 896 9 2 : tunables 0 0 0 : slabdata 1 1 0 dnotify_struct 0 0 32 128 1 : tunables 0 0 0 : slabdata 0 0 0 posix_timers_cache 0 0 240 17 1 : tunables 0 0 0 : slabdata 0 0 0 UNIX 0 0 1024 8 2 : tunables 0 0 0 : slabdata 0 0 0 ip4-frags 0 0 208 19 1 : tunables 0 0 0 : slabdata 0 0 0 tcp_bind_bucket 0 0 128 32 1 : tunables 0 0 0 : slabdata 0 0 0 PING 0 0 960 8 2 : tunables 0 0 0 : slabdata 0 0 0 RAW 8 8 960 8 2 : tunables 0 0 0 : slabdata 1 1 0 tw_sock_TCP 0 0 240 17 1 : tunables 0 0 0 : slabdata 0 0 0 request_sock_TCP 0 0 304 13 1 : tunables 0 0 0 : slabdata 0 0 0 TCP 0 0 2176 15 8 : tunables 0 0 0 : slabdata 0 0 0 hugetlbfs_inode_cache 13 13 616 13 2 : tunables 0 0 0 : slabdata 1 1 0 dquot 0 0 256 16 1 : tunables 0 0 0 : slabdata 0 0 0 eventpoll_pwq 0 0 72 56 1 : tunables 0 0 0 : slabdata 0 0 0 dax_cache 10 10 768 10 2 : tunables 0 0 0 : slabdata 1 1 0 request_queue 0 0 2056 15 8 : tunables 0 0 0 : slabdata 0 0 0 biovec-max 8 8 8192 4 8 : tunables 0 0 0 : slabdata 2 2 0 biovec-128 8 8 2048 8 4 : tunables 0 0 0 : slabdata 1 1 0 biovec-64 8 8 1024 8 2 : tunables 0 0 0 : slabdata 1 1 0 user_namespace 8 8 512 8 1 : tunables 0 0 0 : slabdata 1 1 0 uid_cache 21 21 192 21 1 : tunables 0 0 0 : slabdata 1 1 0 dmaengine-unmap-2 64 64 64 64 1 : tunables 0 0 0 : slabdata 1 1 0 sock_inode_cache 24 24 640 12 2 : tunables 0 0 0 : slabdata 2 2 0 skbuff_fclone_cache 0 0 448 9 1 : tunables 0 0 0 : slabdata 0 0 0 skbuff_head_cache 16 16 256 16 1 : tunables 0 0 0 : slabdata 1 1 0 file_lock_cache 0 0 216 18 1 : tunables 0 0 0 : slabdata 0 0 0 net_namespace 0 0 3392 9 8 : tunables 0 0 0 : slabdata 0 0 0 mm/slab_common.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)