[Patch] percpu: remove two suspicious break statements

[Patch] percpu: remove two suspicious break statements

[Amerigo Wang]

These two break statements seem to be very suspicious,
they are at the end of the statements inside the loop.
Remove them.

Signed-off-by: WANG Cong <amwang@redhat.com>
Cc: Tejun Heo <tj@kernel.org>

---
diff --git a/mm/percpu.c b/mm/percpu.c
index 5adfc26..dbcfee8 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -917,7 +917,6 @@ static void pcpu_depopulate_chunk(struct pcpu_chunk *chunk, int off, int size)
 	pcpu_for_each_unpop_region(chunk, rs, re, page_start, page_end) {
 		if (rs == page_start && re == page_end)
 			return;
-		break;
 	}
 
 	/* immutable chunks can't be depopulated */
@@ -972,7 +971,6 @@ static int pcpu_populate_chunk(struct pcpu_chunk *chunk, int off, int size)
 	pcpu_for_each_pop_region(chunk, rs, re, page_start, page_end) {
 		if (rs == page_start && re == page_end)
 			goto clear;
-		break;
 	}
 
 	/* need to allocate and map pages, this chunk can't be immutable */

Re: [Patch] percpu: remove two suspicious break statements

[Tejun Heo]

On 11/30/2009 06:12 PM, Amerigo Wang wrote:
> These two break statements seem to be very suspicious,
> they are at the end of the statements inside the loop.
> Remove them.
> 
> Signed-off-by: WANG Cong <amwang@redhat.com>
> Cc: Tejun Heo <tj@kernel.org>

That actually is correct code.  It's checking whether the first
iteration covers the whole thing.  I thought the comment there

  /* quick path, check whether all pages are already there */

explained it but looking at it again, it definitely isn't enough
unless you already know what's going on there.  Can you please post a
patch to add the comment?

Thanks.

-- 
tejun

Re: [Patch] percpu: remove two suspicious break statements

[Christoph Lameter]

On Mon, 30 Nov 2009, Tejun Heo wrote:

> That actually is correct code.  It's checking whether the first
> iteration covers the whole thing.  I thought the comment there

A loop statement that never creates a loop? You either execute a goto or a break.

Some more elaborate documentation is needed.

[PATCH] percpu: explain quick paths in pcpu_[de]populate_chunk()

[Tejun Heo]

pcpu_[de]populate_chunk() check whether there's actually any work to
do at the beginning and exit early if not.  This checking is done by
seeing whether the first iteration of pcpu_for_each_[un]pop_region()
covers the whole requested region.  The resulting code is a bit
unusual in that it's loop-like but never loops which apparently
confuses people.  Add comments to explain it.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Amerigo Wang <amwang@redhat.com>
Cc: Christoph Lameter <cl@linux-foundation.org>
---
Added to percpu#for-next.  This should be clear enough, right?

 mm/percpu.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/mm/percpu.c b/mm/percpu.c
index 442010c..c264315 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -912,10 +912,15 @@ static void pcpu_depopulate_chunk(struct pcpu_chunk *chunk, int off, int size)
 	unsigned long *populated;
 	int rs, re;
 
-	/* quick path, check whether it's empty already */
+	/*
+	 * Quick path, check whether it's already empty.  If the
+	 * region is completely empty, the first iteration will cover
+	 * the whole region.
+	 */
 	pcpu_for_each_unpop_region(chunk, rs, re, page_start, page_end) {
 		if (rs == page_start && re == page_end)
 			return;
+		/* it didn't cover the whole thing, break to slow path */
 		break;
 	}
 
@@ -967,10 +972,15 @@ static int pcpu_populate_chunk(struct pcpu_chunk *chunk, int off, int size)
 	unsigned int cpu;
 	int rs, re, rc;
 
-	/* quick path, check whether all pages are already there */
+	/*
+	 * Quick path, check whether all pages are already there.  If
+	 * the region is fully populated, the first iteration will
+	 * cover the whole region.
+	 */
 	pcpu_for_each_pop_region(chunk, rs, re, page_start, page_end) {
 		if (rs == page_start && re == page_end)
 			goto clear;
+		/* it didn't cover the whole thing, break to slow path */
 		break;
 	}
 
-- 
1.6.4.2

Re: [PATCH] percpu: explain quick paths in pcpu_[de]populate_chunk()

