Messages in this thread Patch in this message | | | Date | Tue, 01 Dec 2009 14:35:34 +0800 | From | Cong Wang <> | Subject | Re: [PATCH] percpu: explain quick paths in pcpu_[de]populate_chunk() |
| |
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>
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); | |