linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm/slub.c: remove 'per_cpu' which is useless variable
@ 2013-07-08  8:07 Chen Gang
  2013-07-11  6:45 ` Pekka Enberg
  0 siblings, 1 reply; 24+ messages in thread
From: Chen Gang @ 2013-07-08  8:07 UTC (permalink / raw)
  To: cl, penberg, mpm; +Cc: linux-mm, Andrew Morton

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 <gang.chen@asianux.com>
---
 mm/slub.c |   17 ++++++-----------
 1 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 2caaa67..aa847eb 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -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) -
-				count_partial(n, count_free);
-
+			if (flags & SO_TOTAL)
+				x = atomic_long_read(&n->total_objects);
+			else if (flags & SO_OBJECTS)
+				x = atomic_long_read(&n->total_objects) -
+					count_partial(n, count_free);
 			else
 				x = atomic_long_read(&n->nr_slabs);
 			total += x;
-- 
1.7.7.6

--
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>

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH] mm/slub.c: remove 'per_cpu' which is useless variable
  2013-07-08  8:07 [PATCH] mm/slub.c: remove 'per_cpu' which is useless variable Chen Gang
@ 2013-07-11  6:45 ` Pekka Enberg
  2013-07-11  6:50   ` Chen Gang
  0 siblings, 1 reply; 24+ messages in thread
From: Pekka Enberg @ 2013-07-11  6:45 UTC (permalink / raw)
  To: Chen Gang; +Cc: cl, mpm, linux-mm, Andrew Morton

Hi,

On 07/08/2013 11:07 AM, Chen Gang wrote:
> 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?

> Also beautify code with tab alignment.

That needs to be a separate patch.

>
> Signed-off-by: Chen Gang <gang.chen@asianux.com>
> ---
>   mm/slub.c |   17 ++++++-----------
>   1 files changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/mm/slub.c b/mm/slub.c
> index 2caaa67..aa847eb 100644
> --- a/mm/slub.c
> +++ b/mm/slub.c
> @@ -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) -
> -				count_partial(n, count_free);
> -
> +			if (flags & SO_TOTAL)
> +				x = atomic_long_read(&n->total_objects);
> +			else if (flags & SO_OBJECTS)
> +				x = atomic_long_read(&n->total_objects) -
> +					count_partial(n, count_free);
>   			else
>   				x = atomic_long_read(&n->nr_slabs);
>   			total += x;
>

--
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>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] mm/slub.c: remove 'per_cpu' which is useless variable
  2013-07-11  6:45 ` Pekka Enberg
@ 2013-07-11  6:50   ` Chen Gang
  2013-07-11 16:45     ` Christoph Lameter
  0 siblings, 1 reply; 24+ messages in thread
From: Chen Gang @ 2013-07-11  6:50 UTC (permalink / raw)
  To: Pekka Enberg; +Cc: cl, mpm, linux-mm, Andrew Morton

On 07/11/2013 02:45 PM, Pekka Enberg wrote:
> Hi,
> 
> On 07/08/2013 11:07 AM, Chen Gang wrote:
>> 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?
> 

Hmm..., waiting discussing result...  :-)

>> Also beautify code with tab alignment.
> 
> That needs to be a separate patch.

OK, I should send it after get result from above discussion.


Thanks.
-- 
Chen Gang

--
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>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] mm/slub.c: remove 'per_cpu' which is useless variable
  2013-07-11  6:50   ` Chen Gang
@ 2013-07-11 16:45     ` Christoph Lameter
  2013-07-11 18:28       ` Pekka Enberg
  0 siblings, 1 reply; 24+ messages in thread
From: Christoph Lameter @ 2013-07-11 16:45 UTC (permalink / raw)
  To: Chen Gang; +Cc: Pekka Enberg, mpm, linux-mm, Andrew Morton

On Thu, 11 Jul 2013, Chen Gang wrote:

> On 07/11/2013 02:45 PM, Pekka Enberg wrote:
> > Hi,
> >
> > On 07/08/2013 11:07 AM, Chen Gang wrote:
> >> 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?

--
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>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] mm/slub.c: remove 'per_cpu' which is useless variable
  2013-07-11 16:45     ` Christoph Lameter
