[PATCH 1/3] mm/slab: use list_first_entry_or_null()

[PATCH 1/3] mm/slab: use list_first_entry_or_null()

[Geliang Tang]

Simplify the code with list_first_entry_or_null().

Signed-off-by: Geliang Tang <geliangtang@163.com>
---
 mm/slab.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 4765c97..6bb0466 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2791,18 +2791,18 @@ retry:
 	}
 
 	while (batchcount > 0) {
-		struct list_head *entry;
 		struct page *page;
 		/* Get slab alloc is to come from. */
-		entry = n->slabs_partial.next;
-		if (entry == &n->slabs_partial) {
+		page = list_first_entry_or_null(&n->slabs_partial,
+				struct page, lru);
+		if (!page) {
 			n->free_touched = 1;
-			entry = n->slabs_free.next;
-			if (entry == &n->slabs_free)
+			page = list_first_entry_or_null(&n->slabs_free,
+					struct page, lru);
+			if (!page)
 				goto must_grow;
 		}
 
-		page = list_entry(entry, struct page, lru);
 		check_spinlock_acquired(cachep);
 
 		/*
@@ -3085,7 +3085,6 @@ retry:
 static void *____cache_alloc_node(struct kmem_cache *cachep, gfp_t flags,
 				int nodeid)
 {
-	struct list_head *entry;
 	struct page *page;
 	struct kmem_cache_node *n;
 	void *obj;
@@ -3098,15 +3097,16 @@ static void *____cache_alloc_node(struct kmem_cache *cachep, gfp_t flags,
 retry:
 	check_irq_off();
 	spin_lock(&n->list_lock);
-	entry = n->slabs_partial.next;
-	if (entry == &n->slabs_partial) {
+	page = list_first_entry_or_null(&n->slabs_partial,
+			struct page, lru);
+	if (!page) {
 		n->free_touched = 1;
-		entry = n->slabs_free.next;
-		if (entry == &n->slabs_free)
+		page = list_first_entry_or_null(&n->slabs_free,
+				struct page, lru);
+		if (!page)
 			goto must_grow;
 	}
 
-	page = list_entry(entry, struct page, lru);
 	check_spinlock_acquired_node(cachep, nodeid);
 
 	STATS_INC_NODEALLOCS(cachep);
-- 
2.5.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>

[PATCH 2/3] mm/slab: use list_for_each_entry in cache_flusharray

[Geliang Tang]

Simplify the code with list_for_each_entry().

Signed-off-by: Geliang Tang <geliangtang@163.com>
---
 mm/slab.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 6bb0466..5d5aa3b 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -3338,17 +3338,12 @@ free_done:
 #if STATS
 	{
 		int i = 0;
-		struct list_head *p;
-
-		p = n->slabs_free.next;
-		while (p != &(n->slabs_free)) {
-			struct page *page;
+		struct page *page;
 
-			page = list_entry(p, struct page, lru);
+		list_for_each_entry(page, &n->slabs_free, lru) {
 			BUG_ON(page->active);
 
 			i++;
-			p = p->next;
 		}
 		STATS_SET_FREEABLE(cachep, i);
 	}
-- 
2.5.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>

[PATCH 3/3] mm/slab: use list_{empty_careful,last_entry} in drain_freelist

[Geliang Tang]

To simplify the code, use list_empty_careful instead of list_empty.
To make the intention clearer, use list_last_entry instead of list_entry.

Signed-off-by: Geliang Tang <geliangtang@163.com>
---
 mm/slab.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 5d5aa3b..1a7d91c 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2362,21 +2362,14 @@ static void drain_cpu_caches(struct kmem_cache *cachep)
 static int drain_freelist(struct kmem_cache *cache,
 			struct kmem_cache_node *n, int tofree)
 {
-	struct list_head *p;
 	int nr_freed;
 	struct page *page;
 
 	nr_freed = 0;
-	while (nr_freed < tofree && !list_empty(&n->slabs_free)) {
+	while (nr_freed < tofree && !list_empty_careful(&n->slabs_free)) {
 
 		spin_lock_irq(&n->list_lock);
-		p = n->slabs_free.prev;
-		if (p == &n->slabs_free) {
-			spin_unlock_irq(&n->list_lock);
-			goto out;
-		}
-
-		page = list_entry(p, struct page, lru);
+		page = list_last_entry(&n->slabs_free, struct page, lru);
 #if DEBUG
 		BUG_ON(page->active);
 #endif
@@ -2390,7 +2383,6 @@ static int drain_freelist(struct kmem_cache *cache,
 		slab_destroy(cache, page);
 		nr_freed++;
 	}
-out:
 	return nr_freed;
 }
 
-- 
2.5.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>

Re: [PATCH 3/3] mm/slab: use list_{empty_careful,last_entry} in drain_freelist

[Christoph Lameter]

On Wed, 2 Dec 2015, Geliang Tang wrote:

> diff --git a/mm/slab.c b/mm/slab.c
> index 5d5aa3b..1a7d91c 100644
> --- a/mm/slab.c
> +++ b/mm/slab.c
> @@ -2362,21 +2362,14 @@ static void drain_cpu_caches(struct kmem_cache *cachep)
>  static int drain_freelist(struct kmem_cache *cache,
>  			struct kmem_cache_node *n, int tofree)
>  {
> -	struct list_head *p;
>  	int nr_freed;
>  	struct page *page;
>
>  	nr_freed = 0;
> -	while (nr_freed < tofree && !list_empty(&n->slabs_free)) {
> +	while (nr_freed < tofree && !list_empty_careful(&n->slabs_free)) {
>
>  		spin_lock_irq(&n->list_lock);
> -		p = n->slabs_free.prev;
> -		if (p == &n->slabs_free) {
> -			spin_unlock_irq(&n->list_lock);
> -			goto out;
> -		}
> -
> -		page = list_entry(p, struct page, lru);
> +		page = list_last_entry(&n->slabs_free, struct page, lru);

This is safe? Process could be rescheduled and lots of things could happen
before disabling irqs.

--
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] mm/slab.c: use list_{empty_careful,last_entry} in drain_freelist

[Geliang Tang]

To make the intention clearer, use list_empty_careful and list_last_entry
in drain_freelist().

Signed-off-by: Geliang Tang <geliangtang@163.com>
---
 mm/slab.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/mm/slab.c b/mm/slab.c
index 5d5aa3b..925921e 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -2362,7 +2362,6 @@ static void drain_cpu_caches(struct kmem_cache *cachep)
 static int drain_freelist(struct kmem_cache *cache,
 			struct kmem_cache_node *n, int tofree)
 {
-	struct list_head *p;
 	int nr_freed;
 	struct page *page;
 
@@ -2370,13 +2369,12 @@ static int drain_freelist(struct kmem_cache *cache,
 	while (nr_freed < tofree && !list_empty(&n->slabs_free)) {
 
 		spin_lock_irq(&n->list_lock);
-		p = n->slabs_free.prev;
-		if (p == &n->slabs_free) {
+		if (list_empty_careful(&n->slabs_free)) {
 			spin_unlock_irq(&n->list_lock);
 			goto out;
 		}
 
-		page = list_entry(p, struct page, lru);
+		page = list_last_entry(&n->slabs_free, struct page, lru);
 #if DEBUG
 		BUG_ON(page->active);
 #endif
-- 
2.5.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>

Re: [PATCH v2] mm/slab.c: use list_{empty_careful,last_entry} in drain_freelist

[Christoph Lameter]

On Thu, 3 Dec 2015, Geliang Tang wrote:

>  	while (nr_freed < tofree && !list_empty(&n->slabs_free)) {
>
>  		spin_lock_irq(&n->list_lock);
> -		p = n->slabs_free.prev;
> -		if (p == &n->slabs_free) {
> +		if (list_empty_careful(&n->slabs_free)) {

We have taken the lock. Why do we need to be "careful"? list_empty()
shoudl work right?

>  			spin_unlock_irq(&n->list_lock);
>  			goto out;
>  		}
>
> -		page = list_entry(p, struct page, lru);
> +		page = list_last_entry(&n->slabs_free, struct page, lru);

last???

Would the the other new function that returns NULL on the empty list or
the pointer not be useful here too and save some code?

This patch seems to make it difficult to understand 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] mm/slab.c: use list_{empty_careful,last_entry} in drain_freelist

[Geliang Tang]

On Thu, Dec 03, 2015 at 08:53:21AM -0600, Christoph Lameter wrote:
> On Thu, 3 Dec 2015, Geliang Tang wrote:
> 
> >  	while (nr_freed < tofree && !list_empty(&n->slabs_free)) {
> >
> >  		spin_lock_irq(&n->list_lock);
> > -		p = n->slabs_free.prev;
> > -		if (p == &n->slabs_free) {
> > +		if (list_empty_careful(&n->slabs_free)) {
> 
> We have taken the lock. Why do we need to be "careful"? list_empty()
> shoudl work right?

Yes. list_empty() is OK.

> 
> >  			spin_unlock_irq(&n->list_lock);
> >  			goto out;
> >  		}
> >
> > -		page = list_entry(p, struct page, lru);
> > +		page = list_last_entry(&n->slabs_free, struct page, lru);
> 
> last???

The original code delete the page from the tail of slabs_free list.

> 
> Would the the other new function that returns NULL on the empty list or
> the pointer not be useful here too and save some code?

Sorry, I don't really understand what do you mean. Can you please specify
it a little bit?

Thanks.

- Geliang

> 
> This patch seems to make it difficult to understand 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] mm/slab.c: use list_{empty_careful,last_entry} in drain_freelist

[Christoph Lameter]

On Fri, 4 Dec 2015, Geliang Tang wrote:

> On Thu, Dec 03, 2015 at 08:53:21AM -0600, Christoph Lameter wrote:
> > On Thu, 3 Dec 2015, Geliang Tang wrote:
> >
> > >  	while (nr_freed < tofree && !list_empty(&n->slabs_free)) {
> > >
> > >  		spin_lock_irq(&n->list_lock);
> > > -		p = n->slabs_free.prev;
> > > -		if (p == &n->slabs_free) {
> > > +		if (list_empty_careful(&n->slabs_free)) {
> >
> > We have taken the lock. Why do we need to be "careful"? list_empty()
> > shoudl work right?
>
> Yes. list_empty() is OK.
>
> >
> > >  			spin_unlock_irq(&n->list_lock);
> > >  			goto out;
> > >  		}
> > >
> > > -		page = list_entry(p, struct page, lru);
> > > +		page = list_last_entry(&n->slabs_free, struct page, lru);
> >
> > last???
>
> The original code delete the page from the tail of slabs_free list.

Maybe make the code clearer by using another method to get the page
pointer?

> >
> > Would the the other new function that returns NULL on the empty list or
> > the pointer not be useful here too and save some code?
>
> Sorry, I don't really understand what do you mean. Can you please specify
> it a little bit?

I take that back. list_empty is the best choice here.

--
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] mm/slab.c: use list_{empty_careful,last_entry} in drain_freelist

[Geliang Tang]

On Fri, Dec 04, 2015 at 10:16:38AM -0600, Christoph Lameter wrote:
> On Fri, 4 Dec 2015, Geliang Tang wrote:
> 
> > On Thu, Dec 03, 2015 at 08:53:21AM -0600, Christoph Lameter wrote:
> > > On Thu, 3 Dec 2015, Geliang Tang wrote:
> > >
> > > >  	while (nr_freed < tofree && !list_empty(&n->slabs_free)) {
> > > >
> > > >  		spin_lock_irq(&n->list_lock);
> > > > -		p = n->slabs_free.prev;
> > > > -		if (p == &n->slabs_free) {
> > > > +		if (list_empty_careful(&n->slabs_free)) {
> > >
> > > We have taken the lock. Why do we need to be "careful"? list_empty()
> > > shoudl work right?
> >
> > Yes. list_empty() is OK.
> >
> > >
> > > >  			spin_unlock_irq(&n->list_lock);
> > > >  			goto out;
> > > >  		}
> > > >
> > > > -		page = list_entry(p, struct page, lru);
> > > > +		page = list_last_entry(&n->slabs_free, struct page, lru);
> > >
> > > last???
> >
> > The original code delete the page from the tail of slabs_free list.
> 
> Maybe make the code clearer by using another method to get the page
> pointer?
> 
> > >
> > > Would the the other new function that returns NULL on the empty list or
> > > the pointer not be useful here too and save some code?
> >
> > Sorry, I don't really understand what do you mean. Can you please specify
> > it a little bit?
> 
> I take that back. list_empty is the best choice here.

If we use list_empty(), there will be two list_empty() in the code:

        while (nr_freed < tofree && !list_empty(&n->slabs_free)) {
                spin_lock_irq(&n->list_lock);
                if (list_empty(&n->slabs_free)) {
                        spin_unlock_irq(&n->list_lock);
                        goto out; 
                }
                page = list_last_entry(&n->slabs_free, struct page, lru);
                list_del(&page->lru);
                spin_unlock_irq(&n->list_lock);
        }

Or can we drop the first list_empty() like this? It will function the same as the above code.

        while (nr_freed < tofree) {
                spin_lock_irq(&n->list_lock);
                if (list_empty(&n->slabs_free)) {
                        spin_unlock_irq(&n->list_lock);
                        goto out; 
                }
                page = list_last_entry(&n->slabs_free, struct page, lru);
                list_del(&page->lru);
                spin_unlock_irq(&n->list_lock);
        }

Please let me know which one is better?

Thanks.

- Geliang

--
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] mm/slab: use list_for_each_entry in cache_flusharray

[Christoph Lameter]

On Wed, 2 Dec 2015, Geliang Tang wrote:

> Simplify the code with list_for_each_entry().

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 1/3] mm/slab: use list_first_entry_or_null()

[Christoph Lameter]

On Wed, 2 Dec 2015, Geliang Tang wrote:

> Simplify the code with list_first_entry_or_null().

Looks like there are two code snippets here in slab.c that
could become a function or so. So this could be improved upon by creating
a function called get_first_slab() or so.

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>