Re: [PATCH] documentation: explain memory barriers

[Nick Piggin <nickpiggin@yahoo.com.au>]

On Thursday 09 October 2008 16:51, Chris Snook wrote:
> Andrew Morton wrote:
> > On Wed, 08 Oct 2008 21:17:58 -0400 Chris Snook <csnook@redhat.com> wrote:
> >> Randy Dunlap wrote:
> >>> On Wed, 1 Oct 2008 22:54:04 -0700 Andrew Morton wrote:
> >>>> This sequence is repeated three or four times and should be pulled out
> >>>> into a well-commented function.  That comment should explain the logic
> >>>> behind the use of these barriers, please.
> >>>
> >>> and on 2008-OCT-08 Ben Hutchings wrote:
> >>>> All memory barriers need a comment to explain why and what they're
> >>>> doing.
> >
> > I approve this message.
> >
> >> Seriously?  When a barrier is used, it's generally self-evident what
> >> it's doing.
> >
> > fs/buffer.c:sync_buffer().  Have fun.
>
> The real disaster there is the clear_buffer_##name macro and friends, as
> evidenced by fs/ext2/inode.c:599
>
>                 clear_buffer_new(bh_result); /* What's this do? */

That's not a disaster. It is relatively easy even if you have no
idea about any of that code to a) work out what BH_New flag does,
see where it gets set and where it gets cleared, and then work out
what that does. Actually, buffer.c used to leak BH_New in some
cases, but now it should be a bug if BH_New is found to be set
there (buffer.c should take any BH_New buffers, initialize them
appropriately, and clear BH_New; a dangling BH_New would indicate
uninitialized data going to or coming from the block).

No, they're easy, because you can find every single place where any
one cares about them with a single simple grep.

Again, fs/buffer.c:sync_buffer(). Which stores and/or loads is that
barrier sequencing against which subsequent stores and/or loads? Why?

For another fun example, mm/filemap.c:sync_page. This actually has a
big comment, but (to me) it isn't even evident then which loads and
stores are being sequenced against which subsequent ones because it
is not explicitly documented. And I do have some experience in adding
barriers to existing mm code where they have been missed completely.

mempool_free, set_dumpable, freeze_bdev.



> I'm completely in favor of documenting everything that can potentially
> interact with that train wreck,

What's the train-wreck, again?


> but I maintain that the vast majority of 
> memory barriers are self-evident.

They are self-evident if you have spent a lot of time getting the
state machine and locking/concurrency model in your mind. If you
have not, then how do you know there is not some memory operation
way back or forward in the instruction stream that is supposed to
be ordered by this barrier?

All memory barriers have to be documented, except acquire/release
for critical sections, IMO.