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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <AANLkTinf6GKjw0AJjr7768eMU8+yzuE+UeEao2frsdmk@mail.gmail.com>
Date:	Fri, 1 Oct 2010 17:02:43 -0700
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Michel Lespinasse <walken@...gle.com>
Cc:	linux-mm@...ck.org, Ying Han <yinghan@...gle.com>,
	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Rik van Riel <riel@...hat.com>,
	Nick Piggin <npiggin@...e.de>,
	Peter Zijlstra <peterz@...radead.org>,
	Hugh Dickins <hughd@...gle.com>
Subject: Re: [PATCH 2/2] Release mmap_sem when page fault blocks on disk transfer.

On Fri, Oct 1, 2010 at 4:06 PM, Michel Lespinasse <walken@...gle.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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/