[PATCH 1/3] percpu: clean up of schunk->map[] assignment in pcpu_setup_first_chunk

[PATCH 1/3] percpu: clean up of schunk->map[] assignment in pcpu_setup_first_chunk

[Baoquan He]

The original assignment is a little redundent.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/percpu.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/mm/percpu.c b/mm/percpu.c
index 2dd7448..a63b4d8 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -1668,9 +1668,8 @@ int __init pcpu_setup_first_chunk(const struct pcpu_alloc_info *ai,
 	schunk->map[1] = ai->static_size;
 	schunk->map_used = 1;
 	if (schunk->free_size)
-		schunk->map[++schunk->map_used] = 1 | (ai->static_size + schunk->free_size);
-	else
-		schunk->map[1] |= 1;
+		schunk->map[++schunk->map_used] = ai->static_size + schunk->free_size;
+	schunk->map[schunk->map_used] |= 1;
 
 	/* init dynamic chunk if necessary */
 	if (dyn_size) {
-- 
1.9.3

[PATCH 2/3] perpuc: check pcpu_first_chunk and pcpu_reserved_chunk to avoid handling them twice

[Baoquan He]

In pcpu_setup_first_chunk() pcpu_reserved_chunk is assigned to point to
static chunk. While pcpu_first_chunk is got from below code:

	pcpu_first_chunk = dchunk ?: schunk;

Then it could point to static chunk too if dynamic chunk doesn't exist. So
in this patch adding a check in percpu_init_late() to see if pcpu_first_chunk
is equal to pcpu_reserved_chunk. Only if they are not equal we add
pcpu_reserved_chunk to the target array.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/percpu.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/mm/percpu.c b/mm/percpu.c
index a63b4d8..f7e02c0 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -2260,11 +2260,18 @@ void __init setup_per_cpu_areas(void)
 void __init percpu_init_late(void)
 {
 	struct pcpu_chunk *target_chunks[] =
-		{ pcpu_first_chunk, pcpu_reserved_chunk, NULL };
+		{ pcpu_first_chunk, NULL, NULL };
 	struct pcpu_chunk *chunk;
 	unsigned long flags;
 	int i;
 
+	/*
+	 * pcpu_first_chunk could be the same as pcpu_reserved_chunk, add it to the
+	 * target array only if pcpu_first_chunk is not equal to pcpu_reserved_chunk.
+	 */
+	if (pcpu_first_chunk != pcpu_reserved_chunk)
+		target_chunks[1] = pcpu_reserved_chunk;
+
 	for (i = 0; (chunk = target_chunks[i]); i++) {
 		int *map;
 		const size_t size = PERCPU_DYNAMIC_EARLY_SLOTS * sizeof(map[0]);
-- 
1.9.3

[PATCH 3/3] percpu: add macro PCPU_CHUNK_AREA_IN_USE

[Baoquan He]

chunk->map[] contains <offset|in-use flag> of each area. Now add a
new macro PCPU_CHUNK_AREA_IN_USE and use it as the in-use flag to
replace all magic number '1'.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/percpu.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/mm/percpu.c b/mm/percpu.c
index f7e02c0..2f99073 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -80,6 +80,7 @@
 #define PCPU_ATOMIC_MAP_MARGIN_HIGH	64
 #define PCPU_EMPTY_POP_PAGES_LOW	2
 #define PCPU_EMPTY_POP_PAGES_HIGH	4
+#define PCPU_CHUNK_AREA_IN_USE		1
 
 #ifdef CONFIG_SMP
 /* default addr <-> pcpu_ptr mapping, override in asm/percpu.h if necessary */
@@ -328,8 +329,8 @@ static void pcpu_mem_free(void *ptr, size_t size)
  */
 static int pcpu_count_occupied_pages(struct pcpu_chunk *chunk, int i)
 {
-	int off = chunk->map[i] & ~1;
-	int end = chunk->map[i + 1] & ~1;
+	int off = chunk->map[i] & ~PCPU_CHUNK_AREA_IN_USE;
+	int end = chunk->map[i + 1] & ~PCPU_CHUNK_AREA_IN_USE;
 
 	if (!PAGE_ALIGNED(off) && i > 0) {
 		int prev = chunk->map[i - 1];
@@ -340,7 +341,7 @@ static int pcpu_count_occupied_pages(struct pcpu_chunk *chunk, int i)
 
 	if (!PAGE_ALIGNED(end) && i + 1 < chunk->map_used) {
 		int next = chunk->map[i + 1];
-		int nend = chunk->map[i + 2] & ~1;
+		int nend = chunk->map[i + 2] & ~PCPU_CHUNK_AREA_IN_USE;
 
 		if (!(next & 1) && nend >= round_up(end, PAGE_SIZE))
 			end = round_up(end, PAGE_SIZE);
@@ -738,7 +739,7 @@ static struct pcpu_chunk *pcpu_alloc_chunk(void)
 
 	chunk->map_alloc = PCPU_DFL_MAP_ALLOC;
 	chunk->map[0] = 0;
-	chunk->map[1] = pcpu_unit_size | 1;
+	chunk->map[1] = pcpu_unit_size | PCPU_CHUNK_AREA_IN_USE;
 	chunk->map_used = 1;
 
 	INIT_LIST_HEAD(&chunk->list);
@@ -1664,12 +1665,12 @@ int __init pcpu_setup_first_chunk(const struct pcpu_alloc_info *ai,
 	}
 	schunk->contig_hint = schunk->free_size;
 
-	schunk->map[0] = 1;
+	schunk->map[0] = PCPU_CHUNK_AREA_IN_USE;
 	schunk->map[1] = ai->static_size;
 	schunk->map_used = 1;
 	if (schunk->free_size)
 		schunk->map[++schunk->map_used] = ai->static_size + schunk->free_size;
-	schunk->map[schunk->map_used] |= 1;
+	schunk->map[schunk->map_used] |= PCPU_CHUNK_AREA_IN_USE;
 
 	/* init dynamic chunk if necessary */
 	if (dyn_size) {
@@ -1684,9 +1685,10 @@ int __init pcpu_setup_first_chunk(const struct pcpu_alloc_info *ai,
 		dchunk->nr_populated = pcpu_unit_pages;
 
 		dchunk->contig_hint = dchunk->free_size = dyn_size;
-		dchunk->map[0] = 1;
+		dchunk->map[0] = PCPU_CHUNK_AREA_IN_USE;
 		dchunk->map[1] = pcpu_reserved_chunk_limit;
-		dchunk->map[2] = (pcpu_reserved_chunk_limit + dchunk->free_size) | 1;
+		dchunk->map[2] = (pcpu_reserved_chunk_limit + dchunk->free_size)
+					| PCPU_CHUNK_AREA_IN_USE;
 		dchunk->map_used = 2;
 	}
 
-- 
1.9.3

Re: [PATCH 1/3] percpu: clean up of schunk->map[] assignment in pcpu_setup_first_chunk

[Christoph Lameter]

On Mon, 20 Jul 2015, Baoquan He wrote:

> The original assignment is a little redundent.

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

Re: [PATCH 3/3] percpu: add macro PCPU_CHUNK_AREA_IN_USE

[Christoph Lameter]

On Mon, 20 Jul 2015, Baoquan He wrote:

> chunk->map[] contains <offset|in-use flag> of each area. Now add a
> new macro PCPU_CHUNK_AREA_IN_USE and use it as the in-use flag to
> replace all magic number '1'.

Hmmm... This is a bitflag and the code now looks like there is some sort
of bitmask that were are using. Use bitops or something else that clearly
implies that a bit is flipped instead?

Re: [PATCH 2/3] perpuc: check pcpu_first_chunk and pcpu_reserved_chunk to avoid handling them twice

[Tejun Heo]

On Mon, Jul 20, 2015 at 10:55:29PM +0800, Baoquan He wrote:
> In pcpu_setup_first_chunk() pcpu_reserved_chunk is assigned to point to
> static chunk. While pcpu_first_chunk is got from below code:
> 
> 	pcpu_first_chunk = dchunk ?: schunk;
> 
> Then it could point to static chunk too if dynamic chunk doesn't exist. So
> in this patch adding a check in percpu_init_late() to see if pcpu_first_chunk
> is equal to pcpu_reserved_chunk. Only if they are not equal we add
> pcpu_reserved_chunk to the target array.

So, I don't think this is actually possible.  dyn_size can't be zero
so if reserved chunk is created, dyn chunk is also always created and
thus first chunk can't equal reserved chunk.  It might be useful to
add some comments explaining this or maybe WARN_ON() but I don't think
this path is necessary.

Thanks.

-- 
tejun

Re: [PATCH 3/3] percpu: add macro PCPU_CHUNK_AREA_IN_USE

[Tejun Heo]

On Mon, Jul 20, 2015 at 10:55:30PM +0800, Baoquan He wrote:
> chunk->map[] contains <offset|in-use flag> of each area. Now add a
> new macro PCPU_CHUNK_AREA_IN_USE and use it as the in-use flag to
> replace all magic number '1'.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>

idk, maybe.  Can you at least go for something shorter?  PCPU_MAP_BUSY
or whatever?

-- 
tejun

Re: [PATCH 1/3] percpu: clean up of schunk->map[] assignment in pcpu_setup_first_chunk

[Tejun Heo]

On Mon, Jul 20, 2015 at 10:55:28PM +0800, Baoquan He wrote:
> The original assignment is a little redundent.
> 
> Signed-off-by: Baoquan He <bhe@redhat.com>

Heh, I'm not sure this is actually better.  Anyways, applied to
percpu/for-4.3.  In general tho, I don't really think this level of
micro cleanup patches are worthwhile.  If something around it changes,
sure, take the chance and clean it up but as standalone patches these
aren't that readily justifiable.

Thanks.

-- 
tejun

Re: [PATCH 2/3] perpuc: check pcpu_first_chunk and pcpu_reserved_chunk to avoid handling them twice

[Baoquan He]

Hi Tejun,

On 07/21/15 at 11:28am, Tejun Heo wrote:
> On Mon, Jul 20, 2015 at 10:55:29PM +0800, Baoquan He wrote:
> > In pcpu_setup_first_chunk() pcpu_reserved_chunk is assigned to point to
> > static chunk. While pcpu_first_chunk is got from below code:
> > 
> > 	pcpu_first_chunk = dchunk ?: schunk;
> > 
> > Then it could point to static chunk too if dynamic chunk doesn't exist. So
> > in this patch adding a check in percpu_init_late() to see if pcpu_first_chunk
> > is equal to pcpu_reserved_chunk. Only if they are not equal we add
> > pcpu_reserved_chunk to the target array.
> 
> So, I don't think this is actually possible.  dyn_size can't be zero
> so if reserved chunk is created, dyn chunk is also always created and
> thus first chunk can't equal reserved chunk.  It might be useful to
> add some comments explaining this or maybe WARN_ON() but I don't think
> this path is necessary.

Thanks for your reviewing.

Yes, dyn_size can't be zero. But in pcpu_setup_first_chunk(), the local
variable dyn_size could be zero caused by below code:

if (ai->reserved_size) {
                schunk->free_size = ai->reserved_size;
                pcpu_reserved_chunk = schunk;
                pcpu_reserved_chunk_limit = ai->static_size +
ai->reserved_size;
        } else {
                schunk->free_size = dyn_size;
                dyn_size = 0;                   /* dynamic area covered
*/
        }

So if no reserved_size dyn_size is assigned to zero, and is checked to
see if dchunk need be created in below code:
	/* init dynamic chunk if necessary */
        if (dyn_size) {
		...
	}

I think v1 patch is a little ugly, so made a v2 like this:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>From 0f22f04878f0779e4d9e66ae24f9bfc5321782c2 Mon Sep 17 00:00:00 2001
From: Baoquan He <bhe@redhat.com>
Date: Sun, 12 Jul 2015 19:33:26 +0800
Subject: [PATCH] perpuc: check pcpu_first_chunk and pcpu_reserved_chunk to
 avoid handling them twice

In pcpu_setup_first_chunk() pcpu_reserved_chunk is assigned to point to
static chunk. While pcpu_first_chunk is got from below code:

	pcpu_first_chunk = dchunk ?: schunk;

Then it could point to static chunk too if dynamic chunk doesn't exist.

In this patch add a new helper function percpu_install_map() to replace
the old map of chunk with newly allocated map. Then call percpu_install_map()
separately in percpu_init_late() if pcpu_first_chunk and pcpu_reserved_chunk
exist.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/percpu.c | 39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)

diff --git a/mm/percpu.c b/mm/percpu.c
index a63b4d8..9d0f9f6 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -2251,6 +2251,23 @@ void __init setup_per_cpu_areas(void)
 
 #endif	/* CONFIG_SMP */
 
+static void __init percpu_install_map(struct pcpu_chunk * chunk)
+{
+	int *map;
+	unsigned long flags;
+	const size_t size = PERCPU_DYNAMIC_EARLY_SLOTS * sizeof(map[0]);
+
+	BUILD_BUG_ON(size > PAGE_SIZE);
+
+	map = pcpu_mem_zalloc(size);
+	BUG_ON(!map);
+
+	spin_lock_irqsave(&pcpu_lock, flags);
+	memcpy(map, chunk->map, size);
+	chunk->map = map;
+	spin_unlock_irqrestore(&pcpu_lock, flags);
+}
+
 /*
  * First and reserved chunks are initialized with temporary allocation
  * map in initdata so that they can be used before slab is online.
@@ -2259,26 +2276,10 @@ void __init setup_per_cpu_areas(void)
  */
 void __init percpu_init_late(void)
 {
-	struct pcpu_chunk *target_chunks[] =
-		{ pcpu_first_chunk, pcpu_reserved_chunk, NULL };
-	struct pcpu_chunk *chunk;
-	unsigned long flags;
-	int i;
-
-	for (i = 0; (chunk = target_chunks[i]); i++) {
-		int *map;
-		const size_t size = PERCPU_DYNAMIC_EARLY_SLOTS * sizeof(map[0]);
+	percpu_install_map(pcpu_first_chunk);
 
-		BUILD_BUG_ON(size > PAGE_SIZE);
-
-		map = pcpu_mem_zalloc(size);
-		BUG_ON(!map);
-
-		spin_lock_irqsave(&pcpu_lock, flags);
-		memcpy(map, chunk->map, size);
-		chunk->map = map;
-		spin_unlock_irqrestore(&pcpu_lock, flags);
-	}
+	if (pcpu_first_chunk != pcpu_reserved_chunk)
+		percpu_install_map(pcpu_reserved_chunk);
 }
 
 /*
-- 
1.9.3

Re: [PATCH 3/3] percpu: add macro PCPU_CHUNK_AREA_IN_USE

[Baoquan He]

Hi Christoph,

On 07/20/15 at 10:35am, Christoph Lameter wrote:
> On Mon, 20 Jul 2015, Baoquan He wrote:
> 
> > chunk->map[] contains <offset|in-use flag> of each area. Now add a
> > new macro PCPU_CHUNK_AREA_IN_USE and use it as the in-use flag to
> > replace all magic number '1'.
> 
> Hmmm... This is a bitflag and the code now looks like there is some sort
> of bitmask that were are using. Use bitops or something else that clearly
> implies that a bit is flipped instead?

Thanks for your reviewing and suggesting.

I tried your suggestion and changed to use set_bit/clear_bit to do
instead. It's like this:

@@ -328,8 +329,10 @@ static void pcpu_mem_free(void *ptr, size_t size)
  */
 static int pcpu_count_occupied_pages(struct pcpu_chunk *chunk, int i)
 {
-       int off = chunk->map[i] & ~1;
-       int end = chunk->map[i + 1] & ~1;
+       int off = chunk->map[i];
+       int end = chunk->map[i + 1];
+       clear_bit(PCPU_CHUNK_AREA_IN_USE_BIT, &chunk->map[i]);
+       clear_bit(PCPU_CHUNK_AREA_IN_USE_BIT, &chunk->map[i + 1]);

Looks like code becomes a little redundent. If several different bits in
chunk->map[] have different usage and need several different flags,
bitops maybe better. While now only the lowest bit need be handle, use
bitops kindof too much and can make code a little messy.

You and Tejun may be a little struggled on this change since it make
code longer. Tejun has suggested that at least use a shorter name, like
PCPU_MAP_BUSY. I am going to post v2 to see if it's better.

Thanks
Baoquan

Re: [PATCH 3/3] percpu: add macro PCPU_CHUNK_AREA_IN_USE

[Baoquan He]

Hi Tejun,

On 07/21/15 at 11:30am, Tejun Heo wrote:
> On Mon, Jul 20, 2015 at 10:55:30PM +0800, Baoquan He wrote:
> > chunk->map[] contains <offset|in-use flag> of each area. Now add a
> > new macro PCPU_CHUNK_AREA_IN_USE and use it as the in-use flag to
> > replace all magic number '1'.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> 
> idk, maybe.  Can you at least go for something shorter?  PCPU_MAP_BUSY
> or whatever?

Thanks for suggestion.

I know this change makes code longer. PCPU_MAP_BUSY is better, I am gonna
repost with it.

Thanks
Baoquan

Re: [PATCH 1/3] percpu: clean up of schunk->map[] assignment in pcpu_setup_first_chunk

[Baoquan He]

On 07/21/15 at 11:33am, Tejun Heo wrote:
> On Mon, Jul 20, 2015 at 10:55:28PM +0800, Baoquan He wrote:
> > The original assignment is a little redundent.
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> 
> Heh, I'm not sure this is actually better.  Anyways, applied to
> percpu/for-4.3.  In general tho, I don't really think this level of
> micro cleanup patches are worthwhile.  If something around it changes,
> sure, take the chance and clean it up but as standalone patches these
> aren't that readily justifiable.

Understood. They are very tiny cleanups, not inprovement. Just when
trying to fix a kdump corrupted header bug where cpu information is
stored in percpu variable I tried to understand the whole percpu
implementation and found these. Didn't put them together because that
change is kdump only in kernel/kexec.c and that patch is testing by
customers on big server. Understanding percpu code is always in my
TODO list, now it's done. I am fine if patch like patch 3/3 makes code
messy and should not be applied.

Thanks for your reviewing and suggestion.

Thanks
Baoquan

Re: [PATCH 2/3] perpuc: check pcpu_first_chunk and pcpu_reserved_chunk to avoid handling them twice

[Tejun Heo]

Hello,

On Wed, Jul 22, 2015 at 08:03:57AM +0800, Baoquan He wrote:
> Yes, dyn_size can't be zero. But in pcpu_setup_first_chunk(), the local
> variable dyn_size could be zero caused by below code:
> 
> if (ai->reserved_size) {
>                 schunk->free_size = ai->reserved_size;
>                 pcpu_reserved_chunk = schunk;
>                 pcpu_reserved_chunk_limit = ai->static_size +
> ai->reserved_size;
>         } else {
>                 schunk->free_size = dyn_size;
>                 dyn_size = 0;                   /* dynamic area covered
> */
>         }
> 
> So if no reserved_size dyn_size is assigned to zero, and is checked to
> see if dchunk need be created in below code:

Hmmm... but then pcpu_reserved_chunk is NULL so there still is no
duplicate on the list, no?

Thanks.

-- 
tejun

Re: [PATCH 3/3] percpu: add macro PCPU_CHUNK_AREA_IN_USE

[Tejun Heo]

Hello,

On Wed, Jul 22, 2015 at 08:28:39AM +0800, Baoquan He wrote:
> I know this change makes code longer. PCPU_MAP_BUSY is better, I am gonna
> repost with it.

While at it, can you also please add comment on top of the definition
of PCPU_MAP_BUSY explaining what's going on?

Thanks.

-- 
tejun

Re: [PATCH 2/3] perpuc: check pcpu_first_chunk and pcpu_reserved_chunk to avoid handling them twice

[Baoquan He]

On 07/22/15 at 09:52am, Tejun Heo wrote:
> Hello,
> 
> On Wed, Jul 22, 2015 at 08:03:57AM +0800, Baoquan He wrote:
> > Yes, dyn_size can't be zero. But in pcpu_setup_first_chunk(), the local
> > variable dyn_size could be zero caused by below code:
> > 
> > if (ai->reserved_size) {
> >                 schunk->free_size = ai->reserved_size;
> >                 pcpu_reserved_chunk = schunk;
> >                 pcpu_reserved_chunk_limit = ai->static_size +
> > ai->reserved_size;
> >         } else {
> >                 schunk->free_size = dyn_size;
> >                 dyn_size = 0;                   /* dynamic area covered
> > */
> >         }
> > 
> > So if no reserved_size dyn_size is assigned to zero, and is checked to
> > see if dchunk need be created in below code:
> 
> Hmmm... but then pcpu_reserved_chunk is NULL so there still is no
> duplicate on the list, no?

Yes, you are quite right. I was mistaken. So NACK this patch.

Thanks a lot.

Re: [PATCH 2/3] perpuc: check pcpu_first_chunk and pcpu_reserved_chunk to avoid handling them twice

[Tejun Heo]

On Wed, Jul 22, 2015 at 10:29:00PM +0800, Baoquan He wrote:
> > > So if no reserved_size dyn_size is assigned to zero, and is checked to
> > > see if dchunk need be created in below code:
> > 
> > Hmmm... but then pcpu_reserved_chunk is NULL so there still is no
> > duplicate on the list, no?
> 
> Yes, you are quite right. I was mistaken. So NACK this patch.

But, yeah, it'd be great if we can add a WARN_ON() to ensure that this
really doesn't happen along with some comments.

Thanks!

-- 
tejun

[PATCH v2 3/3] percpu: add macro PCPU_MAP_BUSY

[Baoquan He]

chunk->map[] contains <offset|in-use flag> of each area. Now add a
new macro PCPU_MAP_BUSY and use it as the in-use flag to replace all
magic number '1'.

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/percpu.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/mm/percpu.c b/mm/percpu.c
index a63b4d8..8cf18dc 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -81,6 +81,15 @@
 #define PCPU_EMPTY_POP_PAGES_LOW	2
 #define PCPU_EMPTY_POP_PAGES_HIGH	4
 
+/* we use int array chunk->map[] to describe each area of chunk. Each array
+ * element is represented by one int - contains offset|1 for <offset, in use>
+ * or offset for <ofset, free> (offset need be guaranteed to be even). In the
+ * end there's a sentry entry - <total size, in-use>. So the size of the N-th
+ * area would be the offset of (N+1)-th - the offset of N-th, namely
+ * SIZEn = chunk->map[N+1]&~1 - chunk->map[N]&~1
+ * For more read-able code define PCPU_MAP_BUSY to represent in-use flag.*/
+#define PCPU_MAP_BUSY			1
+
 #ifdef CONFIG_SMP
 /* default addr <-> pcpu_ptr mapping, override in asm/percpu.h if necessary */
 #ifndef __addr_to_pcpu_ptr
@@ -328,8 +337,8 @@ static void pcpu_mem_free(void *ptr, size_t size)
  */
 static int pcpu_count_occupied_pages(struct pcpu_chunk *chunk, int i)
 {
-	int off = chunk->map[i] & ~1;
-	int end = chunk->map[i + 1] & ~1;
+	int off = chunk->map[i] & ~PCPU_MAP_BUSY;
+	int end = chunk->map[i + 1] & ~PCPU_MAP_BUSY;
 
 	if (!PAGE_ALIGNED(off) && i > 0) {
 		int prev = chunk->map[i - 1];
@@ -340,7 +349,7 @@ static int pcpu_count_occupied_pages(struct pcpu_chunk *chunk, int i)
 
 	if (!PAGE_ALIGNED(end) && i + 1 < chunk->map_used) {
 		int next = chunk->map[i + 1];
-		int nend = chunk->map[i + 2] & ~1;
+		int nend = chunk->map[i + 2] & ~PCPU_MAP_BUSY;
 
 		if (!(next & 1) && nend >= round_up(end, PAGE_SIZE))
 			end = round_up(end, PAGE_SIZE);
@@ -738,7 +747,7 @@ static struct pcpu_chunk *pcpu_alloc_chunk(void)
 
 	chunk->map_alloc = PCPU_DFL_MAP_ALLOC;
 	chunk->map[0] = 0;
-	chunk->map[1] = pcpu_unit_size | 1;
+	chunk->map[1] = pcpu_unit_size | PCPU_MAP_BUSY;
 	chunk->map_used = 1;
 
 	INIT_LIST_HEAD(&chunk->list);
@@ -1664,12 +1673,12 @@ int __init pcpu_setup_first_chunk(const struct pcpu_alloc_info *ai,
 	}
 	schunk->contig_hint = schunk->free_size;
 
-	schunk->map[0] = 1;
+	schunk->map[0] = PCPU_MAP_BUSY;
 	schunk->map[1] = ai->static_size;
 	schunk->map_used = 1;
 	if (schunk->free_size)
 		schunk->map[++schunk->map_used] = ai->static_size + schunk->free_size;
-	schunk->map[schunk->map_used] |= 1;
+	schunk->map[schunk->map_used] |= PCPU_MAP_BUSY;
 
 	/* init dynamic chunk if necessary */
 	if (dyn_size) {
@@ -1684,9 +1693,10 @@ int __init pcpu_setup_first_chunk(const struct pcpu_alloc_info *ai,
 		dchunk->nr_populated = pcpu_unit_pages;
 
 		dchunk->contig_hint = dchunk->free_size = dyn_size;
-		dchunk->map[0] = 1;
+		dchunk->map[0] = PCPU_MAP_BUSY;
 		dchunk->map[1] = pcpu_reserved_chunk_limit;
-		dchunk->map[2] = (pcpu_reserved_chunk_limit + dchunk->free_size) | 1;
+		dchunk->map[2] = (pcpu_reserved_chunk_limit + dchunk->free_size)
+					| PCPU_MAP_BUSY;
 		dchunk->map_used = 2;
 	}
 
-- 
2.4.3

[PATCH v3 2/3] percpu: Add WARN_ON into percpu_init_late

[Baoquan He]

In pcpu_setup_first_chunk() pcpu_reserved_chunk is assigned to point to
static chunk. While pcpu_first_chunk is got from below code:

	pcpu_first_chunk = dchunk ?: schunk;

pcpu_first_chunk might point to static chunk too with possibility. Add a
WARN_ON here to yell out if that happened.*/

Signed-off-by: Baoquan He <bhe@redhat.com>
---
 mm/percpu.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/mm/percpu.c b/mm/percpu.c
index 8cf18dc..974600b 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -2275,6 +2275,13 @@ void __init percpu_init_late(void)
 	unsigned long flags;
 	int i;
 
+	/* In pcpu_setup_first_chunk() pcpu_reserved_chunk is assigned to point to
+	 * static chunk. While pcpu_first_chunk is got from below code:
+	 * 		pcpu_first_chunk = dchunk ?: schunk;
+	 * pcpu_first_chunk might point to static chunk too with possibility. Add a
+	 * WARN_ON here to yell out if that happened.*/
+	WARN_ON(pcpu_first_chunk == pcpu_reserved_chunk);
+
 	for (i = 0; (chunk = target_chunks[i]); i++) {
 		int *map;
 		const size_t size = PERCPU_DYNAMIC_EARLY_SLOTS * sizeof(map[0]);
-- 
2.4.3

Re: [PATCH 3/3] percpu: add macro PCPU_CHUNK_AREA_IN_USE

[Baoquan He]

On 07/22/15 at 09:53am, Tejun Heo wrote:
> Hello,
> 
> On Wed, Jul 22, 2015 at 08:28:39AM +0800, Baoquan He wrote:
> > I know this change makes code longer. PCPU_MAP_BUSY is better, I am gonna
> > repost with it.
> 
> While at it, can you also please add comment on top of the definition
> of PCPU_MAP_BUSY explaining what's going on?

Posted [patch v2 3/3] as you suggested.

Thanks a lot.

Re: [PATCH 2/3] perpuc: check pcpu_first_chunk and pcpu_reserved_chunk to avoid handling them twice

[Baoquan He]

On 07/22/15 at 10:37am, Tejun Heo wrote:
> On Wed, Jul 22, 2015 at 10:29:00PM +0800, Baoquan He wrote:
> > > > So if no reserved_size dyn_size is assigned to zero, and is checked to
> > > > see if dchunk need be created in below code:
> > > 
> > > Hmmm... but then pcpu_reserved_chunk is NULL so there still is no
> > > duplicate on the list, no?
> > 
> > Yes, you are quite right. I was mistaken. So NACK this patch.
> 
> But, yeah, it'd be great if we can add a WARN_ON() to ensure that this
> really doesn't happen along with some comments.

Posted [patch v3 2/3] as you suggested.

Thanks a lot!