[Cong Wang]

Tejun Heo wrote:
> pcpu_[de]populate_chunk() check whether there's actually any work to
> do at the beginning and exit early if not.  This checking is done by
> seeing whether the first iteration of pcpu_for_each_[un]pop_region()
> covers the whole requested region.  The resulting code is a bit
> unusual in that it's loop-like but never loops which apparently
> confuses people.  Add comments to explain it.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Amerigo Wang <amwang@redhat.com>
> Cc: Christoph Lameter <cl@linux-foundation.org>
> ---
> Added to percpu#for-next.  This should be clear enough, right?
> 

Nope, comments can never fix bad code.

Since these two break statements are intentional, why not use if?
Logically, the following two are equalent.

for(a1; a2; a3){
     if (a4)
        return;
     break;
}


a1;
if (a2) {
    if (a4)
        return;
}


And the latter is much more readable than the former.

Re: [PATCH] percpu: explain quick paths in pcpu_[de]populate_chunk()

[Tejun Heo]

Hello,

On 12/01/2009 11:02 AM, Cong Wang wrote:
> Nope, comments can never fix bad code.
> 
> Since these two break statements are intentional, why not use if?
> Logically, the following two are equalent.
> 
> for(a1; a2; a3){
>     if (a4)
>        return;
>     break;
> }
> 
> 
> a1;
> if (a2) {
>    if (a4)
>        return;
> }
> 
> And the latter is much more readable than the former.

I thought about that but didn't want to open code the special and
fairly complex loop construct used there.  To me, it seemed using the
same loop construct would be much less error-prone than open coding
the loop mostly because those two special cases are the only place
where that is necessary.  Maybe we can add pcpu_first_[un]pop_region()
macros and use them there but is the first iteration check that bad
even with sufficient explanations?

Thanks.

-- 
tejun

Re: [PATCH] percpu: explain quick paths in pcpu_[de]populate_chunk()

[Tejun Heo]

On 12/01/2009 02:00 PM, Tejun Heo wrote:
> I thought about that but didn't want to open code the special and
> fairly complex loop construct used there.  To me, it seemed using the
> same loop construct would be much less error-prone than open coding
> the loop mostly because those two special cases are the only place
> where that is necessary.  Maybe we can add pcpu_first_[un]pop_region()
> macros and use them there but is the first iteration check that bad
> even with sufficient explanations?

So, something like the following.

#define pcpu_first_unpop_region(chunk, rs, re, start, end)	do {	\
	(rs) = (start);							\
	pcpu_next_unpop((chunk), &(rs), &(re), (end));			\
} while (0)

#define pcpu_for_each_unpop_region(chunk, rs, re, start, end)		\
	for (pcpu_first_unpop_region(chunk, rs, re, start, end);	\
	     (rs) < (re);						\
	     (rs) = (re) + 1, pcpu_next_unpop((chunk), &(rs), &(re), (end)))

#define pcpu_first_pop_region(chunk, rs, re, start, end)	do {	\
	(rs) = (start);							\
	pcpu_next_pop((chunk), &(rs), &(re), (end));			\
} while (0)

#define pcpu_for_each_pop_region(chunk, rs, re, start, end)		\
	for (pcpu_first_pop_region(chunk, rs, re, start, end);		\
	     (rs) < (re);						\
	     (rs) = (re) + 1, pcpu_next_pop((chunk), &(rs), &(re), (end)))

It might be better to make these proper functions which take pointers
but that makes the only two interfaces for region iterators disagree
about how they take parameters.

So, I don't know.  The first iteration only loop looks a bit unusual
for sure but it isn't something conceptually convoluted.

Thanks.

-- 
tejun

Re: [PATCH] percpu: explain quick paths in pcpu_[de]populate_chunk()

[Cong Wang]

Tejun Heo wrote:
> On 12/01/2009 02:00 PM, Tejun Heo wrote:
>> I thought about that but didn't want to open code the special and
>> fairly complex loop construct used there.  To me, it seemed using the
>> same loop construct would be much less error-prone than open coding
>> the loop mostly because those two special cases are the only place
>> where that is necessary.  Maybe we can add pcpu_first_[un]pop_region()
>> macros and use them there but is the first iteration check that bad
>> even with sufficient explanations?
> 
> So, something like the following.

Thanks for working on this.

