[PATCH 1/3] mm, slub: not retrieve cpu_slub again in new_slab_objects()

[PATCH 1/3] mm, slub: not retrieve cpu_slub again in new_slab_objects()

[Wei Yang]

In current code, the following context always meets:

  local_irq_save/disable()
    ___slab_alloc()
      new_slab_objects()
  local_irq_restore/enable()

This context ensures cpu will continue running until it finish this job
before yield its control, which means the cpu_slab retrieved in
new_slab_objects() is the same as passed in.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
 mm/slub.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index ce2b9e5cea77..11e49d95e0ac 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2402,10 +2402,9 @@ slab_out_of_memory(struct kmem_cache *s, gfp_t gfpflags, int nid)
 }
 
 static inline void *new_slab_objects(struct kmem_cache *s, gfp_t flags,
-			int node, struct kmem_cache_cpu **pc)
+			int node, struct kmem_cache_cpu *c)
 {
 	void *freelist;
-	struct kmem_cache_cpu *c = *pc;
 	struct page *page;
 
 	WARN_ON_ONCE(s->ctor && (flags & __GFP_ZERO));
@@ -2417,7 +2416,6 @@ static inline void *new_slab_objects(struct kmem_cache *s, gfp_t flags,
 
 	page = new_slab(s, flags, node);
 	if (page) {
-		c = raw_cpu_ptr(s->cpu_slab);
 		if (c->page)
 			flush_slab(s, c);
 
@@ -2430,7 +2428,6 @@ static inline void *new_slab_objects(struct kmem_cache *s, gfp_t flags,
 
 		stat(s, ALLOC_SLAB);
 		c->page = page;
-		*pc = c;
 	} else
 		freelist = NULL;
 
@@ -2567,7 +2564,7 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
 		goto redo;
 	}
 
-	freelist = new_slab_objects(s, gfpflags, node, &c);
+	freelist = new_slab_objects(s, gfpflags, node, c);
 
 	if (unlikely(!freelist)) {
 		slab_out_of_memory(s, gfpflags, node);
-- 
2.15.1

[PATCH 2/3] mm, slub: unify access to s->cpu_slab by replacing raw_cpu_ptr() with this_cpu_ptr()

[Wei Yang]

In current code, we use two forms to access s->cpu_slab

  * raw_cpu_ptr()
  * this_cpu_ptr()

This patch unify the access by replacing raw_cpu_ptr() with
this_cpu_ptr().

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
 mm/slub.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/slub.c b/mm/slub.c
index 11e49d95e0ac..715372a786e3 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2643,7 +2643,7 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s,
 	 */
 	do {
 		tid = this_cpu_read(s->cpu_slab->tid);
-		c = raw_cpu_ptr(s->cpu_slab);
+		c = this_cpu_ptr(s->cpu_slab);
 	} while (IS_ENABLED(CONFIG_PREEMPT) &&
 		 unlikely(tid != READ_ONCE(c->tid)));
 
@@ -2916,7 +2916,7 @@ static __always_inline void do_slab_free(struct kmem_cache *s,
 	 */
 	do {
 		tid = this_cpu_read(s->cpu_slab->tid);
-		c = raw_cpu_ptr(s->cpu_slab);
+		c = this_cpu_ptr(s->cpu_slab);
 	} while (IS_ENABLED(CONFIG_PREEMPT) &&
 		 unlikely(tid != READ_ONCE(c->tid)));
 
-- 
2.15.1

Re: [PATCH 2/3] mm, slub: unify access to s->cpu_slab by replacing raw_cpu_ptr() with this_cpu_ptr()

[Christopher Lameter]

On Thu, 25 Oct 2018, Wei Yang wrote:

> In current code, we use two forms to access s->cpu_slab
>
>   * raw_cpu_ptr()
>   * this_cpu_ptr()

Ok the only difference is that for CONFIG_DEBUG_PREEMPT we will do the
debug checks twice.

That tolerable I think but is this really a worthwhile change?

Re: [PATCH 2/3] mm, slub: unify access to s->cpu_slab by replacing raw_cpu_ptr() with this_cpu_ptr()

[Wei Yang]

On Thu, Oct 25, 2018 at 01:53:06PM +0000, Christopher Lameter wrote:
>On Thu, 25 Oct 2018, Wei Yang wrote:
>
>> In current code, we use two forms to access s->cpu_slab
>>
>>   * raw_cpu_ptr()
>>   * this_cpu_ptr()
>
>Ok the only difference is that for CONFIG_DEBUG_PREEMPT we will do the
>debug checks twice.
>
>That tolerable I think but is this really a worthwhile change?

Agree.

My purpose is to make unify the access, looks easy for me to read the
code.

You can decide whether to change this or not :-)

-- 
Wei Yang
Help you, Help me

[PATCH 3/3] mm, slub: make the comment of put_cpu_partial() complete

[Wei Yang]

There are two cases when put_cpu_partial() is invoked.

    * __slab_free
    * get_partial_node

This patch just makes it cover these two cases and fix one typo in
slub_def.h.

Signed-off-by: Wei Yang <richard.weiyang@gmail.com>
---
 include/linux/slub_def.h | 2 +-
 mm/slub.c                | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/slub_def.h b/include/linux/slub_def.h
index 3a1a1dbc6f49..201a635be846 100644
--- a/include/linux/slub_def.h
+++ b/include/linux/slub_def.h
@@ -81,7 +81,7 @@ struct kmem_cache_order_objects {
  */
 struct kmem_cache {
 	struct kmem_cache_cpu __percpu *cpu_slab;
-	/* Used for retriving partial slabs etc */
+	/* Used for retrieving partial slabs etc */
 	slab_flags_t flags;
 	unsigned long min_partial;
 	unsigned int size;	/* The size of an object including meta data */
diff --git a/mm/slub.c b/mm/slub.c
index 715372a786e3..3db6ce58e92e 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -2201,8 +2201,8 @@ static void unfreeze_partials(struct kmem_cache *s,
 }
 
 /*
- * Put a page that was just frozen (in __slab_free) into a partial page
- * slot if available.
+ * Put a page that was just frozen (in __slab_free|get_partial_node) into a
+ * partial page slot if available.
  *
  * If we did not find a slot then simply move all the partials to the
  * per node partial list.
-- 
2.15.1

Re: [PATCH 3/3] mm, slub: make the comment of put_cpu_partial() complete

[Christopher Lameter]

Acked-by: Christoph Lameter <cl@linux.com>

Re: [PATCH 3/3] mm, slub: make the comment of put_cpu_partial() complete

[Wei Yang]

On Thu, Oct 25, 2018 at 01:41:58PM +0000, Christopher Lameter wrote:
>
>Acked-by: Christoph Lameter <cl@linux.com>

Andrew,

Do you like this one?

-- 
Wei Yang
Help you, Help me

Re: [PATCH 1/3] mm, slub: not retrieve cpu_slub again in new_slab_objects()

[Christopher Lameter]

On Thu, 25 Oct 2018, Wei Yang wrote:

> In current code, the following context always meets:
>
>   local_irq_save/disable()
>     ___slab_alloc()
>       new_slab_objects()
>   local_irq_restore/enable()
>
> This context ensures cpu will continue running until it finish this job
> before yield its control, which means the cpu_slab retrieved in
> new_slab_objects() is the same as passed in.

Interrupts can be switched on in new_slab() since it goes to the page
allocator. See allocate_slab().

This means that the percpu slab may change.

Re: [PATCH 1/3] mm, slub: not retrieve cpu_slub again in new_slab_objects()

[Wei Yang]

On Thu, Oct 25, 2018 at 01:46:49PM +0000, Christopher Lameter wrote:
>On Thu, 25 Oct 2018, Wei Yang wrote:
>
>> In current code, the following context always meets:
>>
>>   local_irq_save/disable()
>>     ___slab_alloc()
>>       new_slab_objects()
>>   local_irq_restore/enable()
>>
>> This context ensures cpu will continue running until it finish this job
>> before yield its control, which means the cpu_slab retrieved in
>> new_slab_objects() is the same as passed in.
>
>Interrupts can be switched on in new_slab() since it goes to the page
>allocator. See allocate_slab().
>
>This means that the percpu slab may change.

Ah, you are right, thank :-)

-- 
Wei Yang
Help you, Help me

Re: [PATCH 1/3] mm, slub: not retrieve cpu_slub again in new_slab_objects()

[Wei Yang]

Hi, Christopher

I got one confusion on understanding one case in __slab_free().

The case is     : (new.frozen && !was_frozen)
My confusion is : Is it possible for the page to be on the full list?

This case (new.frozen && !was_frozen) happens when (!prior && !was_frozen).

  * !prior means this page is full
  * !was_frozen means this page is not in cpu_slab->page/partial

There are two cases to lead to (!prior && !was_frozen):

  * in get_freelist(), when page is full
  * in deactivate_slab(), when page is full

The first case will have a page in no list.
The second case will have a page in no list, or the page is put into
full list if SLUB_DEBUG is configured.

Do I miss something?

On Thu, Oct 25, 2018 at 01:46:49PM +0000, Christopher Lameter wrote:
>On Thu, 25 Oct 2018, Wei Yang wrote:
>
>> In current code, the following context always meets:
>>
>>   local_irq_save/disable()
>>     ___slab_alloc()
>>       new_slab_objects()
>>   local_irq_restore/enable()
>>
>> This context ensures cpu will continue running until it finish this job
>> before yield its control, which means the cpu_slab retrieved in
>> new_slab_objects() is the same as passed in.
>
>Interrupts can be switched on in new_slab() since it goes to the page
>allocator. See allocate_slab().
>
>This means that the percpu slab may change.

-- 
Wei Yang
Help you, Help me