Re: [GIT PULL] percpu fixes for 2.6.32-rc6

[Tejun Heo]

Hello, Ingo.

11/12/2009 08:07 PM, Ingo Molnar wrote:
> Well, the pcpu_alloc() function is 115 lines which is a bit long. It 
> does 2-3 things while a function should try to do one thing.

I agree for low level / utility functions but for top level functions
which direct the flow of the whole logic, I usually prefer to put them
flat.  To me, that way things seem less obfuscated.

> Putting the reserved allocation into a separate function also makes the 
> 'main' path of logic more visible and obstructed less by rare details.
> 
> The indentation i pinpointed is 4 levels deep:
> 
>                                 err = "failed to extend area map of "
>                                         "reserved chunk";
> 
> which is a bit too much IMO - the code starts in the middle of the 
> screen, there's barely any space to do anything meaningful.

Well, all that's there is error exit.  Surrounding code segment is,

			if (pcpu_extend_area_map(chunk, new_alloc) < 0) {
				err = "failed to extend area map of "
					"reserved chunk";
				goto fail_unlock_mutex;
			}

So, we might as well just do

			err = "failed to extend area map of reserved chunk";
			if (pcpu_extend_area_map(chunk, new_alloc) < 0)
				goto fail_unlock_mutex;

> But there's other line wrap artifacts as well further down:
> 
>                                 if (pcpu_extend_area_map(chunk,
>                                                          new_alloc) < 0) {

This one is uglier and one level deeper than the previous one.  The
resulting nesting was one of the reasons why I factored out
pcpu_extend_area_map() as a whole and switched on the return value but
that obfuscated locking.  Although it nests quite a bit, I don't think
the loop there is too bad.  It shows what it does pretty well.

> But ... there's no hard rules here and i've seen functions where 4 
> levels of indentation were just ok. Anyway, i just gave you my opinion, 
> and i'm definitely more on the nitpicky side of the code quality 
> equilibrium, YMMV.

Indentation and code style are actually something I end up spending
quite some time on and I did think about the second one.  Factoring
out without hiding locking is a bit difficult but if I rename
new_alloc to new_len, I can fit that thing onto a single line but that
would probably require renaming matching local variable in
pcpu_extend_area_map() which will end up generating unnecessary amount
of diff obfuscating the real change.  At that point, I just thought we
could live with one slightly ugly line break.

So, I don't know.  Pros and cons on these things depend too much on
personal tastes (and even mood at the time of writing) to form uniform
standard to follow.

Thanks.

-- 
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/