lkml.org 
[lkml]   [2007]   [May]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] Documentation/memory-barriers.txt: various fixes
Date
Jarek Poplawski <jarkao2@o2.pl> wrote:

> I think at least some of these fixes are justified.

Yeah. I think you're right in most cases, but not all. See below.

David
---
> @@ -265,8 +265,8 @@
> ...
> Such enforcement is important because the CPUs and other devices in a system
> -can use a variety of tricks to improve performance - including reordering,
> -deferral and combination of memory operations; speculative loads; speculative
> +can use a variety of tricks to improve performance - including: reordering,
> +deferral and combination of memory operations, speculative loads, speculative

I disagree. The colon looks wrong here. If you say it out load, there's no
break in the flow between "including" and "reordering". I also think that
semicolons are correct as there needs to be a bigger pause between "loads" and
"speculative" than between "reordering" and "deferral".

> @@ -300,7 +300,7 @@
> ...
> - load will be directed), a data dependency barrier would be required to
> + load will be directed), the data dependency barrier would be required to

I think that should be "a".

> @@ -457,8 +457,8 @@
> ...
> -But! CPU 2's perception of P may be updated _before_ its perception of B, thus
> +But (!) CPU 2's perception of P may be updated _before_ its perception of B,

That's a matter of taste, I think. However, if my solution is chosen, there
should be an extra space after "But!". Hmmm... actually, I think you're wrong
because the "But!" isn't quite part of the following sentence.

> @@ -602,21 +602,21 @@
>
> This sequence of events is committed to the memory coherence system in an order
> that the rest of the system might perceive as the unordered set of { STORE A,
> -STORE B, STORE C } all occurring before the unordered set of { STORE D, STORE E
> -}:
> +STORE B, STORE C } - all occurring before the unordered set of { STORE D, STORE
> +E }:

Hmmm. I don't think that a dash is correct here. I think it changes the
meaning, by changing the way the elements are grouped.

> | | wwwwwwwwwwwwwwww } <--- At this point the write barrier
> | | +------+ } requires all stores prior to the
> - | | : | E=5 | } barrier to be committed before
> - | | : +------+ } further stores may be take place.
> + | | : | E=5 | } barrier to be committed, before
> + | | : +------+ } further stores may take place

That's partly wrong. The operative term is "committed before".

However "may be take" -> "may take" is correct.

> @@ -644,7 +644,7 @@
>
> +-------+ : : : :
> | | +------+ +-------+ | Sequence of update
> - | |------>| B=2 |----- --->| Y->8 | | of perception on
> + | |------>| B=2 |----- --->| Y->8 | | perception on

I think this changes the meaning to one I don't want. But I'm not entirely
sure. In a way the two concepts "update of perception" and "update perception"
are different things. I think this can be argued either way.

> @@ -1143,14 +1143,14 @@
> ...
> -Therefore, from (1), (2) and (4) an UNLOCK followed by an unconditional LOCK is
> -equivalent to a full barrier, but a LOCK followed by an UNLOCK is not.
> +Therefore, from (1), (2) and (4) the UNLOCK followed by the unconditional LOCK
> +is equivalent to a full barrier, but the LOCK followed by the UNLOCK is not.

I think this should be "a" not "the". I'm not talking about any locks in
particular.

> -A LOCK followed by an UNLOCK may not be assumed to be full memory barrier
> +The LOCK followed by the UNLOCK may not be assumed to be full memory barrier

Again "a" not "the".

> @@ -1239,7 +1239,7 @@
> ...
> -Then there is no guarantee as to what order CPU #3 will see the accesses to *A
> +Then there is no guarantee, as to what order CPU 3 will see the accesses to *A

There shouldn't be a comma there.

> @@ -1375,7 +1375,7 @@
> ...
> -operate without the use of a lock if at all possible. In such a case
> +operate without the use of the lock if at all possible. In such a case

That should definitely be "a" not "the". There is no specific lock mentioned
to be definite about.

> @@ -1396,10 +1396,10 @@
> ...
> - (1) read the next pointer from this waiter's record to know as to where the
> - next waiter record is;
> + (1) read the list.next pointer from this waiter's record to know as to where
> + the next waiter record is;

That's unimportant, and also assumes that "list.next" exists and will exist in
all implementations.

> @@ -1423,7 +1423,7 @@
> ...
> -stack before the up*() function has a chance to read the next pointer.
> +stack before the up_*() function has a chance to read the next pointer.

That's unimportant as we're clearly talking about rwsems. However, to be
consistent, this should probably be up_xxx().

> @@ -1659,16 +1660,16 @@
> ...
> Whether these are guaranteed to be fully ordered and uncombined with
> - respect to each other on the issuing CPU depends on the characteristics
> + respect to each other on the issuing CPU - depends on the characteristics

That dash is definitely wrong. The sentence is of the form "Whether X is/are Y
depends on Z".

> However, intermediary hardware (such as a PCI bridge) may indulge in
> - deferral if it so wishes; to flush a store, a load from the same location
> + deferral if it wishes so; to flush a store, a load from the same location

I disagree on that one. I would say the former, but not the latter.


Anyway, thanks for the review! Any change in your patch I haven't mentioned is
one I'm okay with.

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

\
 
 \ /
  Last update: 2007-05-21 14:13    [from the cache]
©2003-2011 Jasper Spaans. Advertise on this site