[PATCH 0/3] tools/vm: trivial fixes

[PATCH 0/3] tools/vm: trivial fixes

[Sergey Senozhatsky]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Sergey Senozhatsky]

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 <sergey.senozhatsky@gmail.com>
---
 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Christoph Lameter]

On Tue, 10 Nov 2015, Sergey Senozhatsky wrote:

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

Reviewed-by: Christoph Lameter <cl@linux.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Sergey Senozhatsky]

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 <sergey.senozhatsky@gmail.com>
>> ---
>
> This doesn't purport to explain why -O2 was added or why it's needed fora??
> 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[David Rientjes]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 5703 bytes --]

On Wed, 11 Nov 2015, Sergey Senozhatsky wrote:

> > This doesn't purport to explain why -O2 was added or why it's needed fora??
> > 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 <rientjes@google.com>

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

[Sergey Senozhatsky]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Sergey Senozhatsky]

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 <sergey.senozhatsky@gmail.com>
---
 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[David Rientjes]

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 <sergey.senozhatsky@gmail.com>
> ---
>  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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Sergey Senozhatsky]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[David Rientjes]

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Sergey Senozhatsky]

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 <sergey.senozhatsky@gmail.com>
---
 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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Christoph Lameter]

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 <cl@linux.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[David Rientjes]

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 <sergey.senozhatsky@gmail.com>
> ---
>  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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Sergey Senozhatsky]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[David Rientjes]

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[Sergey Senozhatsky]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

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

[David Rientjes]

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.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>