[PATCH] slab: avoid unnecessary touching of a partial slab

[PATCH] slab: avoid unnecessary touching of a partial slab

[Zhao Jin]

In cache_alloc_refill(), after refilling from a partial slab, if the
slab remains partial, it would be deleted from and then added again to
the partial list. As the slab is the first element in the list before
deletion, such behavior has no effect. This patch avoids touching the
slab in this case.

Signed-off-by: Zhao Jin <cronozhj@gmail.com>
---
 mm/slab.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 6d90a09..3469c6f 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3121,11 +3121,14 @@ retry:
 		check_slabp(cachep, slabp);

 		/* move slabp to correct slabp list: */
-		list_del(&slabp->list);
-		if (slabp->free == BUFCTL_END)
+		if (slabp->free == BUFCTL_END) {
+			list_del(&slabp->list);
 			list_add(&slabp->list, &l3->slabs_full);
-		else
+		}
+		else if (&slabp->list != l3->slabs_partial.next) {
+			list_del(&slabp->list);
 			list_add(&slabp->list, &l3->slabs_partial);
+		}
 	}

 must_grow:
-- 
1.7.4.1

Re: [PATCH] slab: avoid unnecessary touching of a partial slab

[Zhao Jin]

Sorry, one line in this patch does not conform to the coding style of
kernel source code. Here is the corrected version:

---
 mm/slab.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 6d90a09..0c7a212 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3121,11 +3121,13 @@ retry:
 		check_slabp(cachep, slabp);

 		/* move slabp to correct slabp list: */
-		list_del(&slabp->list);
-		if (slabp->free == BUFCTL_END)
+		if (slabp->free == BUFCTL_END) {
+			list_del(&slabp->list);
 			list_add(&slabp->list, &l3->slabs_full);
-		else
+		} else if (&slabp->list != l3->slabs_partial.next) {
+			list_del(&slabp->list);
 			list_add(&slabp->list, &l3->slabs_partial);
+		}
 	}

 must_grow:
-- 
1.7.4.1

Re: [PATCH] slab: avoid unnecessary touching of a partial slab

[Christoph Lameter]

On Fri, 26 Aug 2011, Zhao Jin wrote:

> In cache_alloc_refill(), after refilling from a partial slab, if the
> slab remains partial, it would be deleted from and then added again to
> the partial list. As the slab is the first element in the list before
> deletion, such behavior has no effect. This patch avoids touching the
> slab in this case.

The list_del/list_add action is a list_move.

Re: [PATCH] slab: avoid unnecessary touching of a partial slab

[Zhao Jin]

On Fri, 2011-08-26 at 09:08 -0500, Christoph Lameter wrote:
> On Fri, 26 Aug 2011, Zhao Jin wrote:
> 
> > In cache_alloc_refill(), after refilling from a partial slab, if the
> > slab remains partial, it would be deleted from and then added again to
> > the partial list. As the slab is the first element in the list before
> > deletion, such behavior has no effect. This patch avoids touching the
> > slab in this case.
> 
> The list_del/list_add action is a list_move.

But in cache_alloc_refill(), it is the first slab (that is: its list
field is pointed by the l3->slabs_partial.next) that is picked from the
partial list for refilling. Assume it remains partial after that. Since
list_add inserts a new element after the head (l3->slabs_partial), the
position of the slab in the partial list won't change after
list_del/list_add: it will be still the first element. Therefore,
list_del/list_add has done nothing actually in this case.

Regards,
zhj

Re: [PATCH] slab: avoid unnecessary touching of a partial slab

[Christoph Lameter]

On Fri, 26 Aug 2011, Zhao Jin wrote:

> On Fri, 2011-08-26 at 09:08 -0500, Christoph Lameter wrote:
> > On Fri, 26 Aug 2011, Zhao Jin wrote:
> >
> > > In cache_alloc_refill(), after refilling from a partial slab, if the
> > > slab remains partial, it would be deleted from and then added again to
> > > the partial list. As the slab is the first element in the list before
> > > deletion, such behavior has no effect. This patch avoids touching the
> > > slab in this case.
> >
> > The list_del/list_add action is a list_move.
>
> But in cache_alloc_refill(), it is the first slab (that is: its list
> field is pointed by the l3->slabs_partial.next) that is picked from the
> partial list for refilling. Assume it remains partial after that. Since
> list_add inserts a new element after the head (l3->slabs_partial), the
> position of the slab in the partial list won't change after
> list_del/list_add: it will be still the first element. Therefore,
> list_del/list_add has done nothing actually in this case.

Correct but still tthe del/add is a list_move operation. Convert that the
other case as well?

Re: [PATCH] slab: avoid unnecessary touching of a partial slab

[Zhao Jin]

On Fri, 2011-08-26 at 09:50 -0500, Christoph Lameter wrote:
> On Fri, 26 Aug 2011, Zhao Jin wrote:
> 
> > On Fri, 2011-08-26 at 09:08 -0500, Christoph Lameter wrote:
> > > On Fri, 26 Aug 2011, Zhao Jin wrote:
> > >
> > > > In cache_alloc_refill(), after refilling from a partial slab, if the
> > > > slab remains partial, it would be deleted from and then added again to
> > > > the partial list. As the slab is the first element in the list before
> > > > deletion, such behavior has no effect. This patch avoids touching the
> > > > slab in this case.
> > >
> > > The list_del/list_add action is a list_move.
> >
> > But in cache_alloc_refill(), it is the first slab (that is: its list
> > field is pointed by the l3->slabs_partial.next) that is picked from the
> > partial list for refilling. Assume it remains partial after that. Since
> > list_add inserts a new element after the head (l3->slabs_partial), the
> > position of the slab in the partial list won't change after
> > list_del/list_add: it will be still the first element. Therefore,
> > list_del/list_add has done nothing actually in this case.
> 
> Correct but still tthe del/add is a list_move operation. Convert that the
> other case as well?
> 
Yes. The patch avoids list_del/list_add only if it is the case that a
partial slab was used for refilling and remains partial (that is:
partial => partial). If it is not the case( so it must be either partial
=> full, free => full or free => partial) the patched code behaves the
same as current (list_del/list_add).

Regards,
zhj

Re: [PATCH] slab: avoid unnecessary touching of a partial slab

[Christoph Lameter]

On Fri, 26 Aug 2011, Zhao Jin wrote:

> > Correct but still tthe del/add is a list_move operation. Convert that the
> > other case as well?
> >
> Yes. The patch avoids list_del/list_add only if it is the case that a
> partial slab was used for refilling and remains partial (that is:
> partial => partial). If it is not the case( so it must be either partial
> => full, free => full or free => partial) the patched code behaves the
> same as current (list_del/list_add).

The second case is what I want you to change to list_move.

Re: [PATCH] slab: avoid unnecessary touching of a partial slab

[Zhao Jin]

On Fri, 2011-08-26 at 10:05 -0500, Christoph Lameter wrote:
> On Fri, 26 Aug 2011, Zhao Jin wrote:
> 
> > > Correct but still tthe del/add is a list_move operation. Convert that the
> > > other case as well?
> > >
> > Yes. The patch avoids list_del/list_add only if it is the case that a
> > partial slab was used for refilling and remains partial (that is:
> > partial => partial). If it is not the case( so it must be either partial
> > => full, free => full or free => partial) the patched code behaves the
> > same as current (list_del/list_add).
> 
> The second case is what I want you to change to list_move.
> 
> 
Oops. I misunderstood your reply... I shall do it. Thanks a lot for
pointing it out.

Regards,
zhj