Messages in this thread | | | Date | Thu, 12 Nov 2009 19:11:15 +0900 | From | Tejun Heo <> | Subject | Re: [GIT PULL] percpu fixes for 2.6.32-rc6 |
| |
Hello, Ingo.
11/12/2009 04:57 AM, Ingo Molnar wrote: > Sure - pulled it into tip:master for testing earlier today and after a > few hours of it's looking good so far in x86 runtime tests. I also did > cross-build testing to a dozen non-x86 architectures and it was fine > there too.
Great.
> btw., there's some 80-cols checkpatch warning artifacts in the commit: > > + if (pcpu_extend_area_map(chunk, new_alloc) < 0) { > + err = "failed to extend area map of " > + "reserved chunk"; > > which suggest that the logic here is perhaps nested a bit too deep. It > could be improved by moving the reserved allocation branch of > pcpu_alloc():
Strange, although the line break isn't the prettiest thing, the only checkpatch problem I can see is the following.
> scripts/checkpatch.pl 0001-percpu-restructure-pcpu_extend_area_map-to-fix-bugs-.patch ERROR: trailing whitespace #80: FILE: mm/percpu.c:382: +^Ireturn new_alloc;^I$
total: 1 errors, 0 warnings, 179 lines checked
0001-percpu-restructure-pcpu_extend_area_map-to-fix-bugs-.patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS.
The patch adds a trailing tab. I'll fix that up (I usually catch these while using quilt but this one didn't go through quilt and I forgot to run checkpatch).
> if (reserved && pcpu_reserved_chunk) { > > into a helper inline function, something like __pcpu_alloc_reserved(). > > It's a rare special case anyway. It could be changed to return with the > pcpu_lock always taken, so the above branch would look like this: > > if (unlikely(reserved)) { > off = __pcpu_alloc_reserved(&chunk, size, align, &err); > if (off < 0) > goto fail_unlock; > goto area_found; > } > > Which is a cleaner flow IMO, and which simplifes pcpu_alloc().
Hmmm... The thing is that the nesting isn't that deep there and breaking string in the middle is something we do quite often. What checkpatch warning did you see?
Thanks.
-- tejun
| |