2015-11-10 13:33:39

by Sergey Senozhatsky

[permalink] [raw]
Subject: [PATCH 0/3] tools/vm: trivial fixes

Hello,
A set of simple tweaks to tools/vm: fix Makefile and make
gcc happier.

Sergey Senozhatsky (3):
tools/vm: fix Makefile multi-targets
tools/vm/page-types: suppress gcc warnings
tools/vm/slabinfo: update struct slabinfo members' types

tools/vm/Makefile | 8 ++++----
tools/vm/page-types.c | 5 +++--
tools/vm/slabinfo.c | 12 +++++++-----
3 files changed, 14 insertions(+), 11 deletions(-)

--
2.6.2.280.g74301d6


2015-11-10 13:33:46

by Sergey Senozhatsky

[permalink] [raw]
Subject: [PATCH 1/3] tools/vm: fix Makefile multi-targets

Build all of the $(TARGETS), not just the first one.

Without the patch (make -n)
==============================

make -C ../lib/api
gcc -Wall -Wextra -I../lib/ -o page-types page-types.c ...

Only 'page-types' target was built.

With the patch (make -n)
===========================

make -C ../lib/api
gcc -Wall -Wextra -O2 -I../lib/ -o page-types page-types.c ...
gcc -Wall -Wextra -O2 -I../lib/ -o slabinfo slabinfo.c ...
gcc -Wall -Wextra -O2 -I../lib/ -o page_owner_sort page_owner_sort.c ...

All 3 targets ('page-types', 'slabinfo', 'page_owner_sort') were
built.

Signed-off-by: Sergey Senozhatsky <[email protected]>
---
tools/vm/Makefile | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/vm/Makefile b/tools/vm/Makefile
index 93aadaf..51c3f8b 100644
--- a/tools/vm/Makefile
+++ b/tools/vm/Makefile
@@ -1,15 +1,15 @@
# Makefile for vm tools
#
-TARGETS=page-types slabinfo page_owner_sort
+TARGETS = page-types slabinfo page_owner_sort

LIB_DIR = ../lib/api
LIBS = $(LIB_DIR)/libapi.a

CC = $(CROSS_COMPILE)gcc
-CFLAGS = -Wall -Wextra -I../lib/
+CFLAGS = -Wall -Wextra -O2 -I../lib/
LDFLAGS = $(LIBS)

-$(TARGETS): $(LIBS)
+all: $(LIBS) $(TARGETS)

$(LIBS):
make -C $(LIB_DIR)
@@ -18,5 +18,5 @@ $(LIBS):
$(CC) $(CFLAGS) -o $@ $< $(LDFLAGS)

clean:
- $(RM) page-types slabinfo page_owner_sort
+ $(RM) $(TARGETS)
make -C $(LIB_DIR) clean
--
2.6.2.280.g74301d6

2015-11-10 13:33:51

by Sergey Senozhatsky

[permalink] [raw]
Subject: [PATCH 2/3] tools/vm/page-types: suppress gcc warnings

Define 'end' and 'first' as volatile to suppress
gcc warnings:

page-types.c:854:13: warning: variable 'end' might be
clobbered by 'longjmp' or 'vfork' [-Wclobbered]
page-types.c:858:6: warning: variable 'first' might be
clobbered by 'longjmp' or 'vfork' [-Wclobbered]

Signed-off-by: Sergey Senozhatsky <[email protected]>
---
tools/vm/page-types.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/vm/page-types.c b/tools/vm/page-types.c
index bcf5ec7..0651cd5 100644
--- a/tools/vm/page-types.c
+++ b/tools/vm/page-types.c
@@ -851,11 +851,12 @@ static void walk_file(const char *name, const struct stat *st)
uint8_t vec[PAGEMAP_BATCH];
uint64_t buf[PAGEMAP_BATCH], flags;
unsigned long nr_pages, pfn, i;
- off_t off, end = st->st_size;
+ off_t off;
int fd;
ssize_t len;
void *ptr;
- int first = 1;
+ volatile int first = 1;
+ volatile off_t end = st->st_size;

fd = checked_open(name, O_RDONLY|O_NOATIME|O_NOFOLLOW);

--
2.6.2.280.g74301d6

2015-11-10 13:33:55

by Sergey Senozhatsky

