Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1281921 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/3] mm/slab: use list_first_entry_or_null() Geliang Tang <geliangtang@163.com> - 2015-12-02 16:50 +0100
    [PATCH 2/3] mm/slab: use list_for_each_entry in cache_flusharray Geliang Tang <geliangtang@163.com> - 2015-12-02 16:50 +0100
      Re: [PATCH 2/3] mm/slab: use list_for_each_entry in  cache_flusharray Christoph Lameter <cl@linux.com> - 2015-12-02 17:00 +0100
    [PATCH 3/3] mm/slab: use list_{empty_careful,last_entry} in drain_freelist Geliang Tang <geliangtang@163.com> - 2015-12-02 16:50 +0100
      Re: [PATCH 3/3] mm/slab: use list_{empty_careful,last_entry} in  drain_freelist Christoph Lameter <cl@linux.com> - 2015-12-02 17:10 +0100
        [PATCH v2] mm/slab.c: use list_{empty_careful,last_entry} in drain_freelist Geliang Tang <geliangtang@163.com> - 2015-12-03 15:10 +0100
          Re: [PATCH v2] mm/slab.c: use list_{empty_careful,last_entry} in  drain_freelist Christoph Lameter <cl@linux.com> - 2015-12-03 16:00 +0100
            Re: [PATCH v2] mm/slab.c: use list_{empty_careful,last_entry} in  drain_freelist Geliang Tang <geliangtang@163.com> - 2015-12-04 14:50 +0100
              Re: [PATCH v2] mm/slab.c: use list_{empty_careful,last_entry} in  drain_freelist Christoph Lameter <cl@linux.com> - 2015-12-04 17:20 +0100
                Re: [PATCH v2] mm/slab.c: use list_{empty_careful,last_entry} in  drain_freelist Geliang Tang <geliangtang@163.com> - 2015-12-05 03:40 +0100
    Re: [PATCH 1/3] mm/slab: use list_first_entry_or_null() Christoph Lameter <cl@linux.com> - 2015-12-02 17:00 +0100

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

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1281943 — Re: [PATCH 2/3] mm/slab: use list_for_each_entry in cache_flusharray

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1281948

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web