mm/slub.c: remove 'per_cpu' which is useless variable

Remove 'per_cpu', since it is useless now after the patch: "205ab99
slub: Update statistics handling for variable order slabs".

Also beautify code with tab alignment.

Signed-off-by Chen Gang <[email protected]>

---
  mm/slub.c |   17 ++++++-----------
  1 files changed, 6 insertions(+), 11 deletions(-)

mm/slub.c +1 -9

index 2caaa67..aa847eb 100644
@@ -4271,12 +4271,10 @@ static ssize_t show_slab_objects(struct kmem_cache *s,
  	int node;
  	int x;
  	unsigned long *nodes;
-	unsigned long *per_cpu;
 
-	nodes = kzalloc(2 * sizeof(unsigned long) * nr_node_ids, GFP_KERNEL);
+	nodes = kzalloc(sizeof(unsigned long) * nr_node_ids, GFP_KERNEL);
  	if (!nodes)
  		return -ENOMEM;
-	per_cpu = nodes + nr_node_ids;
 
  	if (flags & SO_CPU) {
  		int cpu;

@@ -4307,8 +4305,6 @@ static ssize_t show_slab_objects(struct kmem_cache *s,
  				total += x;
  				nodes[node] += x;
  			}
-
-			per_cpu[node]++;
  		}
  	}
 