@ 2013-07-11 18:28       ` Pekka Enberg
  2013-07-11 20:16         ` Christoph Lameter
  0 siblings, 1 reply; 24+ messages in thread
From: Pekka Enberg @ 2013-07-11 18:28 UTC (permalink / raw)
  To: Christoph Lameter; +Cc: Chen Gang, mpm, linux-mm, Andrew Morton

On 7/11/13 7:45 PM, Christoph Lameter wrote:
> On Thu, 11 Jul 2013, Chen Gang wrote:
>
>> On 07/11/2013 02:45 PM, Pekka Enberg wrote:
>>> Hi,
>>>
>>> On 07/08/2013 11:07 AM, Chen Gang wrote:
>>>> 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?

What duplication are you referring to?

			Pekka

--
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>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] mm/slub.c: remove 'per_cpu' which is useless variable
  2013-07-11 18:28       ` Pekka Enberg
@ 2013-07-11 20:16         ` Christoph Lameter
  2013-07-11 23:52           ` Chen Gang
  0 siblings, 1 reply; 24+ messages in thread
From: Christoph Lameter @ 2013-07-11 20:16 UTC (permalink / raw)
  To: Pekka Enberg; +Cc: Chen Gang, mpm, linux-mm, Andrew Morton

On Thu, 11 Jul 2013, Pekka Enberg wrote:

> > 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.




--
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>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] mm/slub.c: remove 'per_cpu' which is useless variable
  2013-07-11 20:16         ` Christoph Lameter
@ 2013-07-11 23:52           ` Chen Gang
  2013-07-12  0:23             ` [PATCH v2] " Chen Gang
  0 siblings, 1 reply; 24+ messages in thread
From: Chen Gang @ 2013-07-11 23:52 UTC (permalink / raw)
  To: Christoph Lameter; +Cc: Pekka Enberg, mpm, linux-mm, Andrew Morton

On 07/12/2013 04:16 AM, Christoph Lameter wrote:
> On Thu, 11 Jul 2013, Pekka Enberg wrote:
> 
>>> 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.
> 

OK, thanks. I should send patch v2 for it.

Thanks.
-- 
Chen Gang

--
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>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2] mm/slub.c: remove 'per_cpu' which is useless variable
  2013-07-11 23:52           ` Chen Gang
@ 2013-07-12  0:23             ` Chen Gang
  2013-07-12  0:55               ` [PATCH] mm/slub.c: add parameter length checking for alloc_loc_track() Chen Gang
  2013-07-12 13:45               ` [PATCH v2] mm/slub.c: remove 'per_cpu' which is useless variable Christoph Lameter
  0 siblings, 2 replies; 24+ messages in thread
From: Chen Gang @ 2013-07-12  0:23 UTC (permalink / raw)
  To: Christoph Lameter; +Cc: Pekka Enberg, mpm, linux-mm, Andrew Morton

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.

Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 mm/slub.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 2b02d66..c94a03f 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -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]++;
 		}
 	}
 
-- 
1.7.7.6

--
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>

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH] mm/slub.c: add parameter length checking for alloc_loc_track()
  2013-07-12  0:23             ` [PATCH v2] " Chen Gang
@ 2013-07-12  0:55               ` Chen Gang
  2013-07-12 13:49                 ` Christoph Lameter
  2013-07-12 13:45               ` [PATCH v2] mm/slub.c: remove 'per_cpu' which is useless variable Christoph Lameter
  1 sibling, 1 reply; 24+ messages in thread
From: Chen Gang @ 2013-07-12  0:55 UTC (permalink / raw)
  To: Christoph Lameter; +Cc: Pekka Enberg, mpm, linux-mm, Andrew Morton

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"


