Re: [PATCH v2 03/10] slab: move up code to get kmem_cache_node in free_block()

Re: [PATCH v2 03/10] slab: move up code to get kmem_cache_node in free_block()

[George Spelvin]

Seeing this patch made me think of a larger cleanup.
Would this be worth adding to the series?
(I incorporated your patch, as I couldn't decide whether
to write mine before or after; I assume it's no problem
to separate whichever way you want to order it.)

Commit comment something like:

Subject: Let clear_obj_pfmemalloc return the modified pointer

All callers fetch it immediately after, so just return the written
value for convenience.

Signed-off-by: George Spelvin <linux@horizon.com>
---
 mm/slab.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 388cb1ae6f..7fdc8df104 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -215,9 +215,9 @@ static inline void set_obj_pfmemalloc(void **objp)
 	return;
 }
 
-static inline void clear_obj_pfmemalloc(void **objp)
+static inline void *clear_obj_pfmemalloc(void **objp)
 {
-	*objp = (void *)((unsigned long)*objp & ~SLAB_OBJ_PFMEMALLOC);
+	return *objp = (void *)((unsigned long)*objp & ~SLAB_OBJ_PFMEMALLOC);
 }
 
 /*
@@ -809,10 +809,8 @@ static void *__ac_get_obj(struct kmem_cache *cachep, struct array_cache *ac,
 	if (unlikely(is_obj_pfmemalloc(objp))) {
 		struct kmem_cache_node *n;
 
-		if (gfp_pfmemalloc_allowed(flags)) {
-			clear_obj_pfmemalloc(&objp);
-			return objp;
-		}
+		if (gfp_pfmemalloc_allowed(flags))
+			return clear_obj_pfmemalloc(&objp);
 
 		/* The caller cannot use PFMEMALLOC objects, find another one */
 		for (i = 0; i < ac->avail; i++) {
@@ -833,9 +831,8 @@ static void *__ac_get_obj(struct kmem_cache *cachep, struct array_cache *ac,
 		if (!list_empty(&n->slabs_free) && force_refill) {
 			struct page *page = virt_to_head_page(objp);
 			ClearPageSlabPfmemalloc(page);
-			clear_obj_pfmemalloc(&objp);
 			recheck_pfmemalloc_active(cachep, ac);
-			return objp;
+			return clear_obj_pfmemalloc(&objp);
 		}
 
 		/* No !PFMEMALLOC objects available */
@@ -3362,17 +3359,12 @@ static void free_block(struct kmem_cache *cachep, void **objpp, int nr_objects,
 		       int node)
 {
 	int i;
-	struct kmem_cache_node *n;
+	struct kmem_cache_node *n = cachep->node[node];
 
 	for (i = 0; i < nr_objects; i++) {
-		void *objp;
-		struct page *page;
-
-		clear_obj_pfmemalloc(&objpp[i]);
-		objp = objpp[i];
+		void *objp = clear_obj_pfmemalloc(&objpp[i]);
+		struct page *page = virt_to_head_page(objp);
 
-		page = virt_to_head_page(objp);
-		n = cachep->node[node];
 		list_del(&page->lru);
 		check_spinlock_acquired_node(cachep, node);
 		slab_put_obj(cachep, page, objp, node);

Re: [PATCH v2 03/10] slab: move up code to get kmem_cache_node in free_block()

[David Rientjes]

On Wed, 7 May 2014, George Spelvin wrote:

> diff --git a/mm/slab.c b/mm/slab.c
> index 388cb1ae6f..7fdc8df104 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -215,9 +215,9 @@ static inline void set_obj_pfmemalloc(void **objp)
>  	return;
>  }
>  
> -static inline void clear_obj_pfmemalloc(void **objp)
> +static inline void *clear_obj_pfmemalloc(void **objp)
>  {
> -	*objp = (void *)((unsigned long)*objp & ~SLAB_OBJ_PFMEMALLOC);
> +	return *objp = (void *)((unsigned long)*objp & ~SLAB_OBJ_PFMEMALLOC);
>  }
>  
>  /*
> @@ -809,10 +809,8 @@ static void *__ac_get_obj(struct kmem_cache *cachep, struct array_cache *ac,
>  	if (unlikely(is_obj_pfmemalloc(objp))) {
>  		struct kmem_cache_node *n;
>  
> -		if (gfp_pfmemalloc_allowed(flags)) {
> -			clear_obj_pfmemalloc(&objp);
> -			return objp;
> -		}
> +		if (gfp_pfmemalloc_allowed(flags))
> +			return clear_obj_pfmemalloc(&objp);
>  
>  		/* The caller cannot use PFMEMALLOC objects, find another one */
>  		for (i = 0; i < ac->avail; i++) {
> @@ -833,9 +831,8 @@ static void *__ac_get_obj(struct kmem_cache *cachep, struct array_cache *ac,
>  		if (!list_empty(&n->slabs_free) && force_refill) {
>  			struct page *page = virt_to_head_page(objp);
>  			ClearPageSlabPfmemalloc(page);
> -			clear_obj_pfmemalloc(&objp);
>  			recheck_pfmemalloc_active(cachep, ac);
> -			return objp;
> +			return clear_obj_pfmemalloc(&objp);
>  		}
>  
>  		/* No !PFMEMALLOC objects available */
> @@ -3362,17 +3359,12 @@ static void free_block(struct kmem_cache *cachep, void **objpp, int nr_objects,
>  		       int node)
>  {
>  	int i;
> -	struct kmem_cache_node *n;
> +	struct kmem_cache_node *n = cachep->node[node];
>  
>  	for (i = 0; i < nr_objects; i++) {
> -		void *objp;
> -		struct page *page;
> -
> -		clear_obj_pfmemalloc(&objpp[i]);
> -		objp = objpp[i];
> +		void *objp = clear_obj_pfmemalloc(&objpp[i]);
> +		struct page *page = virt_to_head_page(objp);
>  
> -		page = virt_to_head_page(objp);
> -		n = cachep->node[node];
>  		list_del(&page->lru);
>  		check_spinlock_acquired_node(cachep, node);
>  		slab_put_obj(cachep, page, objp, node);

I think this unnecessarily obfuscates 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>

Re: [PATCH v2 03/10] slab: move up code to get kmem_cache_node in free_block()

[George Spelvin]

> I think this unnecessarily obfuscates the code.

Thanks for the feedback!  (Even if it's negative, I appreciate it.)

To me, the confusing thing is the whole passing-a-pointer-to-a-pointer
business.  How about the following, which makes set_obj_pfmemalloc and
clear_obj_pfmemalloc take void *, not void **?  Is this better, or worse?

(Also Signed-off-by: George Spelvin <linux@horizon.com>)

diff --git a/mm/slab.c b/mm/slab.c
index 388cb1ae6f..f09716ce87 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -209,15 +209,14 @@ static inline bool is_obj_pfmemalloc(void *objp)
 	return (unsigned long)objp & SLAB_OBJ_PFMEMALLOC;
 }
 
-static inline void set_obj_pfmemalloc(void **objp)
+static inline void *set_obj_pfmemalloc(void *objp)
 {
-	*objp = (void *)((unsigned long)*objp | SLAB_OBJ_PFMEMALLOC);
-	return;
+	return (void *)((unsigned long)objp | SLAB_OBJ_PFMEMALLOC);
 }
 
-static inline void clear_obj_pfmemalloc(void **objp)
+static inline void *clear_obj_pfmemalloc(void *objp)
 {
-	*objp = (void *)((unsigned long)*objp & ~SLAB_OBJ_PFMEMALLOC);
+	return (void *)((unsigned long)objp & ~SLAB_OBJ_PFMEMALLOC);
 }
 
 /*
@@ -809,10 +808,8 @@ static void *__ac_get_obj(struct kmem_cache *cachep, struct array_cache *ac,
 	if (unlikely(is_obj_pfmemalloc(objp))) {
 		struct kmem_cache_node *n;
 
-		if (gfp_pfmemalloc_allowed(flags)) {
-			clear_obj_pfmemalloc(&objp);
-			return objp;
-		}
+		if (gfp_pfmemalloc_allowed(flags))
+			return clear_obj_pfmemalloc(objp);
 
 		/* The caller cannot use PFMEMALLOC objects, find another one */
 		for (i = 0; i < ac->avail; i++) {
@@ -833,9 +830,8 @@ static void *__ac_get_obj(struct kmem_cache *cachep, struct array_cache *ac,
 		if (!list_empty(&n->slabs_free) && force_refill) {
 			struct page *page = virt_to_head_page(objp);
 			ClearPageSlabPfmemalloc(page);
-			clear_obj_pfmemalloc(&objp);
 			recheck_pfmemalloc_active(cachep, ac);
-			return objp;
+			return clear_obj_pfmemalloc(objp);
 		}
 
 		/* No !PFMEMALLOC objects available */
@@ -866,7 +862,7 @@ static void *__ac_put_obj(struct kmem_cache *cachep, struct array_cache *ac,
 		/* Some pfmemalloc slabs exist, check if this is one */
 		struct page *page = virt_to_head_page(objp);
 		if (PageSlabPfmemalloc(page))
-			set_obj_pfmemalloc(&objp);
+			objp = set_obj_pfmemalloc(objp);
 	}
 
 	return objp;
@@ -3362,17 +3358,12 @@ static void free_block(struct kmem_cache *cachep, void **objpp, int nr_objects,
 		       int node)
 {
 	int i;
-	struct kmem_cache_node *n;
+	struct kmem_cache_node *n = cachep->node[node];
 
 	for (i = 0; i < nr_objects; i++) {
-		void *objp;
-		struct page *page;
-
-		clear_obj_pfmemalloc(&objpp[i]);
-		objp = objpp[i];
+		void *objp = objpp[i] = clear_obj_pfmemalloc(objpp[i]);
+		struct page *page = virt_to_head_page(objp);
 
-		page = virt_to_head_page(objp);
-		n = cachep->node[node];
 		list_del(&page->lru);
 		check_spinlock_acquired_node(cachep, node);
 		slab_put_obj(cachep, page, objp, node);

Re: [PATCH v2 03/10] slab: move up code to get kmem_cache_node in free_block()

[David Rientjes]

On Wed, 7 May 2014, George Spelvin wrote:

> > I think this unnecessarily obfuscates the code.
> 
> Thanks for the feedback!  (Even if it's negative, I appreciate it.)
> 
> To me, the confusing thing is the whole passing-a-pointer-to-a-pointer
> business.  How about the following, which makes set_obj_pfmemalloc and
> clear_obj_pfmemalloc take void *, not void **?  Is this better, or worse?
> 

A function called clear_obj_pfmemalloc() doesn't indicate it's returning 
anything, I think the vast majority of people would believe that it 
returns void just as it does.  There's no complier generated code 
optimization with this patch and I'm not sure it's even correct since 
you're now clearing after doing recheck_pfmemalloc_active().

I think it does make sense to remove the pointless "return;" in 
set_obj_pfmemalloc(), however.  Not sure it's worth asking someone to 
merge it, though.

--
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 v2 03/10] slab: move up code to get kmem_cache_node in free_block()

[George Spelvin]

> A function called clear_obj_pfmemalloc() doesn't indicate it's returning 
> anything, I think the vast majority of people would believe that it 
> returns void just as it does.  There's no complier generated code 
> optimization with this patch and

> I'm not sure it's even correct since 
> you're now clearing after doing recheck_pfmemalloc_active().

I thought this through before rearranging the code.
recheck_pfmemalloc_active() checks global lists, but __ac_get_obj()
is doing clear_obj_pfmemalloc on a local variable.

I think it does make sense to remove the pointless "return;" in 
set_obj_pfmemalloc(), however.  Not sure it's worth asking someone to 
merge it, though.

Re: [PATCH v2 03/10] slab: move up code to get kmem_cache_node in free_block()

[George Spelvin]

(Oops, previous e-mail was sent halfway through composition in error.)

> I'm not sure it's even correct since 
> you're now clearing after doing recheck_pfmemalloc_active().

I thought this through before rearranging the code.
recheck_pfmemalloc_active() checks global lists, but __ac_get_obj()
is doing clear_obj_pfmemalloc on a local variable.  So it
can't affect recheck_pfmemalloc_active().

> A function called clear_obj_pfmemalloc() doesn't indicate it's returning 
> anything, I think the vast majority of people would believe that it 
> returns void just as it does.

Perhaps the name needs to be modified, but it's still pretty clear.
It just clears the bit in its argument and returns it, as opposed to
operating in-place.

In particular, when reading the code that calls it, there is obviously
a return value.  What could it possibly be?

> There's no complier generated code optimization with this patch

On that subject, you're absolutely correct:
   text    data     bss     dec     hex filename
  10635     939       4   11578    2d3a mm/slab.o.before
  10635     939       4   11578    2d3a mm/slab.o.after

If I don't have CONFIG_CC_OPTIMIZE_FOR_SIZE=y, the padding NOPs after
unconditional jumps get aligned a little differently and it actually
gets bigger.

   text    data     bss     dec     hex filename
  12958    1079       4   14041    36d9 mm/slab.o.before
  12990    1079       4   14073    36f9 mm/slab.o.after

__ac_get_obj actually spills one fewer register in this case, and
the code paths seem a little cleaner, but I haven't gone through
it completely.

Re: [PATCH v2 03/10] slab: move up code to get kmem_cache_node in free_block()

[Christoph Lameter]

On Wed, 7 May 2014, David Rientjes wrote:

> > @@ -3362,17 +3359,12 @@ static void free_block(struct kmem_cache *cachep, void **objpp, int nr_objects,
> >  		       int node)
> >  {
> >  	int i;
> > -	struct kmem_cache_node *n;
> > +	struct kmem_cache_node *n = cachep->node[node];
> >
> >  	for (i = 0; i < nr_objects; i++) {
> > -		void *objp;
> > -		struct page *page;
> > -
> > -		clear_obj_pfmemalloc(&objpp[i]);
> > -		objp = objpp[i];
> > +		void *objp = clear_obj_pfmemalloc(&objpp[i]);
> > +		struct page *page = virt_to_head_page(objp);
> >
> > -		page = virt_to_head_page(objp);
> > -		n = cachep->node[node];
> >  		list_del(&page->lru);
> >  		check_spinlock_acquired_node(cachep, node);
> >  		slab_put_obj(cachep, page, objp, node);
>
> I think this unnecessarily obfuscates the code.

It takes the lookup out of the loop. What does the obfuscation?


--
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 v2 03/10] slab: move up code to get kmem_cache_node in free_block()

[George Spelvin]

>>> @@ -3362,17 +3359,12 @@ static void free_block(struct kmem_cache *cachep, void **objpp, int nr_objects,
>>>  		       int node)
>>>  {
>>>  	int i;
>>> -	struct kmem_cache_node *n;
>>> +	struct kmem_cache_node *n = cachep->node[node];
>>>
>>>  	for (i = 0; i < nr_objects; i++) {
>>> -		void *objp;
>>> -		struct page *page;
>>> -
>>> -		clear_obj_pfmemalloc(&objpp[i]);
>>> -		objp = objpp[i];
>>> +		void *objp = clear_obj_pfmemalloc(&objpp[i]);
>>> +		struct page *page = virt_to_head_page(objp);
>>>
>>> -		page = virt_to_head_page(objp);
>>> -		n = cachep->node[node];
>>>  		list_del(&page->lru);
>>>  		check_spinlock_acquired_node(cachep, node);
>>>  		slab_put_obj(cachep, page, objp, node);
>>
>> I think this unnecessarily obfuscates the code.

> It takes the lookup out of the loop. What does the obfuscation?

Taking the lookup of n out was the original cleanup patch; that part is
not my doing.

I changed clear_obj_pfmemalloc to return the modified pointer.
(As well as storing it back in version 1 which you quoted, instead of
storing it back in version 2.)

[PATCH v2 00/10] clean-up and remove lockdep annotation in SLAB

[Joonsoo Kim]

This patchset does some clean-up and tries to remove lockdep annotation.

Patches 1~3 are just for really really minor improvement.
Patches 4~10 are for clean-up and removing lockdep annotation.

There are two cases that lockdep annotation is needed in SLAB.
1) holding two node locks
2) holding two array cache(alien cache) locks

I looked at the code and found that we can avoid these cases without
any negative effect.

1) occurs if freeing object makes new free slab and we decide to
destroy it. Although we don't need to hold the lock during destroying
a slab, current code do that. Destroying a slab without holding the lock
would help the reduction of the lock contention. To do it, I change the
implementation that new free slab is destroyed after releasing the lock.

2) occurs on similar situation. When we free object from non-local node,
we put this object to alien cache with holding the alien cache lock.
If alien cache is full, we try to flush alien cache to proper node cache,
and, in this time, new free slab could be made. Destroying it would be
started and we will free metadata object which comes from another node.
In this case, we need another node's alien cache lock to free object.
This forces us to hold two array cache locks and then we need lockdep
annotation although they are always different locks and deadlock cannot
be possible. To prevent this situation, I use same way as 1).

In this way, we can avoid 1) and 2) cases, and then, can remove lockdep
annotation. As short stat noted, this makes SLAB code much simpler.

Many of this series get Ack from Christoph Lameter on previous iteration,
but 1, 2, 9 and 10 need to get Ack. There is no big change from previous
iteration. It is just rebased on current linux-next.

Thanks.

Joonsoo Kim (10):
  slab: add unlikely macro to help compiler
  slab: makes clear_obj_pfmemalloc() just return masked value
  slab: move up code to get kmem_cache_node in free_block()
  slab: defer slab_destroy in free_block()
  slab: factor out initialization of arracy cache
  slab: introduce alien_cache
  slab: use the lock on alien_cache, instead of the lock on array_cache
  slab: destroy a slab without holding any alien cache lock
  slab: remove a useless lockdep annotation
  slab: remove BAD_ALIEN_MAGIC

 mm/slab.c |  391 ++++++++++++++++++++++---------------------------------------
 mm/slab.h |    2 +-
 2 files changed, 140 insertions(+), 253 deletions(-)

-- 
1.7.9.5

--
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 v2 03/10] slab: move up code to get kmem_cache_node in free_block()

[Joonsoo Kim]

node isn't changed, so we don't need to retreive this structure
everytime we move the object. Maybe compiler do this optimization,
but making it explicitly is better.

Acked-by: Christoph Lameter <cl@linux.com>
Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

diff --git a/mm/slab.c b/mm/slab.c
index e2c80df..92d08e3 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3340,7 +3340,7 @@ static void free_block(struct kmem_cache *cachep, void **objpp, int nr_objects,
 		       int node)
 {
 	int i;
-	struct kmem_cache_node *n;
+	struct kmem_cache_node *n = cachep->node[node];
 
 	for (i = 0; i < nr_objects; i++) {
 		void *objp;
@@ -3349,7 +3349,6 @@ static void free_block(struct kmem_cache *cachep, void **objpp, int nr_objects,
 		objp = clear_obj_pfmemalloc(objpp[i]);
 
 		page = virt_to_head_page(objp);
-		n = cachep->node[node];
 		list_del(&page->lru);
 		check_spinlock_acquired_node(cachep, node);
 		slab_put_obj(cachep, page, objp, node);
-- 
1.7.9.5

--
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 v2 03/10] slab: move up code to get kmem_cache_node in free_block()

[David Rientjes]

On Wed, 7 May 2014, Joonsoo Kim wrote:

> node isn't changed, so we don't need to retreive this structure
> everytime we move the object. Maybe compiler do this optimization,
> but making it explicitly is better.
> 
> Acked-by: Christoph Lameter <cl@linux.com>
> Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>

Acked-by: David Rientjes <rientjes@google.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>