> 
> #define pcpu_first_unpop_region(chunk, rs, re, start, end)	do {	\
> 	(rs) = (start);							\
> 	pcpu_next_unpop((chunk), &(rs), &(re), (end));			\
> } while (0)
> 
> #define pcpu_for_each_unpop_region(chunk, rs, re, start, end)		\
> 	for (pcpu_first_unpop_region(chunk, rs, re, start, end);	\
> 	     (rs) < (re);						\
> 	     (rs) = (re) + 1, pcpu_next_unpop((chunk), &(rs), &(re), (end)))
> 
> #define pcpu_first_pop_region(chunk, rs, re, start, end)	do {	\
> 	(rs) = (start);							\
> 	pcpu_next_pop((chunk), &(rs), &(re), (end));			\
> } while (0)
> 
> #define pcpu_for_each_pop_region(chunk, rs, re, start, end)		\
> 	for (pcpu_first_pop_region(chunk, rs, re, start, end);		\
> 	     (rs) < (re);						\
> 	     (rs) = (re) + 1, pcpu_next_pop((chunk), &(rs), &(re), (end)))
> 
> It might be better to make these proper functions which take pointers
> but that makes the only two interfaces for region iterators disagree
> about how they take parameters.
> 
> So, I don't know.  The first iteration only loop looks a bit unusual
> for sure but it isn't something conceptually convoluted.

Now this seems to be better. So with this change, we can do:

pcpu_first_pop_region(chunk, rs, re, start, end);
if (rs < re && ...)
    return;

Right?

Re: [PATCH] percpu: explain quick paths in pcpu_[de]populate_chunk()

[Tejun Heo]

Hello,

On 12/01/2009 02:40 PM, Cong Wang wrote:
>> So, I don't know.  The first iteration only loop looks a bit unusual
>> for sure but it isn't something conceptually convoluted.
> 
> Now this seems to be better. So with this change, we can do:
> 
> pcpu_first_pop_region(chunk, rs, re, start, end);
> if (rs < re && ...)
>    return;
> 
> Right?

Yeap, but is that any better?  Passing lvalue loop parameters to loop
constructs is customary.  For almost all other cases, it's not, so

 pcpu_first_pop_region(chunk, &rs, &re, start, end)

would be better but then we have two similar looking interfaces which
take different types of parameters.  Also, you probably can drop rs <
re test there but for me it just seems easier to simply check the
first iteration.  If you think it's something worth changing and it
looks better afterwards, please send a patch.

Thanks.

-- 
tejun

Re: [PATCH] percpu: explain quick paths in pcpu_[de]populate_chunk()

[Cong Wang]