@@ -4318,12 +4314,11 @@ static ssize_t show_slab_objects(struct kmem_cache *s,
  		for_each_node_state(node, N_NORMAL_MEMORY) {
  			struct kmem_cache_node *n = get_node(s, node);
 
-		if (flags & SO_TOTAL)
-			x = atomic_long_read(&n->total_objects);
-		else if (flags & SO_OBJECTS)
-			x = atomic_long_read(&n->total_objects) -

Hi,

Quoted 2 lines · Chen Gang

Remove 'per_cpu', since it is useless now after the patch: "205ab99
slub: Update statistics handling for variable order slabs".

Whoa, that's a really old commit. Christoph?

Quoted 1 line

Also beautify code with tab alignment.

That needs to be a separate patch.

Quoted 7 lines · Pekka Enberg

Hi,

Quoted 2 lines · Chen Gang

Remove 'per_cpu', since it is useless now after the patch: "205ab99
slub: Update statistics handling for variable order slabs".

Whoa, that's a really old commit. Christoph?

Quoted 3 lines · Chen Gang

Quoted 2 lines · Pekka Enberg

Hi,

Quoted 5 lines

Quoted 5 lines · Chen Gang

Quoted 2 lines

Remove 'per_cpu', since it is useless now after the patch: "205ab99
slub: Update statistics handling for variable order slabs".

Whoa, that's a really old commit. Christoph?

Gosh we have some code duplication there. Patch mismerged?

Quoted 1 line

Gosh we have some code duplication there. Patch mismerged?

What duplication are you referring to?

Quoted 3 lines · Pekka Enberg

Quoted 1 line

Gosh we have some code duplication there. Patch mismerged?

What duplication are you referring to?

Sorry, no duplicate. The partial list is handled in the same way as the per cpu slab. Yep its ok to remove. Chen: Clean up your patches and then we can merge them.

Since alloc_loc_track() will alloc additional space, and already knows about 'max', so need be sure of 'max' must be larger than 't->count'.

The caller may not notice about it, e.g. call from add_location() in "mm/slub.c", which only let "max = 2 * max" when "t->count >= t->max"

Quoted 2 lines · Chen Gang

Since alloc_loc_track() will alloc additional space, and already knows about 'max', so need be sure of 'max' must be larger than 't->count'.

alloc_loc_track is only called if t->count > max from add_location:

/*
* Not found. Insert new tracking element.
*/
if (t->count >= t->max && !alloc_loc_track(t, 2 * t->max, GFP_ATOMIC))

                return 0;

Quoted 5 lines · Christoph Lameter

Quoted 2 lines · Chen Gang

Since alloc_loc_track() will alloc additional space, and already knows about 'max', so need be sure of 'max' must be larger than 't->count'.

Quoted 2 lines

alloc_loc_track is only called if t->count > max from add_location:

For add_location(), if "t->count > t->max", it calls alloc_loc_track() with "max == 2 * t->max".

In this case we need be sure that "t->count < 2 * t->max".

Quoted 12 lines · Chen Gang

Quoted 12 lines · Christoph Lameter

Quoted 7 lines

Quoted 7 lines · Chen Gang

Quoted 3 lines

Quoted 3 lines

Since alloc_loc_track() will alloc additional space, and already knows about 'max', so need be sure of 'max' must be larger than 't->count'.

alloc_loc_track is only called if t->count > max from add_location:

For add_location(), if "t->count > t->max", it calls alloc_loc_track() with "max == 2 * t->max".

Quoted 1 line

Quoted 1 line

In this case we need be sure that "t->count < 2 * t->max".

We are sure about that since t->count is always incremented by one and then checked against t->max. The location database is build up from a single hardware thread without any concurrency.

So we do not really need this patch.

Hmm... what you says above is reasonable.

In this case, since alloc_loc_track() is a static function, it will depend on the related maintainers' willing and opinions to decide whether add the related check or not (just like add 'BUG_ON' or not).

I need respect the original related maintainers' willing and opinions.

Quoted 2 lines · Chen Gang

Hmm... what you says above is reasonable.

Quoted 5 lines

In this case, since alloc_loc_track() is a static function, it will depend on the related maintainers' willing and opinions to decide whether add the related check or not (just like add 'BUG_ON' or not).

I need respect the original related maintainers' willing and opinions.

You are talking to the author of the code.

Hmm... when anybody says "need respect original authors' willing and opinions", I think it often means we have found the direct issue, but none of us find the root issue.

e.g. for our this case:
the direct issue is:
"whether need check the length with 'max' parameter".
but maybe the root issue is:
"whether use 'size' as related parameter name instead of 'max'". in alloc_loc_track(), 'max' just plays the 'size' role.

Quoted 3 lines · Chen Gang

Quoted 3 lines

Hmm... when anybody says "need respect original authors' willing and opinions", I think it often means we have found the direct issue, but none of us find the root issue.

Is there an actual problem / failure being addressed by this patch?

Quoted 6 lines

Quoted 6 lines

e.g. for our this case:
the direct issue is:
"whether need check the length with 'max' parameter".
but maybe the root issue is:
"whether use 'size' as related parameter name instead of 'max'". in alloc_loc_track(), 'max' just plays the 'size' role.

"max" determines the size of the loc_track structure. So these can roughly mean the same thing.

Yes, "'max' can roughly mean the same thing", but they are still a little different.

'max' also means: "the caller tells callee: I have told you the maximize buffer length, so I need not check the buffer length to be sure of no memory overflow, you need be sure of it".

'size' means: "the caller tells callee: you should use the size which I give you, I am sure it is OK, do not care about whether it can cause memory overflow or not".

The diff may like this:

Quoted 3 lines · Chen Gang F T

Quoted 3 lines

Yes, "'max' can roughly mean the same thing", but they are still a little different.

Quoted 7 lines

Quoted 7 lines

'max' also means: "the caller tells callee: I have told you the maximize buffer length, so I need not check the buffer length to be sure of no memory overflow, you need be sure of it".

'size' means: "the caller tells callee: you should use the size which I give you, I am sure it is OK, do not care about whether it can cause memory overflow or not".

Ok that makes sense.

Quoted 1 line

Quoted 1 line

The diff may like this:

I am fine with such a patch.

Ultimately I would like the tracking and debugging technology to be abstracted from the slub allocator and made generally useful by putting it into mm/slab_common.c. SLAB has similar things but does not have all the features.

Quoted 10 lines · Chen Gang F T

Yes, "'max' can roughly mean the same thing", but they are still a little different.

'max' also means: "the caller tells callee: I have told you the maximize buffer length, so I need not check the buffer length to be sure of no memory overflow, you need be sure of it".

'size' means: "the caller tells callee: you should use the size which I give you, I am sure it is OK, do not care about whether it can cause memory overflow or not".

Ok that makes sense.

Quoted 1 line

The diff may like this:

I am fine with such a patch.

Ultimately I would like the tracking and debugging technology to be abstracted from the slub allocator and made generally useful by putting it into mm/slab_common.c. SLAB has similar things but does not have all the features.

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

Quoted 19 lines · Christoph Lameter

Quoted 10 lines · Chen Gang F T

Yes, "'max' can roughly mean the same thing", but they are still a little different.

'max' also means: "the caller tells callee: I have told you the maximize buffer length, so I need not check the buffer length to be sure of no memory overflow, you need be sure of it".

'size' means: "the caller tells callee: you should use the size which I give you, I am sure it is OK, do not care about whether it can cause memory overflow or not".

Ok that makes sense.

Quoted 1 line

The diff may like this:

I am fine with such a patch.

Quoted 4 lines

Ultimately I would like the tracking and debugging technology to be abstracted from the slub allocator and made generally useful by putting it into mm/slab_common.c. SLAB has similar things but does not have all the features.

Coincidence, I am doing this work recently and will post patches soon. ;-)

Regards,
Wanpeng Li

Quoted 6 lines

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

Quoted 10 lines · Chen Gang F T

Yes, "'max' can roughly mean the same thing", but they are still a little different.

'max' also means: "the caller tells callee: I have told you the maximize buffer length, so I need not check the buffer length to be sure of no memory overflow, you need be sure of it".

'size' means: "the caller tells callee: you should use the size which I give you, I am sure it is OK, do not care about whether it can cause memory overflow or not".

Ok that makes sense.

Quoted 1 line

The diff may like this:

I am fine with such a patch.

Ultimately I would like the tracking and debugging technology to be abstracted from the slub allocator and made generally useful by putting it into mm/slab_common.c. SLAB has similar things but does not have all the features.

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

Remove 'per_cpu', since it is useless now after the patch: "205ab99
slub: Update statistics handling for variable order slabs". And the partial list is handled in the same way as the per cpu slab.

Quoted 3 lines · Chen Gang

Remove 'per_cpu', since it is useless now after the patch: "205ab99
slub: Update statistics handling for variable order slabs". And the partial list is handled in the same way as the per cpu slab.

Acked-by Christoph Lameter <[email protected]>