[permalink] [raw]
Subject: [PATCH 3/3] tools/vm/slabinfo: update struct slabinfo members' types

Align some of `struct slabinfo' members' types with
`struct kmem_cache' to suppress gcc warnings:

slabinfo.c:847:22: warning: comparison between signed
and unsigned integer expressions [-Wsign-compare]
slabinfo.c:869:20: warning: comparison between signed
and unsigned integer expressions [-Wsign-compare]
slabinfo.c:872:22: warning: comparison between signed
and unsigned integer expressions [-Wsign-compare]
slabinfo.c:894:20: warning: comparison between signed
and unsigned integer expressions [-Wsign-compare]

Signed-off-by: Sergey Senozhatsky <[email protected]>
---
tools/vm/slabinfo.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/tools/vm/slabinfo.c b/tools/vm/slabinfo.c
index 86e698d0..1813854 100644
--- a/tools/vm/slabinfo.c
+++ b/tools/vm/slabinfo.c
@@ -19,6 +19,7 @@
#include <getopt.h>
#include <regex.h>
#include <errno.h>
+#include <limits.h>

#define MAX_SLABS 500
#define MAX_ALIASES 500
@@ -28,10 +29,11 @@ struct slabinfo {
char *name;
int alias;
int refs;
- int aliases, align, cache_dma, cpu_slabs, destroy_by_rcu;
- int hwcache_align, object_size, objs_per_slab;
- int sanity_checks, slab_size, store_user, trace;
+ int aliases, cache_dma, cpu_slabs, destroy_by_rcu;
+ int sanity_checks, store_user, trace;
int order, poison, reclaim_account, red_zone;
+ unsigned int hwcache_align, align, object_size;
+ unsigned int objs_per_slab, slab_size;
unsigned long partial, objects, slabs, objects_partial, objects_total;
unsigned long alloc_fastpath, alloc_slowpath;
unsigned long free_fastpath, free_slowpath;
@@ -766,10 +768,10 @@ static void totals(void)

int used_slabs = 0;
char b1[20], b2[20], b3[20], b4[20];
- unsigned long long max = 1ULL << 63;
+ unsigned long long max = ULLONG_MAX;

/* Object size */
- unsigned long long min_objsize = max, max_objsize = 0, avg_objsize;
+ unsigned int min_objsize = UINT_MAX, max_objsize = 0, avg_objsize;

/* Number of partial slabs in a slabcache */
unsigned long long min_partial = max, max_partial = 0,
--
2.6.2.280.g74301d6

Subject: Re: [PATCH 3/3] tools/vm/slabinfo: update struct slabinfo members' types

On Tue, 10 Nov 2015, Sergey Senozhatsky wrote:

> Align some of `struct slabinfo' members' types with
> `struct kmem_cache' to suppress gcc warnings:

Acked-by: Christoph Lameter <[email protected]>

Subject: Re: [PATCH 1/3] tools/vm: fix Makefile multi-targets

On Tue, 10 Nov 2015, Sergey Senozhatsky wrote:

> Build all of the $(TARGETS), not just the first one.

Reviewed-by: Christoph Lameter <[email protected]>

2015-11-11 12:29:34

by Sergey Senozhatsky

[permalink] [raw]
Subject: Re: [PATCH 1/3] tools/vm: fix Makefile multi-targets

Hello,

On (11/10/15 17:20), David Rientjes wrote:
>>On (11/10/15 22:32), Sergey Senozhatsky wrote:
>>
>> Build all of the $(TARGETS), not just the first one.
>>
>> Without the patch (make -n)
>> ==============================
>>
>> make -C ../lib/api
>> gcc -Wall -Wextra -I../lib/ -o page-types page-types.c ...
>>
>> Only 'page-types' target was built.
>>
>> With the patch (make -n)
>> ===========================
>>
>> make -C ../lib/api
>> gcc -Wall -Wextra -O2 -I../lib/ -o page-types page-types.c ...
>> gcc -Wall -Wextra -O2 -I../lib/ -o slabinfo slabinfo.c ...
>> gcc -Wall -Wextra -O2 -I../lib/ -o page_owner_sort page_owner_sort.c ...
>>
>> All 3 targets ('page-types', 'slabinfo', 'page_owner_sort') were
>> built.
>>
>> Signed-off-by: Sergey Senozhatsky <[email protected]>
>> ---
>
> This doesn't purport to explain why -O2 was added or why it's needed for←
> these tools.

Hm, that was a sem-automatic action I saw no issues with (besides, I
considered this to be too small for a separate patch). `slabinfo' can
be used to collected extended '-X' measurements for gnuplot script
e.g.
`while [ 1 ]; do slabinfo -X >> stats; sleep 0.1s; done`

so making it a bit lighter is sort of positive change, though I'm not
married to this option.


perf stats:

-- OLD

Performance counter stats for './slabinfo.old -X -B -N 100':

197.879348 task-clock (msec) # 0.969 CPUs utilized
22 context-switches # 0.111 K/sec
0 cpu-migrations # 0.000 K/sec
2,276 page-faults # 0.012 M/sec
182,916,015 cycles # 0.924 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
259,843,733 instructions # 1.42 insns per cycle
53,949,755 branches # 272.640 M/sec
157,607 branch-misses # 0.29% of all branches

0.204190648 seconds time elapsed


-- NEW (-O2)

Performance counter stats for './slabinfo.new -X -B -N 100':

169.963546 task-clock (msec) # 0.977 CPUs utilized
9 context-switches # 0.053 K/sec
0 cpu-migrations # 0.000 K/sec
2,276 page-faults # 0.013 M/sec
153,582,826 cycles # 0.904 GHz
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
218,505,232 instructions # 1.42 insns per cycle
45,410,422 branches # 267.177 M/sec
114,126 branch-misses # 0.25% of all branches

0.173921887 seconds time elapsed


./scripts/bloat-o-meter tools/vm/slabinfo.old tools/vm/slabinfo.new
add/remove: 5/23 grow/shrink: 7/7 up/down: 6434/-9495 (-3061)
function old new delta
main 878 3699 +2821
output_slabs 307 2075 +1768
report 781 1905 +1124
rename_slabs 167 333 +166
read_slab_obj.isra - 131 +131
ops.isra - 104 +104
set_obj.isra - 102 +102
read_obj 145 236 +91
sort_slabs 414 478 +64
slab_empty.part - 32 +32
get_obj.part - 17 +17
slab_numa 542 556 +14
decode_numa_list 235 230 -5
fatal 176 160 -16
usage 17 - -17
onoff 27 - -27
show_tracking 193 162 -31
get_obj_and_str 157 124 -33
slab_size 43 - -43
slab_mismatch 47 - -47
get_obj 48 - -48
slab_waste 56 - -56
link_slabs 226 169 -57
slab_activity 60 - -60
slab_validate 63 - -63
slab_shrink 63 - -63
slab_empty 74 - -74
first_line 74 - -74
store_size 271 191 -80
xtotals 142 - -142
ops 145 - -145
set_obj 162 - -162
read_slab_obj 182 - -182
find_one_alias 187 - -187
sort_aliases 288 - -288
alias 298 - -298
debug_opt_scan 355 - -355
slab_debug 870 - -870
totals 4136 3198 -938
slabcache 1289 - -1289
read_slab_dir 1798 - -1798
slab_stats 2047 - -2047



I believe the remaining tools will not `suffer' as well.
Do you prefer to remove -O2?

-ss

2015-11-11 20:34:13

by David Rientjes

[permalink] [raw]
Subject: Re: [PATCH 1/3] tools/vm: fix Makefile multi-targets

On Wed, 11 Nov 2015, Sergey Senozhatsky wrote:

> > This doesn't purport to explain why -O2 was added or why it's needed for←
> > these tools.
>
> Hm, that was a sem-automatic action I saw no issues with (besides, I
> considered this to be too small for a separate patch). `slabinfo' can
> be used to collected extended '-X' measurements for gnuplot script
> e.g.
> `while [ 1 ]; do slabinfo -X >> stats; sleep 0.1s; done`
>
> so making it a bit lighter is sort of positive change, though I'm not
> married to this option.
>
>
> perf stats:
>
> -- OLD
>
> Performance counter stats for './slabinfo.old -X -B -N 100':
>
> 197.879348 task-clock (msec) # 0.969 CPUs utilized
> 22 context-switches # 0.111 K/sec
> 0 cpu-migrations # 0.000 K/sec
> 2,276 page-faults # 0.012 M/sec
> 182,916,015 cycles # 0.924 GHz
> <not supported> stalled-cycles-frontend
> <not supported> stalled-cycles-backend
> 259,843,733 instructions # 1.42 insns per cycle
> 53,949,755 branches # 272.640 M/sec
> 157,607 branch-misses # 0.29% of all branches
>
> 0.204190648 seconds time elapsed
>
>
> -- NEW (-O2)
>
> Performance counter stats for './slabinfo.new -X -B -N 100':
>
> 169.963546 task-clock (msec) # 0.977 CPUs utilized
> 9 context-switches # 0.053 K/sec
> 0 cpu-migrations # 0.000 K/sec
> 2,276 page-faults # 0.013 M/sec
> 153,582,826 cycles # 0.904 GHz
> <not supported> stalled-cycles-frontend
> <not supported> stalled-cycles-backend
> 218,505,232 instructions # 1.42 insns per cycle
> 45,410,422 branches # 267.177 M/sec
> 114,126 branch-misses # 0.25% of all branches
>
> 0.173921887 seconds time elapsed
>
>
> ./scripts/bloat-o-meter tools/vm/slabinfo.old tools/vm/slabinfo.new
> add/remove: 5/23 grow/shrink: 7/7 up/down: 6434/-9495 (-3061)
> function old new delta
> main 878 3699 +2821
> output_slabs 307 2075 +1768
> report 781 1905 +1124
> rename_slabs 167 333 +166
> read_slab_obj.isra - 131 +131
> ops.isra - 104 +104
> set_obj.isra - 102 +102
> read_obj 145 236 +91
> sort_slabs 414 478 +64
> slab_empty.part - 32 +32
> get_obj.part - 17 +17
> slab_numa 542 556 +14
> decode_numa_list 235 230 -5
> fatal 176 160 -16
> usage 17 - -17
> onoff 27 - -27
> show_tracking 193 162 -31
> get_obj_and_str 157 124 -33
> slab_size 43 - -43
> slab_mismatch 47 - -47
> get_obj 48 - -48
> slab_waste 56 - -56
> link_slabs 226 169 -57
> slab_activity 60 - -60
> slab_validate 63 - -63
> slab_shrink 63 - -63
> slab_empty 74 - -74
> first_line 74 - -74
> store_size 271 191 -80
> xtotals 142 - -142
> ops 145 - -145
> set_obj 162 - -162
> read_slab_obj 182 - -182
> find_one_alias 187 - -187
> sort_aliases 288 - -288
> alias 298 - -298
> debug_opt_scan 355 - -355
> slab_debug 870 - -870
> totals 4136 3198 -938
> slabcache 1289 - -1289
> read_slab_dir 1798 - -1798
> slab_stats 2047 - -2047
>
>
>
> I believe the remaining tools will not `suffer' as well.
> Do you prefer to remove -O2?
>

No, I have no objection to removing -O2. I'd prefer that the rationale be
included in the commit description, however.

Acked-by: David Rientjes <[email protected]>

2015-11-11 20:44:19

by David Rientjes

[permalink] [raw]
Subject: Re: [PATCH 2/3] tools/vm/page-types: suppress gcc warnings

On Tue, 10 Nov 2015, Sergey Senozhatsky wrote:

> Define 'end' and 'first' as volatile to suppress
> gcc warnings:
>
> page-types.c:854:13: warning: variable 'end' might be
> clobbered by 'longjmp' or 'vfork' [-Wclobbered]
> page-types.c:858:6: warning: variable 'first' might be
> clobbered by 'longjmp' or 'vfork' [-Wclobbered]
>
> Signed-off-by: Sergey Senozhatsky <[email protected]>
> ---
> tools/vm/page-types.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/tools/vm/page-types.c b/tools/vm/page-types.c
> index bcf5ec7..0651cd5 100644
> --- a/tools/vm/page-types.c
> +++ b/tools/vm/page-types.c
> @@ -851,11 +851,12 @@ static void walk_file(const char *name, const struct stat *st)
> uint8_t vec[PAGEMAP_BATCH];
> uint64_t buf[PAGEMAP_BATCH], flags;
> unsigned long nr_pages, pfn, i;
> - off_t off, end = st->st_size;
> + off_t off;
> int fd;
> ssize_t len;
> void *ptr;
> - int first = 1;
> + volatile int first = 1;
> + volatile off_t end = st->st_size;
>
> fd = checked_open(name, O_RDONLY|O_NOATIME|O_NOFOLLOW);
>

This can't possibly be correct, the warnings are legitimate and the result
of the sigsetjmp() in the function. You may be interested in
returns_twice rather than marking random automatic variables as volatile.

2015-11-11 20:55:09

by David Rientjes

[permalink] [raw]
Subject: Re: [PATCH 3/3] tools/vm/slabinfo: update struct slabinfo members' types

On Tue, 10 Nov 2015, Sergey Senozhatsky wrote:

> Align some of `struct slabinfo' members' types with
> `struct kmem_cache' to suppress gcc warnings:
>
> slabinfo.c:847:22: warning: comparison between signed
> and unsigned integer expressions [-Wsign-compare]
> slabinfo.c:869:20: warning: comparison between signed
> and unsigned integer expressions [-Wsign-compare]
> slabinfo.c:872:22: warning: comparison between signed
> and unsigned integer expressions [-Wsign-compare]
> slabinfo.c:894:20: warning: comparison between signed
> and unsigned integer expressions [-Wsign-compare]
>
> Signed-off-by: Sergey Senozhatsky <[email protected]>
> ---
> tools/vm/slabinfo.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/tools/vm/slabinfo.c b/tools/vm/slabinfo.c
> index 86e698d0..1813854 100644
> --- a/tools/vm/slabinfo.c
> +++ b/tools/vm/slabinfo.c
> @@ -19,6 +19,7 @@
> #include <getopt.h>
> #include <regex.h>
> #include <errno.h>
> +#include <limits.h>
>
> #define MAX_SLABS 500
> #define MAX_ALIASES 500
> @@ -28,10 +29,11 @@ struct slabinfo {
> char *name;
> int alias;
> int refs;
> - int aliases, align, cache_dma, cpu_slabs, destroy_by_rcu;
> - int hwcache_align, object_size, objs_per_slab;
> - int sanity_checks, slab_size, store_user, trace;
> + int aliases, cache_dma, cpu_slabs, destroy_by_rcu;
> + int sanity_checks, store_user, trace;
> int order, poison, reclaim_account, red_zone;
> + unsigned int hwcache_align, align, object_size;
> + unsigned int objs_per_slab, slab_size;
> unsigned long partial, objects, slabs, objects_partial, objects_total;
> unsigned long alloc_fastpath, alloc_slowpath;
> unsigned long free_fastpath, free_slowpath;
> @@ -766,10 +768,10 @@ static void totals(void)
>
> int used_slabs = 0;
> char b1[20], b2[20], b3[20], b4[20];
> - unsigned long long max = 1ULL << 63;
> + unsigned long long max = ULLONG_MAX;
>
> /* Object size */
> - unsigned long long min_objsize = max, max_objsize = 0, avg_objsize;
> + unsigned int min_objsize = UINT_MAX, max_objsize = 0, avg_objsize;
>
> /* Number of partial slabs in a slabcache */
> unsigned long long min_partial = max, max_partial = 0,

avg_objsize should not be unsigned int.

2015-11-12 00:54:00

by Sergey Senozhatsky

[permalink] [raw]
Subject: Re: [PATCH 2/3] tools/vm/page-types: suppress gcc warnings

On (11/11/15 12:44), David Rientjes wrote:
[..]
> This can't possibly be correct, the warnings are legitimate and the result
> of the sigsetjmp() in the function. You may be interested in
> returns_twice rather than marking random automatic variables as volatile.

Hm, ok. I saw no probs with `int first' and `end' being volatile

static void walk_file(const char *name, const struct stat *st)
{
int first = 1;

for (...) {

if (sigsetjmp(sigbus_jmp, 1)) {
goto got_sigbus;
}

got_sigbus:
...
if (first && opt_list) {
first = 0;
print_foo();
}
}
}


the `end' is also looked fine.


ANSI C

?7.13.2.1

3 All accessible objects have values, and all other components of the abstract machine 249)
have state, as of the time the longjmp function was called, except that the values of
objects of automatic storage duration that are local to the function containing the
invocation of the corresponding setjmp macro that do not have volatile-qualified type
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
and have been changed between the setjmp invocation and longjmp call are indeterminate.


Thus, adding 'volatile' should do the trick. Isn't it?
I need to google more - is returns_twice actually prevents gcc from
`over-optimizing' (there are some bug reports that state that setjmp can
be screwed up by gcc) or it's actually because the programs that do setjmp
basically violate ANSI C standard and don't volatile-qualify the affected
variables. Any hint would be helpful.

-ss

2015-11-12 00:57:27

by Sergey Senozhatsky

[permalink] [raw]
Subject: Re: [PATCH 1/3] tools/vm: fix Makefile multi-targets

On (11/11/15 12:34), David Rientjes wrote:
[..]
>
> No, I have no objection to removing -O2. I'd prefer that the rationale be
> included in the commit description, however.

yes, agree. thanks.

-ss

2015-11-12 01:12:52

by Sergey Senozhatsky

[permalink] [raw]
Subject: Re: [PATCH 3/3] tools/vm/slabinfo: update struct slabinfo members' types

On (11/11/15 12:55), David Rientjes wrote:
[..]
> > /* Object size */
> > - unsigned long long min_objsize = max, max_objsize = 0, avg_objsize;
> > + unsigned int min_objsize = UINT_MAX, max_objsize = 0, avg_objsize;
> >
> > /* Number of partial slabs in a slabcache */
> > unsigned long long min_partial = max, max_partial = 0,
>
> avg_objsize should not be unsigned int.

Hm. the assumption is that `avg_objsize' cannot be larger
than `max_objsize', which is
`int object_size;' in `struct kmem_cache' from slab_def.h
and
`unsigned int object_size;' in `struct kmem_cache' from slab.h.


avg_objsize = total_used / total_objects;


-ss

2015-11-12 05:07:09

by David Rientjes

[permalink] [raw]
Subject: Re: [PATCH 3/3] tools/vm/slabinfo: update struct slabinfo members' types

On Thu, 12 Nov 2015, Sergey Senozhatsky wrote:

> > > /* Object size */
> > > - unsigned long long min_objsize = max, max_objsize = 0, avg_objsize;
> > > + unsigned int min_objsize = UINT_MAX, max_objsize = 0, avg_objsize;
> > >
> > > /* Number of partial slabs in a slabcache */
> > > unsigned long long min_partial = max, max_partial = 0,
> >
> > avg_objsize should not be unsigned int.
>
> Hm. the assumption is that `avg_objsize' cannot be larger
> than `max_objsize', which is
> `int object_size;' in `struct kmem_cache' from slab_def.h
> and
> `unsigned int object_size;' in `struct kmem_cache' from slab.h.
>
>
> avg_objsize = total_used / total_objects;
>

total_used and total_objects are unsigned long long. This has nothing to
do with object_size in the kernel. If you need to convert max_objsize to
be unsigned long long as well, that would be better.

2015-11-12 05:46:27

by David Rientjes

[permalink] [raw]
Subject: Re: [PATCH 2/3] tools/vm/page-types: suppress gcc warnings

On Thu, 12 Nov 2015, Sergey Senozhatsky wrote:

> > This can't possibly be correct, the warnings are legitimate and the result
> > of the sigsetjmp() in the function. You may be interested in
> > returns_twice rather than marking random automatic variables as volatile.
>
> Hm, ok. I saw no probs with `int first' and `end' being volatile
>

This will only happen with the undocumented change in your first patch
which adds -O2.

I don't know what version of gcc you're using, but only "first" and "end"
being marked volatile isn't sufficient since mere code inspection would
show that "off" will also be clobbered -- it's part of the loop.

2015-11-12 06:16:06

by Sergey Senozhatsky

[permalink] [raw]
Subject: Re: [PATCH 3/3] tools/vm/slabinfo: update struct slabinfo members' types

On (11/11/15 21:07), David Rientjes wrote:
[..]
> > > > /* Object size */
> > > > - unsigned long long min_objsize = max, max_objsize = 0, avg_objsize;
> > > > + unsigned int min_objsize = UINT_MAX, max_objsize = 0, avg_objsize;
> > > >
> > > > /* Number of partial slabs in a slabcache */
> > > > unsigned long long min_partial = max, max_partial = 0,
> > >
> > > avg_objsize should not be unsigned int.
> >
> > Hm. the assumption is that `avg_objsize' cannot be larger
> > than `max_objsize', which is
> > `int object_size;' in `struct kmem_cache' from slab_def.h
> > and
> > `unsigned int object_size;' in `struct kmem_cache' from slab.h.
> >
> >
> > avg_objsize = total_used / total_objects;
> >
>

I'm not sure I clearly understand the problems you're pointing
me to.

> This has nothing to do with object_size in the kernel.

what we have in slabinfo as slab_size(), ->object_size, etc.
comming from slub's sysfs attrs:

chdir("/sys/kernel/slab")
while readdir
...
slab->object_size = get_obj("object_size");
slab->slab_size = get_obj("slab_size");
...

and attr show handlers are:

...
static ssize_t slab_size_show(struct kmem_cache *s, char *buf)
{
return sprintf(buf, "%d\n", s->size);
}
SLAB_ATTR_RO(slab_size);

static ssize_t object_size_show(struct kmem_cache *s, char *buf)
{
return sprintf(buf, "%d\n", s->object_size);
}
SLAB_ATTR_RO(object_size);
...

so those are sprintf("%d") of `struct kmem_cache'-s `int'
values.


> total_used and total_objects are unsigned long long.

yes, that's correct.
but `total_used / total_objects' cannot be larger that the size
of the largest object, which is represented in the kernel and
returned to user space as `int'. it must fit into `unsigned int'.


> If you need to convert max_objsize to be unsigned long long as
> well, that would be better.

... in case if someday `struct kmem_cache' will be updated to keep
`unsigned long' sized objects and sysfs attrs will do sprintf("%lu")?
IOW, if slabs will keep objects bigger that 4gig?

-ss

2015-11-12 09:59:42

by David Rientjes

[permalink] [raw]
Subject: Re: [PATCH 3/3] tools/vm/slabinfo: update struct slabinfo members' types

On Thu, 12 Nov 2015, Sergey Senozhatsky wrote:

> > This has nothing to do with object_size in the kernel.
>
> what we have in slabinfo as slab_size(), ->object_size, etc.
> comming from slub's sysfs attrs:
>
> chdir("/sys/kernel/slab")
> while readdir
> ...
> slab->object_size = get_obj("object_size");
> slab->slab_size = get_obj("slab_size");
> ...
>
> and attr show handlers are:
>
> ...
> static ssize_t slab_size_show(struct kmem_cache *s, char *buf)
> {
> return sprintf(buf, "%d\n", s->size);
> }
> SLAB_ATTR_RO(slab_size);
>
> static ssize_t object_size_show(struct kmem_cache *s, char *buf)
> {
> return sprintf(buf, "%d\n", s->object_size);
> }
> SLAB_ATTR_RO(object_size);
> ...
>
> so those are sprintf("%d") of `struct kmem_cache'-s `int'
> values.
>
>
> > total_used and total_objects are unsigned long long.
>
> yes, that's correct.
> but `total_used / total_objects' cannot be larger that the size
> of the largest object, which is represented in the kernel and
> returned to user space as `int'. it must fit into `unsigned int'.
>

Again, I am referring only to slabinfo as its own logical unit, it
shouldn't be based on the implementation of any slab allocator in
particular. avg_objsize has nothing to do with your patch, which is
advertised as fixing the mismatch in sign type of variables under
comparison.

There seems to be an on-going issue in this patchset that you're not
confronting: you are mixing extraneous changes into patches that are
supposed to do one thing. This already got you in trouble in the first
patch where you just threw -O2 into the Makefile randomly, and without any
mention in the commit description, and then you don't understand how to
fix the warnings that it now presents in page-types.

The warnings being shown are a result of the particular _optimization_
that your gcc version has done and your subsequent patch is only
addressing the ones that appear when you, yourself, compile. Between
different gcc versions, the optimization done by -O2 may be different and
it will warn of more or less variables that may be clobbered as a result
OF ITS OPTIMIZATION. You miss entirely that _any_ variable modified after
the setjmp() can be clobbered, most notably "off" which is the iterator
of the very loop the setjmp() appears in! Playing whack-a-mole in the
warnings you get without understanding them is the issue here.

Please, very respectfully, do not include extraneous changes into
patches, especially without mentioning them in the commit description,
when the change isn't needed or understood.