[-- Attachment #1: Type: text/plain, Size: 1025 bytes --]

Tejun Heo wrote:
> Hello,
> 
> On 12/01/2009 02:40 PM, Cong Wang wrote:
>>> So, I don't know.  The first iteration only loop looks a bit unusual
>>> for sure but it isn't something conceptually convoluted.
>> Now this seems to be better. So with this change, we can do:
>>
>> pcpu_first_pop_region(chunk, rs, re, start, end);
>> if (rs < re && ...)
>>    return;
>>
>> Right?
> 
> Yeap, but is that any better?  Passing lvalue loop parameters to loop
> constructs is customary.  For almost all other cases, it's not, so
> 
>  pcpu_first_pop_region(chunk, &rs, &re, start, end)
> 
> would be better but then we have two similar looking interfaces which
> take different types of parameters.  Also, you probably can drop rs <
> re test there but for me it just seems easier to simply check the
> first iteration.  If you think it's something worth changing and it
> looks better afterwards, please send a patch.
> 

What do you think about the patch below? Untested.

-----------

Signed-off-by: WANG Cong <amwang@redhat.com>


[-- Attachment #2: mm-percpu_c-remove-two-useless-break.diff --]
[-- Type: text/plain, Size: 1326 bytes --]

diff --git a/mm/percpu.c b/mm/percpu.c
index 5adfc26..d1da616 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -911,14 +911,12 @@ static void pcpu_depopulate_chunk(struct pcpu_chunk *chunk, int off, int size)
 	int page_end = PFN_UP(off + size);
 	struct page **pages;
 	unsigned long *populated;
-	int rs, re;
+	int rs = page_start, re;
 
 	/* quick path, check whether it's empty already */
-	pcpu_for_each_unpop_region(chunk, rs, re, page_start, page_end) {
-		if (rs == page_start && re == page_end)
-			return;
-		break;
-	}
+	pcpu_next_unpop(chunk, &rs, &re, page_end);
+	if (rs == page_start && re == page_end)
+		return;
 
 	/* immutable chunks can't be depopulated */
 	WARN_ON(chunk->immutable);
@@ -966,14 +964,12 @@ static int pcpu_populate_chunk(struct pcpu_chunk *chunk, int off, int size)
 	struct page **pages;
 	unsigned long *populated;
 	unsigned int cpu;
-	int rs, re, rc;
+	int rs = page_start, re, rc;
 
 	/* quick path, check whether all pages are already there */
-	pcpu_for_each_pop_region(chunk, rs, re, page_start, page_end) {
-		if (rs == page_start && re == page_end)
-			goto clear;
-		break;
-	}
+	pcpu_next_pop(chunk, &rs, &re, page_end);
+	if (rs == page_start && re == page_end)
+		goto clear;
 
 	/* need to allocate and map pages, this chunk can't be immutable */
 	WARN_ON(chunk->immutable);

Re: [PATCH] percpu: explain quick paths in pcpu_[de]populate_chunk()

[Tejun Heo]

On 12/01/2009 03:35 PM, Cong Wang wrote:
> What do you think about the patch below? Untested.

Oh, yeah, that's prettier.  Just one thing, can you please move rs
initialization right above the pcpu_next_[un]pop() call?  The
input/output parameters for those functions are already pretty
confusing, let's make it at least a bit clearer.

Thanks.

-- 
tejun

[PATCH] percpu: refactor the code in pcpu_[de]populate_chunk()

[Cong Wang]

[-- Attachment #1: Type: text/plain, Size: 551 bytes --]

Tejun Heo wrote:
> On 12/01/2009 03:35 PM, Cong Wang wrote:
>> What do you think about the patch below? Untested.
> 
> Oh, yeah, that's prettier.  Just one thing, can you please move rs
> initialization right above the pcpu_next_[un]pop() call?  The
> input/output parameters for those functions are already pretty
> confusing, let's make it at least a bit clearer.
> 

Sure, done.

------------->

Using break statement at the end of a for loop is confusing,
refactor it by replacing the for loop.

Signed-off-by: WANG Cong <amwang@redhat.com>

---


[-- Attachment #2: mm-percpu_c-remove-two-useless-break.diff --]
[-- Type: text/plain, Size: 1147 bytes --]

diff --git a/mm/percpu.c b/mm/percpu.c
index 5adfc26..2941ed9 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -914,11 +914,10 @@ static void pcpu_depopulate_chunk(struct pcpu_chunk *chunk, int off, int size)
 	int rs, re;
 
 	/* quick path, check whether it's empty already */
-	pcpu_for_each_unpop_region(chunk, rs, re, page_start, page_end) {
-		if (rs == page_start && re == page_end)
-			return;
-		break;
-	}
+	rs = page_start;
+	pcpu_next_unpop(chunk, &rs, &re, page_end);
+	if (rs == page_start && re == page_end)
+		return;
 
 	/* immutable chunks can't be depopulated */
 	WARN_ON(chunk->immutable);
@@ -969,11 +968,10 @@ static int pcpu_populate_chunk(struct pcpu_chunk *chunk, int off, int size)
 	int rs, re, rc;
 
 	/* quick path, check whether all pages are already there */
-	pcpu_for_each_pop_region(chunk, rs, re, page_start, page_end) {
-		if (rs == page_start && re == page_end)
-			goto clear;
-		break;
-	}
+	rs = page_start;
+	pcpu_next_pop(chunk, &rs, &re, page_end);
+	if (rs == page_start && re == page_end)
+		goto clear;
 
 	/* need to allocate and map pages, this chunk can't be immutable */
 	WARN_ON(chunk->immutable);

Re: [PATCH] percpu: refactor the code in pcpu_[de]populate_chunk()

[Tejun Heo]

On 12/01/2009 04:13 PM, Cong Wang wrote:
> Using break statement at the end of a for loop is confusing,
> refactor it by replacing the for loop.
> 
> Signed-off-by: WANG Cong <amwang@redhat.com>

Applied to percpu#for-next.  Thanks.

-- 
tejun