Signed-off-by: Chen Gang <gang.chen@asianux.com>
---
 mm/slub.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 2b02d66..36f606d 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3993,6 +3993,9 @@ static int alloc_loc_track(struct loc_track *t, unsigned long max, gfp_t flags)
 	struct location *l;
 	int order;
 
+	if (t->count >= max)
+		return 0;
+
 	order = get_order(sizeof(struct location) * max);
 
 	l = (void *)__get_free_pages(flags, order);
-- 
1.7.7.6

--
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>

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] mm/slub.c: remove 'per_cpu' which is useless variable
  2013-07-12  0:23             ` [PATCH v2] " Chen Gang
  2013-07-12  0:55               ` [PATCH] mm/slub.c: add parameter length checking for alloc_loc_track() Chen Gang
@ 2013-07-12 13:45               ` Christoph Lameter
  2013-07-15  0:08                 ` Chen Gang
  1 sibling, 1 reply; 24+ messages in thread
From: Christoph Lameter @ 2013-07-12 13:45 UTC (permalink / raw)
  To: Chen Gang; +Cc: Pekka Enberg, mpm, linux-mm, Andrew Morton

On Fri, 12 Jul 2013, Chen Gang wrote:

> 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 <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>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] mm/slub.c: add parameter length checking for alloc_loc_track()
  2013-07-12  0:55               ` [PATCH] mm/slub.c: add parameter length checking for alloc_loc_track() Chen Gang
@ 2013-07-12 13:49                 ` Christoph Lameter
  2013-07-15  0:17                   ` Chen Gang
  0 siblings, 1 reply; 24+ messages in thread
From: Christoph Lameter @ 2013-07-12 13:49 UTC (permalink / raw)
  To: Chen Gang; +Cc: Pekka Enberg, mpm, linux-mm, Andrew Morton

On Fri, 12 Jul 2013, Chen Gang wrote:

> 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;




> 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"

That call already has the condition checked before the call. The only
other caller is list_locations which calls alloc_loc_track when t->count == 0

--
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>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2] mm/slub.c: remove 'per_cpu' which is useless variable
  2013-07-12 13:45               ` [PATCH v2] mm/slub.c: remove 'per_cpu' which is useless variable Christoph Lameter
@ 2013-07-15  0:08                 ` Chen Gang
  0 siblings, 0 replies; 24+ messages in thread
From: Chen Gang @ 2013-07-15  0:08 UTC (permalink / raw)
  To: Christoph Lameter; +Cc: Pekka Enberg, mpm, linux-mm, Andrew Morton

On 07/12/2013 09:45 PM, Christoph Lameter wrote:
> On Fri, 12 Jul 2013, Chen Gang wrote:
> 
>> 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 <cl@linux.com>
> 
> 

Thanks.

-- 
Chen Gang

--
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>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] mm/slub.c: add parameter length checking for alloc_loc_track()
  2013-07-12 13:49                 ` Christoph Lameter
@ 2013-07-15  0:17                   ` Chen Gang
  2013-07-15 15:16                     ` Christoph Lameter
  0 siblings, 1 reply; 24+ messages in thread
From: Chen Gang @ 2013-07-15  0:17 UTC (permalink / raw)
  To: Christoph Lameter; +Cc: Pekka Enberg, mpm, linux-mm, Andrew Morton

On 07/12/2013 09:49 PM, Christoph Lameter wrote:
> On Fri, 12 Jul 2013, Chen Gang wrote:
> 
>> 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".

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

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


Thanks.
-- 
Chen Gang

