Re: [PATCH 2/2] Release mmap_sem when page fault blocks on disk transfer.

[Linus Torvalds <torvalds@linux-foundation.org>]

On Fri, Oct 1, 2010 at 4:06 PM, Michel Lespinasse <walken@google.com> wrote:
>
> To be clear, is it about the helper function or about the comment
> location ? I think the code block is actually short and simple, so
> maybe if I just moved the comment up to the /* Lock the page */
> location it'd also look that way ?

I suspect that if the comment had been up-front rather than mixed deep
in the code, I wouldn't have reacted so much to it.

That said, if something can be cleanly abstracted out as a separate
operation, and a big function be split into smaller ones where the
helper functions do clearly defined things, I think that's generally a
good idea.

Personally, I tend to like comments in front of code - preferably at
the head of a function. If the function is so complex that it needs
comments inside of it, to me that's a sign that perhaps it should be
split up.

That's not _always_ true, of course. Sometimes some particular detail
in a function is what is really specific ("we don't need to use an
atomic instruction here, because xyz"). So it's not a hard rule, but
the "please explain the code _before_ it happens rather than as it
happens" is still a good guideline.

The thing I reacted to in your patch was that in both cases the
comment really explained the _conditional_, not the code inside the
conditional. So putting it inside the conditional was really at the
wrong level, and too late.

                     Linus

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>