Jarek Poplawski <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