--
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>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] mm/slub.c: add parameter length checking for alloc_loc_track()
  2013-07-15  0:17                   ` Chen Gang
@ 2013-07-15 15:16                     ` Christoph Lameter
  2013-07-16  1:03                       ` Chen Gang
  0 siblings, 1 reply; 24+ messages in thread
From: Christoph Lameter @ 2013-07-15 15:16 UTC (permalink / raw)
  To: Chen Gang; +Cc: Pekka Enberg, mpm, linux-mm, Andrew Morton

On Mon, 15 Jul 2013, Chen Gang wrote:

> On 07/12/2013 09:49 PM, Christoph Lameter wrote:
> > On Fri, 12 Jul 2013, Chen Gang wrote:
> >
> >> 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".
>
> 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.

--
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>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] mm/slub.c: add parameter length checking for alloc_loc_track()
  2013-07-15 15:16                     ` Christoph Lameter
@ 2013-07-16  1:03                       ` Chen Gang
  2013-07-17 15:03                         ` Christoph Lameter
  0 siblings, 1 reply; 24+ messages in thread
From: Chen Gang @ 2013-07-16  1:03 UTC (permalink / raw)
  To: Christoph Lameter; +Cc: Pekka Enberg, mpm, linux-mm, Andrew Morton

On 07/15/2013 11:16 PM, Christoph Lameter wrote:
> On Mon, 15 Jul 2013, Chen Gang wrote:
> 
>> > On 07/12/2013 09:49 PM, Christoph Lameter wrote:
>>> > > On Fri, 12 Jul 2013, Chen Gang wrote:
>>> > >
>>>> > >> 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".
>> >
>> > 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.


Thanks.
-- 
Chen Gang

--
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>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] mm/slub.c: add parameter length checking for alloc_loc_track()
  2013-07-16  1:03                       ` Chen Gang
@ 2013-07-17 15:03                         ` Christoph Lameter
  2013-07-18  0:43                           ` Chen Gang
  0 siblings, 1 reply; 24+ messages in thread
From: Christoph Lameter @ 2013-07-17 15:03 UTC (permalink / raw)
  To: Chen Gang; +Cc: Pekka Enberg, mpm, linux-mm, Andrew Morton

On Tue, 16 Jul 2013, Chen Gang wrote:

> 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.

You are talking to the author of the code.

--
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>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] mm/slub.c: add parameter length checking for alloc_loc_track()
  2013-07-17 15:03                         ` Christoph Lameter
@ 2013-07-18  0:43                           ` Chen Gang
  2013-07-18 13:45                             ` Christoph Lameter
  0 siblings, 1 reply; 24+ messages in thread
From: Chen Gang @ 2013-07-18  0:43 UTC (permalink / raw)
  To: Christoph Lameter; +Cc: Pekka Enberg, mpm, linux-mm, Andrew Morton

On 07/17/2013 11:03 PM, Christoph Lameter wrote:
> On Tue, 16 Jul 2013, Chen Gang wrote:
> 
>> 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.
> 
> You are talking to the author of the code.
> 

Firstly, at least, my this patch is really useless.

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.


Thanks.
-- 
Chen Gang

--
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>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] mm/slub.c: add parameter length checking for alloc_loc_track()
  2013-07-18  0:43                           ` Chen Gang
@ 2013-07-18 13:45                             ` Christoph Lameter
  2013-07-19  0:05                               ` Chen Gang F T
  0 siblings, 1 reply; 24+ messages in thread
From: Christoph Lameter @ 2013-07-18 13:45 UTC (permalink / raw)
  To: Chen Gang; +Cc: Pekka Enberg, mpm, linux-mm, Andrew Morton

On Thu, 18 Jul 2013, Chen Gang wrote:

> 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?

> 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.

--
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>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] mm/slub.c: add parameter length checking for alloc_loc_track()
  2013-07-18 13:45                             ` Christoph Lameter
@ 2013-07-19  0:05                               ` Chen Gang F T
  2013-07-19 13:57                                 ` Christoph Lameter
  0 siblings, 1 reply; 24+ messages in thread
From: Chen Gang F T @ 2013-07-19  0:05 UTC (permalink / raw)
  To: Christoph Lameter; +Cc: Chen Gang, Pekka Enberg, mpm, linux-mm, Andrew Morton

On 07/18/2013 09:45 PM, Christoph Lameter wrote:
> On Thu, 18 Jul 2013, Chen Gang wrote:
> 
>> > 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?
> 

No, at least, this patch (add parameter length checking) is useless.

>> > 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:

--------------------------------diff begin------------------------------

diff --git a/mm/slub.c b/mm/slub.c
index 2b02d66..8564677 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -3988,12 +3988,12 @@ static void free_loc_track(struct loc_track *t)
 			get_order(sizeof(struct location) * t->max));
 }
 
-static int alloc_loc_track(struct loc_track *t, unsigned long max, gfp_t flags)
+static int alloc_loc_track(struct loc_track *t, unsigned long size, gfp_t flags)
 {
 	struct location *l;
 	int order;
 
-	order = get_order(sizeof(struct location) * max);
+	order = get_order(sizeof(struct location) * size);
 
 	l = (void *)__get_free_pages(flags, order);
 	if (!l)
@@ -4003,7 +4003,7 @@ static int alloc_loc_track(struct loc_track *t, unsigned long max, gfp_t flags)
 		memcpy(l, t->loc, sizeof(struct location) * t->count);
 		free_loc_track(t);
 	}
-	t->max = max;
+	t->max = size;
 	t->loc = l;
 	return 1;
 }

--------------------------------diff end--------------------------------

Thanks
-- 
Chen Gang

--
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>

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH] mm/slub.c: add parameter length checking for alloc_loc_track()
  2013-07-19  0:05                               ` Chen Gang F T
@ 2013-07-19 13:57                                 ` Christoph Lameter
  2013-07-22  0:27                                   ` Chen Gang
                                                     ` (2 more replies)
  0 siblings, 3 replies; 24+ messages in thread
From: Christoph Lameter @ 2013-07-19 13:57 UTC (permalink / raw)
  To: Chen Gang F T; +Cc: Chen Gang, Pekka Enberg, mpm, linux-mm, Andrew Morton

On Fri, 19 Jul 2013, Chen Gang F T wrote:

> 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.

> 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 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>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] mm/slub.c: add parameter length checking for alloc_loc_track()
  2013-07-19 13:57                                 ` Christoph Lameter
@ 2013-07-22  0:27                                   ` Chen Gang
  2013-07-22  0:42                                   ` Wanpeng Li
  2013-07-22  0:42                                   ` Wanpeng Li
  2 siblings, 0 replies; 24+ messages in thread
From: Chen Gang @ 2013-07-22  0:27 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Chen Gang F T, Pekka Enberg, mpm, linux-mm, Andrew Morton

On 07/19/2013 09:57 PM, Christoph Lameter wrote:
> On Fri, 19 Jul 2013, Chen Gang F T wrote:
> 
>> > 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.
> 

Thanks.

>> > The diff may like this:
> I am fine with such a patch.
> 

If suitable,  I should send the related patch for it.

Is it necessary to send the related patch for it ?


> 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.
> 

At least for me, it is reasonable and necessary.

If possible, I'd like to do with it.

Excuse me, I have to do another things within this month. If really may
let me do, I should finish within next month (2013-08-31), is it OK ?


Thanks.
-- 
Chen Gang

--
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>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] mm/slub.c: add parameter length checking for alloc_loc_track()
  2013-07-19 13:57                                 ` Christoph Lameter
  2013-07-22  0:27                                   ` Chen Gang
  2013-07-22  0:42                                   ` Wanpeng Li
@ 2013-07-22  0:42                                   ` Wanpeng Li
  2013-07-22  1:30                                     ` Chen Gang
  2 siblings, 1 reply; 24+ messages in thread
From: Wanpeng Li @ 2013-07-22  0:42 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Chen Gang F T, Chen Gang, Pekka Enberg, mpm, linux-mm,
	Andrew Morton

On Fri, Jul 19, 2013 at 01:57:28PM +0000, Christoph Lameter wrote:
>On Fri, 19 Jul 2013, Chen Gang F T wrote:
>
>> 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.
>
>> 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.
	
Coincidence, I am doing this work recently and will post patches soon.
;-)

Regards,
Wanpeng Li 

>
>--
>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>

--
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>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] mm/slub.c: add parameter length checking for alloc_loc_track()
  2013-07-19 13:57                                 ` Christoph Lameter
  2013-07-22  0:27                                   ` Chen Gang
@ 2013-07-22  0:42                                   ` Wanpeng Li
  2013-07-22  0:42                                   ` Wanpeng Li
  2 siblings, 0 replies; 24+ messages in thread
From: Wanpeng Li @ 2013-07-22  0:42 UTC (permalink / raw)
  To: Christoph Lameter
  Cc: Chen Gang F T, Chen Gang, Pekka Enberg, mpm, linux-mm,
	Andrew Morton

On Fri, Jul 19, 2013 at 01:57:28PM +0000, Christoph Lameter wrote:
>On Fri, 19 Jul 2013, Chen Gang F T wrote:
>
>> 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.
>
>> 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.
	
Coincidence, I am doing this work recently and will post patches soon.
;-)

Regards,
Wanpeng Li 

>
>--
>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>

--
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>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] mm/slub.c: add parameter length checking for alloc_loc_track()
  2013-07-22  0:42                                   ` Wanpeng Li
@ 2013-07-22  1:30                                     ` Chen Gang
  0 siblings, 0 replies; 24+ messages in thread
From: Chen Gang @ 2013-07-22  1:30 UTC (permalink / raw)
  To: Wanpeng Li
  Cc: Christoph Lameter, Chen Gang F T, Pekka Enberg, mpm, linux-mm,
	Andrew Morton

On 07/22/2013 08:42 AM, Wanpeng Li wrote:
> On Fri, Jul 19, 2013 at 01:57:28PM +0000, Christoph Lameter wrote:
>> On Fri, 19 Jul 2013, Chen Gang F T wrote:
>>
>>> 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.
>>
>>> 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.
> 	
> Coincidence, I am doing this work recently and will post patches soon.
> ;-)
> 

Thanks.  :-)

> Regards,
> Wanpeng Li 
> 
>>
>> --
>> 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>
> 
> 
> 


-- 
Chen Gang

--
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>

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2013-07-22  1:31 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-07-08  8:07 [PATCH] mm/slub.c: remove 'per_cpu' which is useless variable Chen Gang
2013-07-11  6:45 ` Pekka Enberg
2013-07-11  6:50   ` Chen Gang
2013-07-11 16:45     ` Christoph Lameter
2013-07-11 18:28       ` Pekka Enberg
2013-07-11 20:16         ` Christoph Lameter
2013-07-11 23:52           ` Chen Gang
2013-07-12  0:23             ` [PATCH v2] " Chen Gang
2013-07-12  0:55               ` [PATCH] mm/slub.c: add parameter length checking for alloc_loc_track() Chen Gang
2013-07-12 13:49                 ` Christoph Lameter
2013-07-15  0:17                   ` Chen Gang
2013-07-15 15:16                     ` Christoph Lameter
2013-07-16  1:03                       ` Chen Gang
2013-07-17 15:03                         ` Christoph Lameter
2013-07-18  0:43                           ` Chen Gang
2013-07-18 13:45                             ` Christoph Lameter
2013-07-19  0:05                               ` Chen Gang F T
2013-07-19 13:57                                 ` Christoph Lameter
2013-07-22  0:27                                   ` Chen Gang
2013-07-22  0:42                                   ` Wanpeng Li
2013-07-22  0:42                                   ` Wanpeng Li
2013-07-22  1:30                                     ` Chen Gang
2013-07-12 13:45               ` [PATCH v2] mm/slub.c: remove 'per_cpu' which is useless variable Christoph Lameter
2013-07-15  0:08                 ` Chen Gang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).