Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[H. Peter Anvin]

I believe that's how it is actually implemented though, and I believe we'd have failures all over if that was not true.

"Jeremy Fitzhardinge" <jeremy@goop.org> wrote:

>On 07/13/2010 03:14 PM, H. Peter Anvin wrote:
>> Actually, I believe volatile operations (including asm volatile) are
>> strictly ordered *with respect to other volatile operations*. 
>
>The documentation makes no reference to that property; in fact it
>suggests it is outright not true:
>
>    Note that even a volatile `asm' instruction can be moved relative to
>    other
>    code, including across jump instructions.  For example, on many targets
>    there is a system register which can be set to control the rounding
>    mode of floating point operations.  You might try setting it with a
>    volatile `asm', like this PowerPC example:
>
>                asm volatile("mtfsf 255,%0" : : "f" (fpenv));
>                sum = x + y;
>
>    This will not work reliably, as the compiler may move the addition
>    back before the volatile `asm'.  To make it work you need to add an
>    artificial dependency to the `asm' referencing a variable in the
>    code you don't want moved, for example:
>
>             asm volatile ("mtfsf 255,%1" : "=X"(sum): "f"(fpenv));
>             sum = x + y;
>
>    Similarly, you can't expect a sequence of volatile `asm'
>    instructions to remain perfectly consecutive.
>    [...]
>    An `asm' instruction without any output operands will be treated
>    identically to a volatile `asm' instruction.
>
>>  As such I
>> would think we'd want to keep the "memory" clobber here, to make it
>> strictly ordered with regards to *all* memory operations.
>>   
>
>That would keep its overall effect consistent.
>
>    J

-- 
Sent from my mobile phone.  Please pardon any lack of formatting.

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[H. Peter Anvin]

On 07/27/2010 10:46 AM, Jeremy Fitzhardinge wrote:
>  On 07/13/2010 11:15 AM, Linus Torvalds wrote:
>> On Tue, Jul 13, 2010 at 11:04 AM, Avi Kivity<avi@redhat.com>  wrote:
>>> Well, current upstream uses "m":
>> Yes, I'd expect current 2.6.34 to also show the same problem.
>>
>>> and works; I also failed to reproduce with 2.6.32.16.  So I expect some
>>> toolchain involvement.
>> And yes, I do believe it's likely a newer gcc that triggers it too,
>> otherwise we'd have seen more reports of it.
> 
> Did anyone get around to doing a proper fix for this?   I don't see one
> in tip.git or linux-2.6.git.  And it needs to get back into stable...
> 

I'll go ahead and write it up.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Jeremy Fitzhardinge]

  On 07/13/2010 11:15 AM, Linus Torvalds wrote:
> On Tue, Jul 13, 2010 at 11:04 AM, Avi Kivity<avi@redhat.com>  wrote:
>> Well, current upstream uses "m":
> Yes, I'd expect current 2.6.34 to also show the same problem.
>
>> and works; I also failed to reproduce with 2.6.32.16.  So I expect some
>> toolchain involvement.
> And yes, I do believe it's likely a newer gcc that triggers it too,
> otherwise we'd have seen more reports of it.

Did anyone get around to doing a proper fix for this?   I don't see one 
in tip.git or linux-2.6.git.  And it needs to get back into stable...

Thanks,
     J

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[H. Peter Anvin]

On 07/14/2010 01:45 PM, Zachary Amsden wrote:
> That's the kind of bug I think Linus is talking about.  We've been
> expecting volatile to work that way for over a decade, by my
> recollection, and if it doesn't, there is going to be a lot of broken code.
> 
> Shouldn't we at least get a compiler switch to force the volatile
> behavior?  I'd suggest it default to conservative.

At this point, it looks like there is no reason to be alarmed.  The
documentation actually contains a statement about volatiles not being
mutually reordered across sequence points, and since asm is a statement
(rather than an expression) it is always surrounded by sequence points.
 I have filed a gcc ticket to ask for clarification.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Jeremy Fitzhardinge]

On 07/14/2010 01:50 PM, H. Peter Anvin wrote:
> invlpg, in the general case, definitely needs a memory clobber even if
> volatiles are ordered, since it needs to be ordered with regards to
> non-volatile memory operations.
>   

Yes.  I'd say write_crX should need that too, since they can they can
have a variety global effects on how memory addressing works (from tlb
flush to switching pagetable formats).

> Note that memory clobbers don't by themselves enforce ordering since
> they don't prevent the ordering of memory clobbers with respect to each
> other.
>   

Hm.  Well we do *definitely* rely on that.  I guess technically we'd
need at least a memory input on the asm to be ordered with respect to a
memory clobber.  But that doesn't seem to have been an issue (or perhaps
its mixed up with the secret semantics of asm volatile).

    J

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Jeremy Fitzhardinge]

On 07/14/2010 01:45 PM, Zachary Amsden wrote:
> clts() has no memory clobber; it is used to serialize execution of
> code within kernel_fpu_begin() / kernel_fpu_end() blocks.
>
> If the code within is reordered before the clts(), we've corrupted
> guest FPU state.

Hm, that's awkward - you'd really need some way of specifying ordering
with respect to the FPU state, and "memory" would only be an approximate
proxy of that.  And not a good one, since gcc wouldn't regard
register-only fpu ops as being affected by a memory clobber.

    J

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Jeremy Fitzhardinge]

On 07/14/2010 12:40 PM, H.J. Lu wrote:
> On Wed, Jul 14, 2010 at 12:36 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>   
>> On 07/14/2010 12:32 PM, H.J. Lu wrote:
>>     
>>> On Wed, Jul 14, 2010 at 12:00 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>>>       
>>>> On 07/14/2010 11:18 AM, H.J. Lu wrote:
>>>>         
>>>>> There are some discussions on:
>>>>>
>>>>> http://gcc.gnu.org/ml/gcc-patches/2010-06/msg02001.html
>>>>> http://gcc.gnu.org/ml/gcc-patches/2010-07/msg00001.html
>>>>>
>>>>> Are they related?
>>>>>
>>>>>           
>>>> Not directly as far as I can tell.
>>>>
>>>> The issue is if gcc can ever reorder, duplicate or elide a volatile
>>>> operation (either asm volatile or a volatile-annotated memory
>>>> reference.)  In my (and Linus') opinion, this would be an incredibly
>>>> serious bug.
>>>>         
>>> Is there a gcc bug for this?
>>>
>>>       
>> Are you asking for a bug report against the documentation?  We're not
>> sure what the semantics intended by the gcc team to be, which I guess is
>> a documentation bug.
>>
>>     
> Documentation bug is also a bug :-).
>   

The question is "what are the real ordering semantics of asm volatile"? 
What ordering is enforced between other asm volatiles?  What ordering is
enforced between asm volatiles and regular memory accesses? asm volatile
and other code?

The documentation discusses this to some extent, but mostly says "there
are no ordering guarantees".  Older versions of gcc - 2.95, for example
- are more explicit, saying that "asm volatiles" won't be moved out of
their basic block (I think that's how I parse it, anyway).

Linux relies on "asm volatile" being ordered at least with respect to
other asm volatiles.  Is this reasonable now?  Will gcc break this at
some point in the future?  If we can't rely on "asm volatile" ordering
semantics, what other mechanism can we use (for example, to order clts
with respect to FPU-using code)?

Thanks,
    J

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Jeremy Fitzhardinge]

On 07/14/2010 11:19 AM, H. Peter Anvin wrote:
> On 07/14/2010 11:15 AM, Jeremy Fitzhardinge wrote:
>   
>> I think we should consider that deprecated and rely on dependencies and
>> clobbers.
>>
>>     
> I don't think that's practical in general.  If the compiler is *that
> broken*, I don't see how it is usable at all.
>   

Well, over the years, gcc has explicitly changed the docs to weaken the
meaning of "asm volatile" from "not being moved ``significantly''" to
"no real guarantees about movement at all".

I don't see why its so broken.  There are lots of mechanisms to control
asm ordering; we don't need "asm volatile" as well.  But we do need it
to mean "still emit an apparently dead asm with no outputs (or unused
outputs)".

They can still be omitted if the whole basic block is dead code, and
they can be duplicated by things like inlining and loop unrolling.  But
presumably they can never be evaluated more times than the source code
says they should be, so to that extent they're like volatile variables.

I think the use of the "volatile" keyword here is a red herring.  Just
because the gcc devs decided to use it as a qualifier for asm statements
doesn't mean that one should read anything other than a vague, vague
relationship to volatile variables (and extra-vague given how weakly
defined *they* are).

> As Linus indicated, I don't think we can assume the gcc documentation to
> accurately reflect the intent of the gcc team, mostly because I think it
> often lags way behind what they're thinking.
>   

I can get not trusting gcc to do what the documentation says it should
do, but relying on it to do things that the documentation definitely
says it doesn't seems foolhardy.

I'm finding it a bit surreal to be arguing on the side of "don't trust
gcc" vs you and Linus on the "gcc developers are arbitrary, capricious
and untrustworthy, but we can rely on this piece of undocumented
behaviour" side.

    J

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Zachary Amsden]

On 07/14/2010 10:45 AM, Zachary Amsden wrote:
> On 07/14/2010 10:40 AM, Jeremy Fitzhardinge wrote:
>> On 07/14/2010 01:16 PM, Avi Kivity wrote:
>>> On 07/14/2010 08:57 PM, Jeremy Fitzhardinge wrote:
>>>> Anything else?
>>> 1. set up a mapping
>>> 2. invlpg or set cr3
>>> 3. use the mapping
>>>
>>> Moving the invlpg will break your code.
>> invlpg uses memory clobbers.  All the crX ops seem to use a
>> __force_order variable to sequence them - but it looks like it's done
>> precisely backwards and it's barking mad to do allow write_crX to be
>> reordered with respect to memory ops.
>>
>> Hm, looks like glommer added it surreptitiously while unifying
>> system_32.h and system_64.h (system_32.h relied on asm volatile not
>> being reordered; system_64.h used memory clobbers).
>>      J
>
> clts() has no memory clobber; it is used to serialize execution of 
> code within kernel_fpu_begin() / kernel_fpu_end() blocks.
>
> If the code within is reordered before the clts(), we've corrupted 
> guest FPU state.
>
> That's the kind of bug I think Linus is talking about.  We've been 
> expecting volatile to work that way for over a decade, by my 
> recollection, and if it doesn't, there is going to be a lot of broken 
> code.
>
> Shouldn't we at least get a compiler switch to force the volatile 
> behavior?  I'd suggest it default to conservative.

Hmm, well, despite that not being quite correct (if guest has used FPU, 
we save it, which has a memory clobber), it seems to be the case that a 
reordering of the clts() among the other volatile asm statements would 
be a very bad thing - you'd get kernel FPU exceptions.

And among asm volatiles, clts() is fairly unique in not having any 
clobbers or dependencies at all, so it could happen.

Zach

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[H. Peter Anvin]

On 07/14/2010 01:40 PM, Jeremy Fitzhardinge wrote:
> On 07/14/2010 01:16 PM, Avi Kivity wrote:
>> On 07/14/2010 08:57 PM, Jeremy Fitzhardinge wrote:
>>> Anything else?
>>
>> 1. set up a mapping
>> 2. invlpg or set cr3
>> 3. use the mapping
>>
>> Moving the invlpg will break your code.
> 
> invlpg uses memory clobbers.  All the crX ops seem to use a
> __force_order variable to sequence them - but it looks like it's done
> precisely backwards and it's barking mad to do allow write_crX to be
> reordered with respect to memory ops.
> 
> Hm, looks like glommer added it surreptitiously while unifying
> system_32.h and system_64.h (system_32.h relied on asm volatile not
> being reordered; system_64.h used memory clobbers).
>     J

invlpg, in the general case, definitely needs a memory clobber even if
volatiles are ordered, since it needs to be ordered with regards to
non-volatile memory operations.

Note that memory clobbers don't by themselves enforce ordering since
they don't prevent the ordering of memory clobbers with respect to each
other.

	-hpa

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Zachary Amsden]

On 07/14/2010 10:40 AM, Jeremy Fitzhardinge wrote:
> On 07/14/2010 01:16 PM, Avi Kivity wrote:
>    
>> On 07/14/2010 08:57 PM, Jeremy Fitzhardinge wrote:
>>      
>>> Anything else?
>>>        
>> 1. set up a mapping
>> 2. invlpg or set cr3
>> 3. use the mapping
>>
>> Moving the invlpg will break your code.
>>      
> invlpg uses memory clobbers.  All the crX ops seem to use a
> __force_order variable to sequence them - but it looks like it's done
> precisely backwards and it's barking mad to do allow write_crX to be
> reordered with respect to memory ops.
>
> Hm, looks like glommer added it surreptitiously while unifying
> system_32.h and system_64.h (system_32.h relied on asm volatile not
> being reordered; system_64.h used memory clobbers).
>      J
>    

clts() has no memory clobber; it is used to serialize execution of code 
within kernel_fpu_begin() / kernel_fpu_end() blocks.

If the code within is reordered before the clts(), we've corrupted guest 
FPU state.

That's the kind of bug I think Linus is talking about.  We've been 
expecting volatile to work that way for over a decade, by my 
recollection, and if it doesn't, there is going to be a lot of broken code.

Shouldn't we at least get a compiler switch to force the volatile 
behavior?  I'd suggest it default to conservative.

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Jeremy Fitzhardinge]

On 07/14/2010 01:16 PM, Avi Kivity wrote:
> On 07/14/2010 08:57 PM, Jeremy Fitzhardinge wrote:
>> Anything else?
>
> 1. set up a mapping
> 2. invlpg or set cr3
> 3. use the mapping
>
> Moving the invlpg will break your code.

invlpg uses memory clobbers.  All the crX ops seem to use a
__force_order variable to sequence them - but it looks like it's done
precisely backwards and it's barking mad to do allow write_crX to be
reordered with respect to memory ops.

Hm, looks like glommer added it surreptitiously while unifying
system_32.h and system_64.h (system_32.h relied on asm volatile not
being reordered; system_64.h used memory clobbers).
    J

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[H. Peter Anvin]

For those that are interested:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=44943

	-hpa

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Avi Kivity]

On 07/14/2010 08:57 PM, Jeremy Fitzhardinge wrote:
> Anything else?

1. set up a mapping
2. invlpg or set cr3
3. use the mapping

Moving the invlpg will break your code.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[H. Peter Anvin]

On 07/14/2010 11:11 PM, Jeremy Fitzhardinge wrote:
>
> The question is "what are the real ordering semantics of asm volatile"?
> What ordering is enforced between other asm volatiles?  What ordering is
> enforced between asm volatiles and regular memory accesses? asm volatile
> and other code?
>
> The documentation discusses this to some extent, but mostly says "there
> are no ordering guarantees".  Older versions of gcc - 2.95, for example
> - are more explicit, saying that "asm volatiles" won't be moved out of
> their basic block (I think that's how I parse it, anyway).
>

I looked over the text, and I think you're confusing static reordering 
and duplication with execution reordering (gcc is indeed free to move 
around and even replicate asm volatile statements).  One thing is that 
the docs makes it perfectly clear that asm volatile is not ordered with 
respect to *non*-volatile operations (*all* the examples are 
volatile-nonvolatile.)  It says that asm volatile statements may not be 
"consecutive", i.e. gcc may generate code in between them, but all of 
this is well known and understood.

The other thing that the documentation *does* specifically make clear is 
that an "asm volatile" has an implicit dependency on all memory (as an 
input, as opposed to an output/clobber.)

I actually found the following statement in the gcc documentation, 
although it is in the section on C++ (6.1 in my version); the text, 
though, makes it clear that it applies to both C and C++:

Both the C and C++ standard have the concept of volatile objects.  These
are normally accessed by pointers and used for accessing hardware.  The
standards encourage compilers to refrain from optimizations concerning
accesses to volatile objects.  The C standard leaves it implementation
defined  as to what constitutes a volatile access.  The C++ standard
omits to specify this, except to say that C++ should behave in a
similar manner to C with respect to volatiles, where possible.  The
minimum either standard specifies is that at a sequence point all
previous accesses to volatile objects have stabilized and no subsequent
accesses have occurred.  Thus an implementation is free to reorder and
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
combine volatile accesses which occur between sequence points, but
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
cannot do so for accesses across a sequence point.  The use of
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
volatiles does not allow you to violate the restriction on updating
objects multiple times within a sequence point.

[My emphasis, obviously.]

I think that pretty much settles the matter, since any statement 
(including an asm statement) inherently has a sequence point before or 
after it.  However, I will file a gcc bug report to clarify.

	-hpa

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[H.J. Lu]

On Wed, Jul 14, 2010 at 12:36 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 07/14/2010 12:32 PM, H.J. Lu wrote:
>> On Wed, Jul 14, 2010 at 12:00 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>>> On 07/14/2010 11:18 AM, H.J. Lu wrote:
>>>>
>>>> There are some discussions on:
>>>>
>>>> http://gcc.gnu.org/ml/gcc-patches/2010-06/msg02001.html
>>>> http://gcc.gnu.org/ml/gcc-patches/2010-07/msg00001.html
>>>>
>>>> Are they related?
>>>>
>>>
>>> Not directly as far as I can tell.
>>>
>>> The issue is if gcc can ever reorder, duplicate or elide a volatile
>>> operation (either asm volatile or a volatile-annotated memory
>>> reference.)  In my (and Linus') opinion, this would be an incredibly
>>> serious bug.
>>
>> Is there a gcc bug for this?
>>
>
> Are you asking for a bug report against the documentation?  We're not
> sure what the semantics intended by the gcc team to be, which I guess is
> a documentation bug.
>

Documentation bug is also a bug :-).


-- 
H.J.

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[H. Peter Anvin]

On 07/14/2010 12:32 PM, H.J. Lu wrote:
> On Wed, Jul 14, 2010 at 12:00 PM, H. Peter Anvin <hpa@zytor.com> wrote:
>> On 07/14/2010 11:18 AM, H.J. Lu wrote:
>>>
>>> There are some discussions on:
>>>
>>> http://gcc.gnu.org/ml/gcc-patches/2010-06/msg02001.html
>>> http://gcc.gnu.org/ml/gcc-patches/2010-07/msg00001.html
>>>
>>> Are they related?
>>>
>>
>> Not directly as far as I can tell.
>>
>> The issue is if gcc can ever reorder, duplicate or elide a volatile
>> operation (either asm volatile or a volatile-annotated memory
>> reference.)  In my (and Linus') opinion, this would be an incredibly
>> serious bug.
> 
> Is there a gcc bug for this?
> 

Are you asking for a bug report against the documentation?  We're not
sure what the semantics intended by the gcc team to be, which I guess is
a documentation bug.

	-hpa

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[H.J. Lu]

On Wed, Jul 14, 2010 at 12:00 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> On 07/14/2010 11:18 AM, H.J. Lu wrote:
>>
>> There are some discussions on:
>>
>> http://gcc.gnu.org/ml/gcc-patches/2010-06/msg02001.html
>> http://gcc.gnu.org/ml/gcc-patches/2010-07/msg00001.html
>>
>> Are they related?
>>
>
> Not directly as far as I can tell.
>
> The issue is if gcc can ever reorder, duplicate or elide a volatile
> operation (either asm volatile or a volatile-annotated memory
> reference.)  In my (and Linus') opinion, this would be an incredibly
> serious bug.
>
>

Is there a gcc bug for this?



-- 
H.J.

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[H. Peter Anvin]

On 07/14/2010 11:18 AM, H.J. Lu wrote:
> 
> There are some discussions on:
> 
> http://gcc.gnu.org/ml/gcc-patches/2010-06/msg02001.html
> http://gcc.gnu.org/ml/gcc-patches/2010-07/msg00001.html
> 
> Are they related?
> 

Not directly as far as I can tell.

The issue is if gcc can ever reorder, duplicate or elide a volatile
operation (either asm volatile or a volatile-annotated memory
reference.)  In my (and Linus') opinion, this would be an incredibly
serious bug.

The gcc 4.4 issue, which is separate from the definition issue, is that
it seems to assume that it can elide references to "const volatile"
objects.  "const volatile" should mean a value that could change at any
time but which is a bug to write to -- for example a readonly hardware
register.

	-hpa

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Linus Torvalds]

On Wed, Jul 14, 2010 at 11:15 AM, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>
> I think we should consider that deprecated and rely on dependencies and
> clobbers.

That makes no sense. According to that logic, "asm volatile" has no
semantic meaning at ALL. That's just crazy talk.

The sane compiler semantics for "asm volatile" is that it acts as a
volatile memory access. That's what the naming implies, and it has
valid semantics that also happen to match the historical semantics. It
means that it cannot be removed or duplicated, and it cannot be
re-ordered wrt other volatile accesses (whether "asm volatile" or a
traditional C volatile memory access).

I agree that we could just add memory clobbers to them all, but my
objection to that is that it just makes the whole keyword totally
pointless.

                 Linus

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[H. Peter Anvin]

On 07/14/2010 11:15 AM, Jeremy Fitzhardinge wrote:
> 
> I think we should consider that deprecated and rely on dependencies and
> clobbers.
> 

I don't think that's practical in general.  If the compiler is *that
broken*, I don't see how it is usable at all.

As Linus indicated, I don't think we can assume the gcc documentation to
accurately reflect the intent of the gcc team, mostly because I think it
often lags way behind what they're thinking.

	-hpa

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[H.J. Lu]

On Wed, Jul 14, 2010 at 11:08 AM, H. Peter Anvin <hpa@zytor.com> wrote:
> [Adding H.J. to the Cc: list]
>
> On 07/14/2010 10:57 AM, Jeremy Fitzhardinge wrote:
>>>>
>>> I/O ports, for example.
>>>
>>
>> Yes, it looks like they should have memory barriers if we want them to
>> be ordered with respect to normal writes; afaict "asm volatile" has
>> never had strict ordering wrt memory ops.
>>
>
> Noone has talked about strict ordering between volatiles and
> (non-volatile) memory ops in general.  I have been talking about
> volatile to volatile ordering, and I thought I'd been very clear about that.
>
> H.J., we're having a debate about the actual semantics of "volatile",
> especially "asm volatile" in gcc.  In particular, I believe that
> volatile operations should not be possible to reorder with regards to
> each other, and the kernel depends on that fact.
>
>        -hpa
>
> P.S: gcc 4.4 seems to handle "const volatile" incorrectly, probably by
> applying CSE to those values.
>
>

There are some discussions on:

http://gcc.gnu.org/ml/gcc-patches/2010-06/msg02001.html
http://gcc.gnu.org/ml/gcc-patches/2010-07/msg00001.html

Are they related?


-- 
H.J.

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Jeremy Fitzhardinge]

On 07/14/2010 11:08 AM, H. Peter Anvin wrote:
> Noone has talked about strict ordering between volatiles and
> (non-volatile) memory ops in general.  I have been talking about
> volatile to volatile ordering, and I thought I'd been very clear about that.
>   

OK.

> H.J., we're having a debate about the actual semantics of "volatile",
> especially "asm volatile" in gcc.  In particular, I believe that
> volatile operations should not be possible to reorder with regards to
> each other, and the kernel depends on that fact.
>   

I think we should consider that deprecated and rely on dependencies and
clobbers.

    J

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[H. Peter Anvin]

[Adding H.J. to the Cc: list]

On 07/14/2010 10:57 AM, Jeremy Fitzhardinge wrote:
>>>     
>> I/O ports, for example.
>>   
> 
> Yes, it looks like they should have memory barriers if we want them to
> be ordered with respect to normal writes; afaict "asm volatile" has
> never had strict ordering wrt memory ops.
> 

Noone has talked about strict ordering between volatiles and
(non-volatile) memory ops in general.  I have been talking about
volatile to volatile ordering, and I thought I'd been very clear about that.

H.J., we're having a debate about the actual semantics of "volatile",
especially "asm volatile" in gcc.  In particular, I believe that
volatile operations should not be possible to reorder with regards to
each other, and the kernel depends on that fact.

	-hpa

P.S: gcc 4.4 seems to handle "const volatile" incorrectly, probably by
applying CSE to those values.

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Jeremy Fitzhardinge]

On 07/14/2010 10:45 AM, H. Peter Anvin wrote:
> On 07/14/2010 10:34 AM, Jeremy Fitzhardinge wrote:
>   
>> On 07/14/2010 10:30 AM, H. Peter Anvin wrote:
>>     
>>> If gcc ever starts reordering volatile operations, including "asm
>>> volatile", the kernel will break, and will be unfixable.  Just about
>>> every single driver will break.  All over the kernel we're explicitly or
>>> implicitly making the assumption that volatile operations are strictly
>>> ordered by the compiler with respect to each other.
>>>       
>> Can you give an example?  All the cases I've seen rely on the ordering
>> properties of "memory" clobbers, which is sound.  (And volatile
>> variables are a completely unrelated issue, of course.)
>>
>>     
> I/O ports, for example.
>   

Yes, it looks like they should have memory barriers if we want them to
be ordered with respect to normal writes; afaict "asm volatile" has
never had strict ordering wrt memory ops.

Anything else?

    J

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[H. Peter Anvin]

On 07/14/2010 10:34 AM, Jeremy Fitzhardinge wrote:
> On 07/14/2010 10:30 AM, H. Peter Anvin wrote:
>> If gcc ever starts reordering volatile operations, including "asm
>> volatile", the kernel will break, and will be unfixable.  Just about
>> every single driver will break.  All over the kernel we're explicitly or
>> implicitly making the assumption that volatile operations are strictly
>> ordered by the compiler with respect to each other.
> 
> Can you give an example?  All the cases I've seen rely on the ordering
> properties of "memory" clobbers, which is sound.  (And volatile
> variables are a completely unrelated issue, of course.)
> 

I/O ports, for example.

	-hpa

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Jeremy Fitzhardinge]

On 07/14/2010 10:30 AM, H. Peter Anvin wrote:
> If gcc ever starts reordering volatile operations, including "asm
> volatile", the kernel will break, and will be unfixable.  Just about
> every single driver will break.  All over the kernel we're explicitly or
> implicitly making the assumption that volatile operations are strictly
> ordered by the compiler with respect to each other.
>   

Can you give an example?  All the cases I've seen rely on the ordering
properties of "memory" clobbers, which is sound.  (And volatile
variables are a completely unrelated issue, of course.)

    J

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[H. Peter Anvin]

On 07/14/2010 10:19 AM, Jeremy Fitzhardinge wrote:
> On 07/13/2010 05:15 PM, Linus Torvalds wrote:
>> The gcc documentation wrt inline asm's is totally worthless. Don't
>> even bother quoting it - because the gcc people themselves have never
>> cared. If the docs ever end up not matching what they want to do, they
>> will just change the documentation.
>>
>> In other words, at least historically the docs are not in any way
>> meaningful. They are not a "these are the semantics we guarantee",
>> they are just random noise. As I mentioned, the docs historically just
>> said something like "will not be moved significantly", and apparently
>> they've been changed to be something else.
> 
> Sure, I completely agree.  At the moment the docs say "asm volatile
> guarantees nothing", and we can work with that.  So long as we don't
> expect asm volatile to mean anything more (ie, magic semantics involving
> reordering), everyone is happy.

Except we do.

> BTW, gcc 2.95's docs do mention "asm volatile" having an effect on
> ordering, which is probably where the notion came from: "If you write an
> `asm' instruction with no outputs, GNU CC [...] not delete the
> instruction or move it outside of loops. [...] you should write the
> `volatile' keyword to prevent future versions of GNU CC from moving the
> instruction around within a core region".  Lucky we never relied on
> that, right?  Right?

If gcc ever starts reordering volatile operations, including "asm
volatile", the kernel will break, and will be unfixable.  Just about
every single driver will break.  All over the kernel we're explicitly or
implicitly making the assumption that volatile operations are strictly
ordered by the compiler with respect to each other.

	-hpa

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Jeremy Fitzhardinge]

On 07/13/2010 05:15 PM, Linus Torvalds wrote:
> The gcc documentation wrt inline asm's is totally worthless. Don't
> even bother quoting it - because the gcc people themselves have never
> cared. If the docs ever end up not matching what they want to do, they
> will just change the documentation.
>
> In other words, at least historically the docs are not in any way
> meaningful. They are not a "these are the semantics we guarantee",
> they are just random noise. As I mentioned, the docs historically just
> said something like "will not be moved significantly", and apparently
> they've been changed to be something else.
>
>   

Sure, I completely agree.  At the moment the docs say "asm volatile
guarantees nothing", and we can work with that.  So long as we don't
expect asm volatile to mean anything more (ie, magic semantics involving
reordering), everyone is happy.

BTW, gcc 2.95's docs do mention "asm volatile" having an effect on
ordering, which is probably where the notion came from: "If you write an
`asm' instruction with no outputs, GNU CC [...] not delete the
instruction or move it outside of loops. [...] you should write the
`volatile' keyword to prevent future versions of GNU CC from moving the
instruction around within a core region".  Lucky we never relied on
that, right?  Right?

    J

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Linus Torvalds]

On Tue, Jul 13, 2010 at 4:49 PM, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>
> The documentation makes no reference to that property; in fact it
> suggests it is outright not true:

The gcc documentation wrt inline asm's is totally worthless. Don't
even bother quoting it - because the gcc people themselves have never
cared. If the docs ever end up not matching what they want to do, they
will just change the documentation.

In other words, at least historically the docs are not in any way
meaningful. They are not a "these are the semantics we guarantee",
they are just random noise. As I mentioned, the docs historically just
said something like "will not be moved significantly", and apparently
they've been changed to be something else.

The only thing that has ever been meaningful is "this works". And, of
course, that has changed over time too, including actual
recommendations on how to make something work (as mentioned, iirc "+"
was only valid on register constraints, and "+m" used to not be
allowed _or_ recommended, these days it's the only way to do certain
things).

It's irritating, because in other circumstances, gcc people take the
reverse approach, and consider paper documentation more important than
actual implementation issues.  So sometimes they say "hey, this is the
spec", but when it comes to their own docs, the answer has
historically been "we'll just change the spec".

                         Linus

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Jeremy Fitzhardinge]

On 07/13/2010 03:14 PM, H. Peter Anvin wrote:
> Actually, I believe volatile operations (including asm volatile) are
> strictly ordered *with respect to other volatile operations*. 

The documentation makes no reference to that property; in fact it
suggests it is outright not true:

    Note that even a volatile `asm' instruction can be moved relative to
    other
    code, including across jump instructions.  For example, on many targets
    there is a system register which can be set to control the rounding
    mode of floating point operations.  You might try setting it with a
    volatile `asm', like this PowerPC example:

                asm volatile("mtfsf 255,%0" : : "f" (fpenv));
                sum = x + y;

    This will not work reliably, as the compiler may move the addition
    back before the volatile `asm'.  To make it work you need to add an
    artificial dependency to the `asm' referencing a variable in the
    code you don't want moved, for example:

             asm volatile ("mtfsf 255,%1" : "=X"(sum): "f"(fpenv));
             sum = x + y;

    Similarly, you can't expect a sequence of volatile `asm'
    instructions to remain perfectly consecutive.
    [...]
    An `asm' instruction without any output operands will be treated
    identically to a volatile `asm' instruction.

>  As such I
> would think we'd want to keep the "memory" clobber here, to make it
> strictly ordered with regards to *all* memory operations.
>   

That would keep its overall effect consistent.

    J

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[H. Peter Anvin]

On 07/13/2010 11:21 AM, Jeremy Fitzhardinge wrote:
> 
> "asm volatile"'s only real meaning is that it will not get elided if it
> appears its output is unused (assuming it is reachable at all).  I don't
> think you can consider it having any meaningful effects on ordering.
> 

Actually, I believe volatile operations (including asm volatile) are
strictly ordered *with respect to other volatile operations*.  As such I
would think we'd want to keep the "memory" clobber here, to make it
strictly ordered with regards to *all* memory operations.

As for the concept in this patch, it's obviously the right thing, but as
Linus said elsewhere it's incomplete.

Conceptually:

Acked-by: H. Peter Anvin <hpa@zytor.com>

Unfortunately I can't write the productization patch right now since I
have a plane to catch in about an hour, but if noone beats me to it I'll
to it Thursday.

	-hpa

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Peter Palfrader]

On Tue, 13 Jul 2010, Avi Kivity wrote:

> Well, current upstream uses "m":
>
>>         case 8:                                                         \
>>                 asm volatile(lock "cmpxchgq %1,%2"                      \
>>                              : "=a"(__ret)                              \
>>                              : "r"(__new), "m"(*__xg(ptr)), "0"(__old)  \
>>                              : "memory");                               \
>>                 break;                                                  \
>
> and works; I also failed to reproduce with 2.6.32.16.  So I expect some  
> toolchain involvement.
>
> Peter, what gcc are you using?

gcc (Debian 4.3.2-1.1) 4.3.2

-- 
                           |  .''`.  ** Debian GNU/Linux **
      Peter Palfrader      | : :' :      The  universal
 http://www.palfrader.org/ | `. `'      Operating System
                           |   `-    http://www.debian.org/

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Peter Palfrader]

[-- Attachment #1: Type: text/plain, Size: 1125 bytes --]

On Tue, 13 Jul 2010, Linus Torvalds wrote:

> On Tue, Jul 13, 2010 at 10:50 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > No, you didn't back-port it wrong. I just didn't fix the right part. I
> > thought the PV code used xchg, not cmpxchg, so I only patched that.
> > But cmpxchg has the exact same issue.
> >
> > Does this fix it?

It appears to, thanks a lot.

[git|v2.6.32.16] weasel@thelma:/scratch/kernel/2.6.32.16$ nm arch/x86/kernel/pvclock.o
0000000000000000 b last_value
..

And it did boot too.

> Btw, this second patch was a bit more aggressive than the first one,
> and actually removes the "memory" clobber entirely, and the fake cast
> of the target type.

Without the cast gcc spews a fair amount of warnings.  About four every
time it hits the include file.


Just for completeness' sake I attached the patch on top of 2.6.32.16
that I built with.
-- 
                           |  .''`.  ** Debian GNU/Linux **
      Peter Palfrader      | : :' :      The  universal
 http://www.palfrader.org/ | `. `'      Operating System
                           |   `-    http://www.debian.org/

[-- Attachment #2: linus.diff.backported --]
[-- Type: text/plain, Size: 2339 bytes --]

diff --git a/arch/x86/include/asm/cmpxchg_64.h b/arch/x86/include/asm/cmpxchg_64.h
index 52de72e..f0551c5 100644
--- a/arch/x86/include/asm/cmpxchg_64.h
+++ b/arch/x86/include/asm/cmpxchg_64.h
@@ -26,26 +26,26 @@ static inline unsigned long __xchg(unsigned long x, volatile void *ptr,
 	switch (size) {
 	case 1:
 		asm volatile("xchgb %b0,%1"
-			     : "=q" (x)
-			     : "m" (*__xg(ptr)), "0" (x)
+			     : "=q" (x), "+m" (*__xg(ptr))
+			     : "0" (x)
 			     : "memory");
 		break;
 	case 2:
 		asm volatile("xchgw %w0,%1"
-			     : "=r" (x)
-			     : "m" (*__xg(ptr)), "0" (x)
+			     : "=r" (x), "+m" (*__xg(ptr))
+			     : "0" (x)
 			     : "memory");
 		break;
 	case 4:
 		asm volatile("xchgl %k0,%1"
-			     : "=r" (x)
-			     : "m" (*__xg(ptr)), "0" (x)
+			     : "=r" (x), "+m" (*__xg(ptr))
+			     : "0" (x)
 			     : "memory");
 		break;
 	case 8:
 		asm volatile("xchgq %0,%1"
-			     : "=r" (x)
-			     : "m" (*__xg(ptr)), "0" (x)
+			     : "=r" (x), "+m" (*__xg(ptr))
+			     : "0" (x)
 			     : "memory");
 		break;
 	}
@@ -66,28 +66,24 @@ static inline unsigned long __cmpxchg(volatile void *ptr, unsigned long old,
 	unsigned long prev;
 	switch (size) {
 	case 1:
-		asm volatile(LOCK_PREFIX "cmpxchgb %b1,%2"
-			     : "=a"(prev)
-			     : "q"(new), "m"(*__xg(ptr)), "0"(old)
-			     : "memory");
+		asm volatile(LOCK_PREFIX "cmpxchgb %b2,%1"
+			     : "=a"(prev), "+m" (*__xg(ptr))
+			     : "q"(new), "0"(old));
 		return prev;
 	case 2:
-		asm volatile(LOCK_PREFIX "cmpxchgw %w1,%2"
-			     : "=a"(prev)
-			     : "r"(new), "m"(*__xg(ptr)), "0"(old)
-			     : "memory");
+		asm volatile(LOCK_PREFIX "cmpxchgw %w2,%1"
+			     : "=a"(prev), "+m" (*__xg(ptr))
+			     : "r"(new), "0"(old));
 		return prev;
 	case 4:
-		asm volatile(LOCK_PREFIX "cmpxchgl %k1,%2"
-			     : "=a"(prev)
-			     : "r"(new), "m"(*__xg(ptr)), "0"(old)
-			     : "memory");
+		asm volatile(LOCK_PREFIX "cmpxchgl %k2,%1"
+			     : "=a"(prev), "+m" (*__xg(ptr))
+			     : "r"(new), "0"(old));
 		return prev;
 	case 8:
-		asm volatile(LOCK_PREFIX "cmpxchgq %1,%2"
-			     : "=a"(prev)
-			     : "r"(new), "m"(*__xg(ptr)), "0"(old)
-			     : "memory");
+		asm volatile(LOCK_PREFIX "cmpxchgq %2,%1"
+			     : "=a"(prev), "+m" (*__xg(ptr))
+			     : "r"(new), "0"(old));
 		return prev;
 	}
 	return old;

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Jeremy Fitzhardinge]

On 07/13/2010 10:59 AM, Linus Torvalds wrote:
> Btw, this second patch was a bit more aggressive than the first one,
> and actually removes the "memory" clobber entirely, and the fake cast
> of the target type.
>
> That shouldn't matter _except_ if people actually use cmpxchg to
> implement their own locking, since now the compiler could potentially
> move unrelated memory references around the lock. Of course, if you
> use cmpxchg to implement your own locking, you're probably doing
> something wrong anyway (ie you'll get the wrong memory barriers on
> various architectures), so it should all be fine.
>   

There are some places which rely on xchg/cmpxchg being a barrier in
arch-specific code.  For example, the Xen code uses as part of the
shared memory protocol with the hypervisor.

> But I thought I'd mention it. And I don't really know how much gcc
> moves memory accesses around a "asm volatile" - the gcc docs are
> historically very vague ("volatile asms aren't moved around
> 'significantly'", whatever 'significant' means)
>   

"asm volatile"'s only real meaning is that it will not get elided if it
appears its output is unused (assuming it is reachable at all).  I don't
think you can consider it having any meaningful effects on ordering.

    J

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Linus Torvalds]

On Tue, Jul 13, 2010 at 11:04 AM, Avi Kivity <avi@redhat.com> wrote:
>
> Well, current upstream uses "m":

Yes, I'd expect current 2.6.34 to also show the same problem.

> and works; I also failed to reproduce with 2.6.32.16.  So I expect some
> toolchain involvement.

And yes, I do believe it's likely a newer gcc that triggers it too,
otherwise we'd have seen more reports of it.

                  Linus

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Avi Kivity]

On 07/13/2010 07:34 PM, Linus Torvalds wrote:
>
> I bet it is the same. And I have a suspicion: because the only write
> access to that variable is in an asm that uses the "memory" clobber to
> say it wrote to it (rather than say it writes to it directly), and
> because the variable is marked 'static', gcc decides that nothing ever
> writes to it in that compilation unit, and it can be made read-only.
>
> Look at our definition for "xchg()" in
> arch/x86/include/asm/cmpxchg_64.h. It boils down to
>
>                  asm volatile("xchgq %0,%1"                              \
>                               : "=r" (__x)                               \
>                               : "m" (*__xg(ptr)), "0" (__x)              \
>                               : "memory");                               \
>
> for the 8-byte case (which is obviously what atomic64_xchg() uses).
> And the _reason_ we do that thing where we use a memory _input_ and
> then a clobber is that older versions of gcc did not accept the thing
> we _want_ to use, namely using "+m" to say that we actually change the
> memory.  So the above is "wrong", but has historical reasons - and
> it's apparently never been changed.
>
> However, the "+m" was fixed, and we use it elsewhere, so I think the
> "m" plus memory clobber is now purely historical. Does a patch
> something like the appended fix it? I also suspect we should look at
> some other uses in this area. The atomic64_64.h file uses "=m" and
> "m", which looks like another legacy thing (again, "+m" historically
> wasn't allowed, and then later became the 'correct' way to do things).
>
>    

Well, current upstream uses "m":

>         case 8:                                                         \
>                 asm volatile(lock "cmpxchgq %1,%2"                      \
>                              : "=a"(__ret)                              \
>                              : "r"(__new), "m"(*__xg(ptr)), "0"(__old)  \
>                              : "memory");                               \
>                 break;                                                  \

and works; I also failed to reproduce with 2.6.32.16.  So I expect some 
toolchain involvement.

Peter, what gcc are you using?


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Linus Torvalds]

On Tue, Jul 13, 2010 at 10:50 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> No, you didn't back-port it wrong. I just didn't fix the right part. I
> thought the PV code used xchg, not cmpxchg, so I only patched that.
> But cmpxchg has the exact same issue.
>
> Does this fix it?

Btw, this second patch was a bit more aggressive than the first one,
and actually removes the "memory" clobber entirely, and the fake cast
of the target type.

That shouldn't matter _except_ if people actually use cmpxchg to
implement their own locking, since now the compiler could potentially
move unrelated memory references around the lock. Of course, if you
use cmpxchg to implement your own locking, you're probably doing
something wrong anyway (ie you'll get the wrong memory barriers on
various architectures), so it should all be fine.

But I thought I'd mention it. And I don't really know how much gcc
moves memory accesses around a "asm volatile" - the gcc docs are
historically very vague ("volatile asms aren't moved around
'significantly'", whatever 'significant' means)

And btw, none of the above should be taken to mean that I have tested
the patch or found it to be otherwise good. It might be totally broken
for other reasons. Caveat emptor.

                             Linus

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 560 bytes --]

On Tue, Jul 13, 2010 at 10:25 AM, Peter Palfrader <peter@palfrader.org> wrote:
>
> Linus's patch touches __xchg() whereas we're using __cmpxchg() in this
> particular case I think.
>
> At least, applying it to my 2.6.32.16 tree didn't help, last_value was
> still read-only.  Or I backported it wrong.

No, you didn't back-port it wrong. I just didn't fix the right part. I
thought the PV code used xchg, not cmpxchg, so I only patched that.
But cmpxchg has the exact same issue.

Does this fix it?

Again: UNTESTED.

                   Linus

[-- Attachment #2: diff --]
[-- Type: application/octet-stream, Size: 1738 bytes --]

 arch/x86/include/asm/cmpxchg_64.h |   28 ++++++++++++----------------
 1 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/arch/x86/include/asm/cmpxchg_64.h b/arch/x86/include/asm/cmpxchg_64.h
index 485ae41..f6face7 100644
--- a/arch/x86/include/asm/cmpxchg_64.h
+++ b/arch/x86/include/asm/cmpxchg_64.h
@@ -71,28 +71,24 @@ extern void __cmpxchg_wrong_size(void);
 	__typeof__(*(ptr)) __new = (new);				\
 	switch (size) {							\
 	case 1:								\
-		asm volatile(lock "cmpxchgb %b1,%2"			\
-			     : "=a"(__ret)				\
-			     : "q"(__new), "m"(*__xg(ptr)), "0"(__old)	\
-			     : "memory");				\
+		asm volatile(lock "cmpxchgb %b2,%1"			\
+			     : "=a"(__ret), "+m" (*ptr)			\
+			     : "q"(__new), "0"(__old));			\
 		break;							\
 	case 2:								\
-		asm volatile(lock "cmpxchgw %w1,%2"			\
-			     : "=a"(__ret)				\
-			     : "r"(__new), "m"(*__xg(ptr)), "0"(__old)	\
-			     : "memory");				\
+		asm volatile(lock "cmpxchgw %w2,%1"			\
+			     : "=a"(__ret), "+m" (*ptr)			\
+			     : "r"(__new), "0"(__old));			\
 		break;							\
 	case 4:								\
-		asm volatile(lock "cmpxchgl %k1,%2"			\
-			     : "=a"(__ret)				\
-			     : "r"(__new), "m"(*__xg(ptr)), "0"(__old)	\
-			     : "memory");				\
+		asm volatile(lock "cmpxchgl %k2,%1"			\
+			     : "=a"(__ret), "+m" (*ptr)			\
+			     : "r"(__new), "0"(__old));			\
 		break;							\
 	case 8:								\
-		asm volatile(lock "cmpxchgq %1,%2"			\
-			     : "=a"(__ret)				\
-			     : "r"(__new), "m"(*__xg(ptr)), "0"(__old)	\
-			     : "memory");				\
+		asm volatile(lock "cmpxchgq %2,%1"			\
+			     : "=a"(__ret), "+m" (*ptr)			\
+			     : "r"(__new), "0"(__old));			\
 		break;							\
 	default:							\
 		__cmpxchg_wrong_size();					\

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Peter Palfrader]

On Tue, 13 Jul 2010, Avi Kivity wrote:

> On 07/13/2010 07:40 PM, Avi Kivity wrote:
>> On 07/13/2010 07:34 PM, Avi Kivity wrote:
>>>
>>> My bet is that dropping 'static' will fix it.  We may have the wrong  
>>> constraints on atomic64_cmpxchg64(), so the compiler thinks we never  
>>> change last_value.
>>>
>>
>> The constraints are there, but maybe the toolchain is confused.
>>
>
> No they aren't, as Linus just pointed out.  His patch should fix the  
> problem.

Linus's patch touches __xchg() whereas we're using __cmpxchg() in this
particular case I think.

At least, applying it to my 2.6.32.16 tree didn't help, last_value was
still read-only.  Or I backported it wrong.
-- 
                           |  .''`.  ** Debian GNU/Linux **
      Peter Palfrader      | : :' :      The  universal
 http://www.palfrader.org/ | `. `'      Operating System
                           |   `-    http://www.debian.org/

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Peter Palfrader]

On Tue, 13 Jul 2010, Avi Kivity wrote:

>> The same:
>>
>> [git|v2.6.32.16] weasel@thelma:/scratch/kernel/2.6.32.16$ nm arch/x86/kernel/pvclock.o
>> 0000000000000000 r last_value
>>                   U native_read_tsc
>> 0000000000000040 T pvclock_clocksource_read
>> 0000000000000100 T pvclock_read_wallclock
>> 0000000000000000 T pvclock_tsc_khz
>>                   U set_normalized_timespec
>>    
>
> But it's
>
>>
>> static atomic64_t last_value = ATOMIC64_INIT(0);
>>
>
> Doesn't make any sense.
>
> Let's try to see if the toolchain is confused:
>
> - rename last_value to some random name

0000000000000000 r some_random_name

> - drop the 'static' qualifier

0000000000000000 B last_value

> - drop the '= ATOMIC64_INIT(0)'

0000000000000000 b last_value

> - all of the above

0000000000000000 B some_random_name




make -f scripts/Makefile.build obj=arch/x86/kernel arch/x86/kernel/pvclock.o
  gcc -Wp,-MD,arch/x86/kernel/.pvclock.o.d  -nostdinc -isystem /usr/lib/gcc/x86_64-linux-gnu/4.3.2/include -Iinclude  -I/scratch/kernel/2.6.32.16/arch/x86/include -include include/linux/autoconf.h -D__KERNEL__ -Wall -Wundef -Wstrict-prototypes -Wno-trigraphs -fno-strict-aliasing -fno-common -Werror-implicit-function-declaration -Wno-format-security -fno-delete-null-pointer-checks -O2 -m64 -mtune=generic -mno-red-zone -mcmodel=kernel -funit-at-a-time -maccumulate-outgoing-args -fstack-protector -DCONFIG_AS_CFI=1 -DCONFIG_AS_CFI_SIGNAL_FRAME=1 -pipe -Wno-sign-compare -fno-asynchronous-unwind-tables -mno-sse -mno-mmx -mno-sse2 -mno-3dnow -fno-omit-frame-pointer -fno-optimize-sibling-calls -g -Wdeclaration-after-statement -Wno-pointer-sign -fno-strict-overflow   -D"KBUILD_STR(s)=#s" -D"KBUILD_BASENAME=KBUILD_STR(pvclock)"  -D"KBUILD_MODNAME=KBUILD_STR(pvclock)"  -c -o arch/x86/kernel/pvclock.o arch/x86/kernel/pvclock.c

[git|v2.6.32.16] weasel@thelma:/scratch/kernel/2.6.32.16$ gcc --version
gcc (Debian 4.3.2-1.1) 4.3.2
Copyright (C) 2008 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Using -O0 also results in a writeable last_value (b).

-- 
                           |  .''`.  ** Debian GNU/Linux **
      Peter Palfrader      | : :' :      The  universal
 http://www.palfrader.org/ | `. `'      Operating System
                           |   `-    http://www.debian.org/

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Avi Kivity]

On 07/13/2010 07:40 PM, Avi Kivity wrote:
> On 07/13/2010 07:34 PM, Avi Kivity wrote:
>>
>> My bet is that dropping 'static' will fix it.  We may have the wrong 
>> constraints on atomic64_cmpxchg64(), so the compiler thinks we never 
>> change last_value.
>>
>
> The constraints are there, but maybe the toolchain is confused.
>

No they aren't, as Linus just pointed out.  His patch should fix the 
problem.

-- 
error compiling committee.c: too many arguments to function

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Avi Kivity]

On 07/13/2010 07:34 PM, Avi Kivity wrote:
>
> My bet is that dropping 'static' will fix it.  We may have the wrong 
> constraints on atomic64_cmpxchg64(), so the compiler thinks we never 
> change last_value.
>

The constraints are there, but maybe the toolchain is confused.

-- 
error compiling committee.c: too many arguments to function

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 2312 bytes --]

On Tue, Jul 13, 2010 at 8:57 AM, Avi Kivity <avi@redhat.com> wrote:
> On 07/13/2010 05:19 PM, Peter Palfrader wrote:
>>
>>> So it looks like last_value was placed in a read only section.  Please
>>> post your System.map somewhere.
>>>
>>
>> weasel@intrepid:~$ publish System.map
>>
>> http://asteria.noreply.org/~weasel/volatile/2010-07-13-mbm2xEdd8Q4/System.map
>> weasel@intrepid:~$ grep -i last_value System.map
>> ffffffff81712e80 r last_value
>> ffffffff81b05240 b last_value.26163
>>
>
> "r" = "read only"
>
> How does it look in 'nm arch/x86/kernel/pvclock.o'?

I bet it is the same. And I have a suspicion: because the only write
access to that variable is in an asm that uses the "memory" clobber to
say it wrote to it (rather than say it writes to it directly), and
because the variable is marked 'static', gcc decides that nothing ever
writes to it in that compilation unit, and it can be made read-only.

Look at our definition for "xchg()" in
arch/x86/include/asm/cmpxchg_64.h. It boils down to

                asm volatile("xchgq %0,%1"                              \
                             : "=r" (__x)                               \
                             : "m" (*__xg(ptr)), "0" (__x)              \
                             : "memory");                               \

for the 8-byte case (which is obviously what atomic64_xchg() uses).
And the _reason_ we do that thing where we use a memory _input_ and
then a clobber is that older versions of gcc did not accept the thing
we _want_ to use, namely using "+m" to say that we actually change the
memory.  So the above is "wrong", but has historical reasons - and
it's apparently never been changed.

However, the "+m" was fixed, and we use it elsewhere, so I think the
"m" plus memory clobber is now purely historical. Does a patch
something like the appended fix it? I also suspect we should look at
some other uses in this area. The atomic64_64.h file uses "=m" and
"m", which looks like another legacy thing (again, "+m" historically
wasn't allowed, and then later became the 'correct' way to do things).

NOTE NOTE NOTE! This is UNTESTED and INCOMPLETE. We should do cmpxchg
too, and the 32-bit versions. I'm adding Ingo and Peter to the cc.

                     Linus

[-- Attachment #2: diff --]
[-- Type: application/octet-stream, Size: 1363 bytes --]

 arch/x86/include/asm/cmpxchg_64.h |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/cmpxchg_64.h b/arch/x86/include/asm/cmpxchg_64.h
index 485ae41..70ac0a4 100644
--- a/arch/x86/include/asm/cmpxchg_64.h
+++ b/arch/x86/include/asm/cmpxchg_64.h
@@ -26,26 +26,26 @@ extern void __cmpxchg_wrong_size(void);
 	switch (size) {							\
 	case 1:								\
 		asm volatile("xchgb %b0,%1"				\
-			     : "=q" (__x)				\
-			     : "m" (*__xg(ptr)), "0" (__x)		\
+			     : "=q" (__x), "+m" (*__xg(ptr))		\
+			     : "0" (__x)				\
 			     : "memory");				\
 		break;							\
 	case 2:								\
 		asm volatile("xchgw %w0,%1"				\
-			     : "=r" (__x)				\
-			     : "m" (*__xg(ptr)), "0" (__x)		\
+			     : "=r" (__x), "+m" (*__xg(ptr))		\
+			     : "0" (__x)				\
 			     : "memory");				\
 		break;							\
 	case 4:								\
 		asm volatile("xchgl %k0,%1"				\
-			     : "=r" (__x)				\
-			     : "m" (*__xg(ptr)), "0" (__x)		\
+			     : "=r" (__x), "+m" (*__xg(ptr))		\
+			     : "0" (__x)				\
 			     : "memory");				\
 		break;							\
 	case 8:								\
 		asm volatile("xchgq %0,%1"				\
-			     : "=r" (__x)				\
-			     : "m" (*__xg(ptr)), "0" (__x)		\
+			     : "=r" (__x), "+m" (*__xg(ptr))		\
+			     : "0" (__x)				\
 			     : "memory");				\
 		break;							\
 	default:							\

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Avi Kivity]

On 07/13/2010 07:22 PM, Peter Palfrader wrote:
>>>
>>> weasel@intrepid:~$ publish System.map
>>> http://asteria.noreply.org/~weasel/volatile/2010-07-13-mbm2xEdd8Q4/System.map
>>> weasel@intrepid:~$ grep -i last_value System.map
>>> ffffffff81712e80 r last_value
>>> ffffffff81b05240 b last_value.26163
>>>
>>>        
>> "r" = "read only"
>>
>> How does it look in 'nm arch/x86/kernel/pvclock.o'?
>>      
> The same:
>
> [git|v2.6.32.16] weasel@thelma:/scratch/kernel/2.6.32.16$ nm arch/x86/kernel/pvclock.o
> 0000000000000000 r last_value
>                   U native_read_tsc
> 0000000000000040 T pvclock_clocksource_read
> 0000000000000100 T pvclock_read_wallclock
> 0000000000000000 T pvclock_tsc_khz
>                   U set_normalized_timespec
>    

But it's

>
> static atomic64_t last_value = ATOMIC64_INIT(0);
>

Doesn't make any sense.

Let's try to see if the toolchain is confused:

- rename last_value to some random name
- drop the 'static' qualifier
- drop the '= ATOMIC64_INIT(0)'
- all of the above

(better start with the last).

No need to rebuild everything, just look at the output of nm pvclock.o.

My bet is that dropping 'static' will fix it.  We may have the wrong 
constraints on atomic64_cmpxchg64(), so the compiler thinks we never 
change last_value.

-- 
error compiling committee.c: too many arguments to function

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Peter Palfrader]

On Tue, 13 Jul 2010, Avi Kivity wrote:

>>> So it looks like last_value was placed in a read only section.  Please
>>> post your System.map somewhere.
>>>      
>> weasel@intrepid:~$ publish System.map
>> http://asteria.noreply.org/~weasel/volatile/2010-07-13-mbm2xEdd8Q4/System.map
>> weasel@intrepid:~$ grep -i last_value System.map
>> ffffffff81712e80 r last_value
>> ffffffff81b05240 b last_value.26163
>>    
>
> "r" = "read only"
>
> How does it look in 'nm arch/x86/kernel/pvclock.o'?

The same:

[git|v2.6.32.16] weasel@thelma:/scratch/kernel/2.6.32.16$ nm arch/x86/kernel/pvclock.o
0000000000000000 r last_value
                 U native_read_tsc
0000000000000040 T pvclock_clocksource_read
0000000000000100 T pvclock_read_wallclock
0000000000000000 T pvclock_tsc_khz
                 U set_normalized_timespec


-- 
                           |  .''`.  ** Debian GNU/Linux **
      Peter Palfrader      | : :' :      The  universal
 http://www.palfrader.org/ | `. `'      Operating System
                           |   `-    http://www.debian.org/

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Avi Kivity]

On 07/13/2010 05:19 PM, Peter Palfrader wrote:
>
>> So it looks like last_value was placed in a read only section.  Please
>> post your System.map somewhere.
>>      
> weasel@intrepid:~$ publish System.map
> http://asteria.noreply.org/~weasel/volatile/2010-07-13-mbm2xEdd8Q4/System.map
> weasel@intrepid:~$ grep -i last_value System.map
> ffffffff81712e80 r last_value
> ffffffff81b05240 b last_value.26163
>    

"r" = "read only"

How does it look in 'nm arch/x86/kernel/pvclock.o'?

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Peter Palfrader]

On Tue, 13 Jul 2010, Avi Kivity wrote:

>> BUG: unable to handle kernel paging request at ffffffff81712e80
>> IP: [<ffffffff81054f73>] pvclock_clocksource_read+0x83/0xc0
>> PGD 1003067 PUD 1007063 PMD 7e8b1063 PTE 8000000001712161
> Read-only pte
>> Oops: 0003 [#2] SMP
> Write fault.
>> Code: 78 4f 44 89 e1 48 d3 e0 44 89 f2 48 f7 e2 48 0f ac d0 20 4a 8d 0c 38 44 39 03 75 a4 48 8b 15 15 df 6b 00 48 39 ca 77 17 48 89 d0<f0>  48 0f b1 0d 04 df 6b 00 48 39 c1 48 89 c2 75 e7 48 89 ca 48
>> RIP  [<ffffffff81054f73>] pvclock_clocksource_read+0x83/0xc0
> --> lock cmpxchg %rcx,0x6bdf04(%rip)


>> static atomic64_t last_value = ATOMIC64_INIT(0);
>>
>> cycle_t pvclock_clocksource_read(struct pvclock_vcpu_time_info *src)
>>         last = atomic64_cmpxchg(&last_value, last, ret);


> So it looks like last_value was placed in a read only section.  Please  
> post your System.map somewhere.

weasel@intrepid:~$ publish System.map
http://asteria.noreply.org/~weasel/volatile/2010-07-13-mbm2xEdd8Q4/System.map
weasel@intrepid:~$ grep -i last_value System.map
ffffffff81712e80 r last_value
ffffffff81b05240 b last_value.26163

Cheers,
Peter
-- 
                           |  .''`.  ** Debian GNU/Linux **
      Peter Palfrader      | : :' :      The  universal
 http://www.palfrader.org/ | `. `'      Operating System
                           |   `-    http://www.debian.org/

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Avi Kivity]

On 07/13/2010 01:23 PM, Peter Palfrader wrote:
> Sorry for the delay.
>    

A good trace is worth it.

>   [<ffffffff810373c0>] ? child_rip+0x0/0x20
> Code: 78 4f 44 89 e1 48 d3 e0 44 89 f2 48 f7 e2 48 0f ac d0 20 4a 8d 0c 38 44 39 03 75 a4 48 8b 15 15 df 6b 00 48 39 ca 77 17 48 89 d0<f0>  48 0f b1 0d 04 df 6b 00 48 39 c1 48 89 c2 75 e7 48 89 ca 48
> RIP  [<ffffffff81054f73>] pvclock_clocksource_read+0x83/0xc0
>   RSP<ffff88007fb47880>
> CR2: ffffffff81712e80
> ---[ end trace 38f25e02ae34d4c1 ]---
> BUG: unable to handle kernel paging request at ffffffff81712e80
> IP: [<ffffffff81054f73>] pvclock_clocksource_read+0x83/0xc0
> PGD 1003067 PUD 1007063 PMD 7e8b1063 PTE 8000000001712161
>    

Read-only pte

> Oops: 0003 [#2] SMP
>    

Write fault.

> Code: 78 4f 44 89 e1 48 d3 e0 44 89 f2 48 f7 e2 48 0f ac d0 20 4a 8d 0c 38 44 39 03 75 a4 48 8b 15 15 df 6b 00 48 39 ca 77 17 48 89 d0<f0>  48 0f b1 0d 04 df 6b 00 48 39 c1 48 89 c2 75 e7 48 89 ca 48
> RIP  [<ffffffff81054f73>] pvclock_clocksource_read+0x83/0xc0
>    

--> lock cmpxchg %rcx,0x6bdf04(%rip)

> static atomic64_t last_value = ATOMIC64_INIT(0);
>
> cycle_t pvclock_clocksource_read(struct pvclock_vcpu_time_info *src)
> {
>

[...]

>     last = atomic64_read(&last_value);
>     do {
>         if (ret < last)
>             return last;
>         last = atomic64_cmpxchg(&last_value, last, ret);
>     } while (unlikely(last != ret));
>
>     return ret;
> }

So it looks like last_value was placed in a read only section.  Please 
post your System.map somewhere.

-- 
error compiling committee.c: too many arguments to function

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Peter Palfrader]

Sorry for the delay.

On Thu, 08 Jul 2010, Avi Kivity wrote:

> I'm unable to reproduce.
>
> Can you provide the stack trace where this hangs?

Here goes:

| (gdb) target remote :1234
| Remote debugging using :1234
| [New Thread 1]
| 0xffffffff81702314 in _spin_lock (lock=0xffffffff81ab9e30)
|     at /scratch/kernel/2.6.32.16/arch/x86/include/asm/spinlock.h:65
| 65      /scratch/kernel/2.6.32.16/arch/x86/include/asm/spinlock.h: No such file or directory.
|         in /scratch/kernel/2.6.32.16/arch/x86/include/asm/spinlock.h
| (gdb) bt
| #0  0xffffffff81702314 in _spin_lock (lock=0xffffffff81ab9e30)
|     at /scratch/kernel/2.6.32.16/arch/x86/include/asm/spinlock.h:65
| #1  0xffffffff8107b214 in vprintk (
|     fmt=0xffffffff818a3ee8 "<1>BUG: unable to handle kernel ", 
|     args=0xffff88001f8f2c38) at kernel/printk.c:705
| #2  0xffffffff816ff387 in printk (fmt=0xffffffff81ab9e30 "")
|     at kernel/printk.c:595
| #3  0xffffffff8105b94a in no_context (regs=0xffff88001f8f2e98, error_code=3, 
|     address=18446744071586262656) at arch/x86/mm/fault.c:583
| #4  0xffffffff8105bb65 in __bad_area_nosemaphore (regs=0xffff88001f8f2e98, 
|     error_code=3, address=18446744071586262656, si_code=196609)
|     at arch/x86/mm/fault.c:741
| #5  0xffffffff8105bc4e in bad_area_nosemaphore (regs=0xffffffff81ab9e30, 
|     error_code=18446612132843695160, address=4294892530)
|     at arch/x86/mm/fault.c:748
| #6  0xffffffff8105c02f in do_page_fault (regs=0xffff88001f8f2e98, error_code=3)
|     at arch/x86/mm/fault.c:1061
| #7  0xffffffff81702765 in page_fault ()
| #8  0x000d3496569e13a8 in ?? ()
| #9  0x00000000b4b2c8e7 in ?? ()
| #10 0x00000003ede5a5a5 in ?? ()
| #11 0x0000000000000001 in per_cpu__irq_stack_union ()
| #12 0xffff880001c11e80 in ?? ()
| ---Type <return> to continue, or q <return> to quit---
| #13 0x0000000000000000 in ?? ()
| (gdb) 
| (gdb) 
| (gdb) 
| (gdb) bt full
| #0  0xffffffff81702314 in _spin_lock (lock=0xffffffff81ab9e30)
|     at /scratch/kernel/2.6.32.16/arch/x86/include/asm/spinlock.h:65
| No locals.
| #1  0xffffffff8107b214 in vprintk (
|     fmt=0xffffffff818a3ee8 "<1>BUG: unable to handle kernel ", 
|     args=0xffff88001f8f2c38) at kernel/printk.c:705
|         printed_len = 0
|         current_log_level = 4
|         flags = 70
|         p = <value optimized out>
| #2  0xffffffff816ff387 in printk (fmt=0xffffffff81ab9e30 "")
|     at kernel/printk.c:595
|         args = {{gp_offset = 8, fp_offset = 48, 
|     overflow_arg_area = 0xffff88001f8f2d18, 
|     reg_save_area = 0xffff88001f8f2c58}}
|         r = 256
| #3  0xffffffff8105b94a in no_context (regs=0xffff88001f8f2e98, error_code=3, 
|     address=18446744071586262656) at arch/x86/mm/fault.c:583
|         tsk = (struct task_struct *) 0xffff88001f908000
|         flags = 146
|         sig = <value optimized out>
| #4  0xffffffff8105bb65 in __bad_area_nosemaphore (regs=0xffff88001f8f2e98, 
|     error_code=3, address=18446744071586262656, si_code=196609)
| ---Type <return> to continue, or q <return> to quit---
|     at arch/x86/mm/fault.c:741
|         tsk = (struct task_struct *) 0xffff88001f908000
| #5  0xffffffff8105bc4e in bad_area_nosemaphore (regs=0xffffffff81ab9e30, 
|     error_code=18446612132843695160, address=4294892530)
|     at arch/x86/mm/fault.c:748
| No locals.
| #6  0xffffffff8105c02f in do_page_fault (regs=0xffff88001f8f2e98, error_code=3)
|     at arch/x86/mm/fault.c:1061
|         vma = <value optimized out>
|         tsk = (struct task_struct *) 0xffff88001f908000
|         address = 18446744071586262656
|         mm = (struct mm_struct *) 0x0
|         write = <value optimized out>
|         fault = 256
| #7  0xffffffff81702765 in page_fault ()
| No symbol table info available.
| #8  0x000d3496569e13a8 in ?? ()
| No symbol table info available.
| #9  0x00000000b4b2c8e7 in ?? ()
| No symbol table info available.
| #10 0x00000003ede5a5a5 in ?? ()
| No symbol table info available.
| #11 0x0000000000000001 in per_cpu__irq_stack_union ()
| ---Type <return> to continue, or q <return> to quit---
| No symbol table info available.
| #12 0xffff880001c11e80 in ?? ()
| No symbol table info available.
| #13 0x0000000000000000 in ?? ()
| No symbol table info available.
| (gdb) quit


So, I disabled PRINTK_TIME and tried again.  This time I got:

VFS: Mounted root (ext3 filesystem) readonly on device 253:0.
Freeing unused kernel memory: 632k freed
Write protecting the kernel read-only data: 9492k
BUG: unable to handle kernel paging request at ffffffff81712e80
IP: [<ffffffff81054f73>] pvclock_clocksource_read+0x83/0xc0
PGD 1003067 PUD 1007063 PMD 7e8b1063 PTE 8000000001712161
Oops: 0003 [#1] SMP 
last sysfs file: 
CPU 0 
Pid: 1, comm: swapper Not tainted 2.6.32.16-dsa-amd64 #2 
RIP: 0010:[<ffffffff81054f73>]  [<ffffffff81054f73>] pvclock_clocksource_read+0x83/0xc0
RSP: 0018:ffff88007fb47880  EFLAGS: 00010283
RAX: 000d362a57085b85 RBX: ffff880001c11e80 RCX: 000d362a573c6400
RDX: 000d362a57085b85 RSI: 000000000001c410 RDI: ffff880001c11e80
RBP: ffff88007fb478c0 R08: 000000000001a9f2 R09: 6db6db6db6db6db7
R10: ffff88007f15a0a0 R11: ffff88007f15a040 R12: 0000000000000001
R13: 0000001112bb6a47 R14: 00000000b4b2c8e7 R15: 000d362a573c2e34
FS:  0000000000000000(0000) GS:ffff880001c00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
CR2: ffffffff81712e80 CR3: 0000000001001000 CR4: 00000000000006f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process swapper (pid: 1, threadinfo ffff88007fb46000, task ffff88007fb50000)
Stack:
 ffff88007fb47900 ffffffff0001a9f2 ffff88007e884150 ffffffff81975c00
<0> 00000000000001b6 ffffffffb3c3c368 ffff88007fb29078 0000000000000000
<0> ffff88007fb478d0 ffffffff8105423c ffff88007fb478e0 ffffffff81054249
Call Trace:
 [<ffffffff8105423c>] kvm_clock_read+0x1c/0x20
 [<ffffffff81054249>] kvm_clock_get_cycles+0x9/0x10
 [<ffffffff81099101>] ktime_get_ts+0x71/0xe0
 [<ffffffff810add4d>] __delayacct_blkio_start+0x1d/0x20
 [<ffffffff8170032a>] io_schedule+0x5a/0xd0
 [<ffffffff81117bdd>] sync_buffer+0x3d/0x50
 [<ffffffff817008ca>] __wait_on_bit+0x5a/0x90
 [<ffffffff81117ba0>] ? sync_buffer+0x0/0x50
 [<ffffffff81117ba0>] ? sync_buffer+0x0/0x50
 [<ffffffff81700974>] out_of_line_wait_on_bit+0x74/0x90
 [<ffffffff81090b30>] ? wake_bit_function+0x0/0x40
 [<ffffffff81117b46>] __wait_on_buffer+0x26/0x30
 [<ffffffff8116331d>] ext3_find_entry+0x19d/0x6b0
 [<ffffffff81060e90>] ? do_flush_tlb_all+0x0/0x70
 [<ffffffff8116418d>] ext3_lookup+0x4d/0x130
 [<ffffffff81105203>] ? d_alloc+0x123/0x1b0
 [<ffffffff810f9fe6>] do_lookup+0x206/0x260
 [<ffffffff810fbe6b>] __link_path_walk+0x19b/0x1000
 [<ffffffff81060eb8>] ? do_flush_tlb_all+0x28/0x70
 [<ffffffff8107fece>] ? on_each_cpu+0x2e/0x50
 [<ffffffff810fcf5e>] path_walk+0x6e/0xe0
 [<ffffffff810fd0eb>] do_path_lookup+0x5b/0x60
 [<ffffffff810fe015>] do_filp_open+0xf5/0xb10
 [<ffffffff810a4026>] ? smp_call_function_many+0x1e6/0x230
 [<ffffffff8105cad0>] ? __cpa_flush_range+0x0/0x30
 [<ffffffff8105caf5>] ? __cpa_flush_range+0x25/0x30
 [<ffffffff8105db73>] ? change_page_attr_set_clr+0x273/0x410
 [<ffffffff81108a5a>] ? alloc_fd+0x4a/0x140
 [<ffffffff810eed7f>] do_sys_open+0x7f/0x140
 [<ffffffff810eee6b>] sys_open+0x1b/0x20
 [<ffffffff810091ef>] init_post+0x3f/0x1b0
 [<ffffffff81a106e5>] kernel_init+0x1a2/0x1ba
 [<ffffffff810373ca>] child_rip+0xa/0x20
 [<ffffffff81a10543>] ? kernel_init+0x0/0x1ba
 [<ffffffff810373c0>] ? child_rip+0x0/0x20
Code: 78 4f 44 89 e1 48 d3 e0 44 89 f2 48 f7 e2 48 0f ac d0 20 4a 8d 0c 38 44 39 03 75 a4 48 8b 15 15 df 6b 00 48 39 ca 77 17 48 89 d0 <f0> 48 0f b1 0d 04 df 6b 00 48 39 c1 48 89 c2 75 e7 48 89 ca 48 
RIP  [<ffffffff81054f73>] pvclock_clocksource_read+0x83/0xc0
 RSP <ffff88007fb47880>
CR2: ffffffff81712e80
---[ end trace 38f25e02ae34d4c1 ]---
BUG: unable to handle kernel paging request at ffffffff81712e80
IP: [<ffffffff81054f73>] pvclock_clocksource_read+0x83/0xc0
PGD 1003067 PUD 1007063 PMD 7e8b1063 PTE 8000000001712161
Oops: 0003 [#2] SMP 
last sysfs file: 
CPU 0 
Pid: 1, comm: swapper Tainted: G      D    2.6.32.16-dsa-amd64 #2 
RIP: 0010:[<ffffffff81054f73>]  [<ffffffff81054f73>] pvclock_clocksource_read+0x83/0xc0
RSP: 0018:ffff880001c03e78  EFLAGS: 00010097
RAX: 000d362a57085b85 RBX: ffff880001c11e80 RCX: 000d362a5b7e8c49
RDX: 000d362a57085b85 RSI: 0000000000000000 RDI: ffff880001c11e80
RBP: ffff880001c03eb8 R08: 000000000001e50c R09: 0000000000000001
R10: ffffffff81ac2360 R11: 0000000000000021 R12: 0000000000000001
R13: 000000111eca7f9f R14: 00000000b4b2c8e7 R15: 000d362a5b7dc64c
FS:  0000000000000000(0000) GS:ffff880001c00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0018 ES: 0018 CR0: 000000008005003b
CR2: ffffffff81712e80 CR3: 0000000001001000 CR4: 00000000000006f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
Process swapper (pid: 1, threadinfo ffff88007fb46000, task ffff88007fb50000)
Stack:
 0000000000000282 000000000001e50c 0000000000000001 ffffffff81975c00
<0> 0000000000000000 0000000023493c1b 00000000000001b6 0000000000000000
<0> ffff880001c03ec8 ffffffff8105423c ffff880001c03ed8 ffffffff81054249
Call Trace:
 <IRQ> 
 [<ffffffff8105423c>] kvm_clock_read+0x1c/0x20
 [<ffffffff81054249>] kvm_clock_get_cycles+0x9/0x10
 [<ffffffff810991da>] ktime_get+0x6a/0xf0
 [<ffffffff81093cd4>] hrtimer_interrupt+0x54/0x1f0
 [<ffffffff810374cc>] ? call_softirq+0x1c/0x30
 [<ffffffff8104e138>] smp_apic_timer_interrupt+0x68/0xa0
 [<ffffffff81036eb3>] apic_timer_interrupt+0x13/0x20
 <EOI> 
 [<ffffffff810a5bb4>] ? acct_collect+0x164/0x1b0
 [<ffffffff810a5af5>] ? acct_collect+0xa5/0x1b0
 [<ffffffff8107e5bc>] ? do_exit+0x6ec/0x7a0
 [<ffffffff8103a943>] ? oops_end+0xa3/0xb0
 [<ffffffff8105b8a8>] ? no_context+0xe8/0x260
 [<ffffffff810c442d>] ? zone_statistics+0x7d/0xa0
 [<ffffffff8105bb65>] ? __bad_area_nosemaphore+0x145/0x220
 [<ffffffff810b2a01>] ? mempool_alloc_slab+0x11/0x20
 [<ffffffff810b2b30>] ? mempool_alloc+0x60/0x130
 [<ffffffff8105bc4e>] ? bad_area_nosemaphore+0xe/0x10
 [<ffffffff8105c02f>] ? do_page_fault+0x1cf/0x280
 [<ffffffff81702765>] ? page_fault+0x25/0x30
 [<ffffffff81054f73>] ? pvclock_clocksource_read+0x83/0xc0
 [<ffffffff8105423c>] ? kvm_clock_read+0x1c/0x20
 [<ffffffff81054249>] ? kvm_clock_get_cycles+0x9/0x10
 [<ffffffff81099101>] ? ktime_get_ts+0x71/0xe0
 [<ffffffff810add4d>] ? __delayacct_blkio_start+0x1d/0x20
 [<ffffffff8170032a>] ? io_schedule+0x5a/0xd0
 [<ffffffff81117bdd>] ? sync_buffer+0x3d/0x50
 [<ffffffff817008ca>] ? __wait_on_bit+0x5a/0x90
 [<ffffffff81117ba0>] ? sync_buffer+0x0/0x50
 [<ffffffff81117ba0>] ? sync_buffer+0x0/0x50
 [<ffffffff81700974>] ? out_of_line_wait_on_bit+0x74/0x90
 [<ffffffff81090b30>] ? wake_bit_function+0x0/0x40
 [<ffffffff81117b46>] ? __wait_on_buffer+0x26/0x30
 [<ffffffff8116331d>] ? ext3_find_entry+0x19d/0x6b0
 [<ffffffff81060e90>] ? do_flush_tlb_all+0x0/0x70
 [<ffffffff8116418d>] ? ext3_lookup+0x4d/0x130
 [<ffffffff81105203>] ? d_alloc+0x123/0x1b0
 [<ffffffff810f9fe6>] ? do_lookup+0x206/0x260
 [<ffffffff810fbe6b>] ? __link_path_walk+0x19b/0x1000
 [<ffffffff81060eb8>] ? do_flush_tlb_all+0x28/0x70
 [<ffffffff8107fece>] ? on_each_cpu+0x2e/0x50
 [<ffffffff810fcf5e>] ? path_walk+0x6e/0xe0
 [<ffffffff810fd0eb>] ? do_path_lookup+0x5b/0x60
 [<ffffffff810fe015>] ? do_filp_open+0xf5/0xb10
 [<ffffffff810a4026>] ? smp_call_function_many+0x1e6/0x230
 [<ffffffff8105cad0>] ? __cpa_flush_range+0x0/0x30
 [<ffffffff8105caf5>] ? __cpa_flush_range+0x25/0x30
 [<ffffffff8105db73>] ? change_page_attr_set_clr+0x273/0x410
 [<ffffffff81108a5a>] ? alloc_fd+0x4a/0x140
 [<ffffffff810eed7f>] ? do_sys_open+0x7f/0x140
 [<ffffffff810eee6b>] ? sys_open+0x1b/0x20
 [<ffffffff810091ef>] ? init_post+0x3f/0x1b0
 [<ffffffff81a106e5>] ? kernel_init+0x1a2/0x1ba
 [<ffffffff810373ca>] ? child_rip+0xa/0x20
 [<ffffffff81a10543>] ? kernel_init+0x0/0x1ba
 [<ffffffff810373c0>] ? child_rip+0x0/0x20
Code: 78 4f 44 89 e1 48 d3 e0 44 89 f2 48 f7 e2 48 0f ac d0 20 4a 8d 0c 38 44 39 03 75 a4 48 8b 15 15 df 6b 00 48 39 ca 77 17 48 89 d0 <f0> 48 0f b1 0d 04 df 6b 00 48 39 c1 48 89 c2 75 e7 48 89 ca 48 
RIP  [<ffffffff81054f73>] pvclock_clocksource_read+0x83/0xc0
 RSP <ffff880001c03e78>
CR2: ffffffff81712e80
---[ end trace 38f25e02ae34d4c2 ]---
Kernel panic - not syncing: Fatal exception in interrupt
Pid: 1, comm: swapper Tainted: G      D    2.6.32.16-dsa-amd64 #2
Call Trace:
 <IRQ>  [<ffffffff816ff24f>] panic+0xa0/0x171
 [<ffffffff81095095>] ? up+0x35/0x50
 [<ffffffff8107ae7d>] ? release_console_sem+0x1cd/0x220
 [<ffffffff8107b155>] ? console_unblank+0x75/0x90
 [<ffffffff8103a93b>] oops_end+0x9b/0xb0
 [<ffffffff8105b8a8>] no_context+0xe8/0x260
 [<ffffffff8105bb65>] __bad_area_nosemaphore+0x145/0x220
 [<ffffffff810b2a22>] ? mempool_free_slab+0x12/0x20
 [<ffffffff810b2aba>] ? mempool_free+0x8a/0xa0
 [<ffffffff813ec9cb>] ? mix_pool_bytes_extract+0x14b/0x160
 [<ffffffff8105bc4e>] bad_area_nosemaphore+0xe/0x10
 [<ffffffff8105c02f>] do_page_fault+0x1cf/0x280
 [<ffffffff81702765>] page_fault+0x25/0x30
 [<ffffffff81054f73>] ? pvclock_clocksource_read+0x83/0xc0
 [<ffffffff81054f3e>] ? pvclock_clocksource_read+0x4e/0xc0
 [<ffffffff8105423c>] kvm_clock_read+0x1c/0x20
 [<ffffffff81054249>] kvm_clock_get_cycles+0x9/0x10
 [<ffffffff810991da>] ktime_get+0x6a/0xf0
 [<ffffffff81093cd4>] hrtimer_interrupt+0x54/0x1f0
 [<ffffffff810374cc>] ? call_softirq+0x1c/0x30
 [<ffffffff8104e138>] smp_apic_timer_interrupt+0x68/0xa0
 [<ffffffff81036eb3>] apic_timer_interrupt+0x13/0x20
 <EOI>  [<ffffffff810a5bb4>] ? acct_collect+0x164/0x1b0
 [<ffffffff810a5af5>] ? acct_collect+0xa5/0x1b0
 [<ffffffff8107e5bc>] ? do_exit+0x6ec/0x7a0
 [<ffffffff8103a943>] ? oops_end+0xa3/0xb0
 [<ffffffff8105b8a8>] ? no_context+0xe8/0x260
 [<ffffffff810c442d>] ? zone_statistics+0x7d/0xa0
 [<ffffffff8105bb65>] ? __bad_area_nosemaphore+0x145/0x220
 [<ffffffff810b2a01>] ? mempool_alloc_slab+0x11/0x20
 [<ffffffff810b2b30>] ? mempool_alloc+0x60/0x130
 [<ffffffff8105bc4e>] ? bad_area_nosemaphore+0xe/0x10
 [<ffffffff8105c02f>] ? do_page_fault+0x1cf/0x280
 [<ffffffff81702765>] ? page_fault+0x25/0x30
 [<ffffffff81054f73>] ? pvclock_clocksource_read+0x83/0xc0
 [<ffffffff8105423c>] ? kvm_clock_read+0x1c/0x20
 [<ffffffff81054249>] ? kvm_clock_get_cycles+0x9/0x10
 [<ffffffff81099101>] ? ktime_get_ts+0x71/0xe0
 [<ffffffff810add4d>] ? __delayacct_blkio_start+0x1d/0x20
 [<ffffffff8170032a>] ? io_schedule+0x5a/0xd0
 [<ffffffff81117bdd>] ? sync_buffer+0x3d/0x50
 [<ffffffff817008ca>] ? __wait_on_bit+0x5a/0x90
 [<ffffffff81117ba0>] ? sync_buffer+0x0/0x50
 [<ffffffff81117ba0>] ? sync_buffer+0x0/0x50
 [<ffffffff81700974>] ? out_of_line_wait_on_bit+0x74/0x90
 [<ffffffff81090b30>] ? wake_bit_function+0x0/0x40
 [<ffffffff81117b46>] ? __wait_on_buffer+0x26/0x30
 [<ffffffff8116331d>] ? ext3_find_entry+0x19d/0x6b0
 [<ffffffff81060e90>] ? do_flush_tlb_all+0x0/0x70
 [<ffffffff8116418d>] ? ext3_lookup+0x4d/0x130
 [<ffffffff81105203>] ? d_alloc+0x123/0x1b0
 [<ffffffff810f9fe6>] ? do_lookup+0x206/0x260
 [<ffffffff810fbe6b>] ? __link_path_walk+0x19b/0x1000
 [<ffffffff81060eb8>] ? do_flush_tlb_all+0x28/0x70
 [<ffffffff8107fece>] ? on_each_cpu+0x2e/0x50
 [<ffffffff810fcf5e>] ? path_walk+0x6e/0xe0
 [<ffffffff810fd0eb>] ? do_path_lookup+0x5b/0x60
 [<ffffffff810fe015>] ? do_filp_open+0xf5/0xb10
 [<ffffffff810a4026>] ? smp_call_function_many+0x1e6/0x230
 [<ffffffff8105cad0>] ? __cpa_flush_range+0x0/0x30
 [<ffffffff8105caf5>] ? __cpa_flush_range+0x25/0x30
 [<ffffffff8105db73>] ? change_page_attr_set_clr+0x273/0x410
 [<ffffffff81108a5a>] ? alloc_fd+0x4a/0x140
 [<ffffffff810eed7f>] ? do_sys_open+0x7f/0x140
 [<ffffffff810eee6b>] ? sys_open+0x1b/0x20
 [<ffffffff810091ef>] ? init_post+0x3f/0x1b0
 [<ffffffff81a106e5>] ? kernel_init+0x1a2/0x1ba
 [<ffffffff810373ca>] ? child_rip+0xa/0x20
 [<ffffffff81a10543>] ? kernel_init+0x0/0x1ba
 [<ffffffff810373c0>] ? child_rip+0x0/0x20

[Also available at http://asteria.noreply.org/~weasel/volatile/2010-07-13-CQL4Lj0xCNo/typescript.zoe.kvmclock.panic ]
-- 
                           |  .''`.  ** Debian GNU/Linux **
      Peter Palfrader      | : :' :      The  universal
 http://www.palfrader.org/ | `. `'      Operating System
                           |   `-    http://www.debian.org/

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Gleb Natapov]

On Thu, Jul 08, 2010 at 10:51:14AM +0200, Peter Palfrader wrote:
> On Thu, 08 Jul 2010, Gleb Natapov wrote:
> 
> > > > And what if you provide -cpu qemu64,-kvmclock to qemu command line?
> > > 
> > > Adding that to the glob of options that already were there from libvirt
> > > didn't disable it, but using an LD_PRELOAD wrapper[1] to that purpose on
> > > the host when starting kvm did help.
> > > 
> > Strange. -kvmclock should have had the same effect. What qemu is it?
> > What is your command line?
> 
> | QEMU PC emulator version 0.11.1 (qemu-kvm-0.11.1), Copyright (c) 2003-2008 Fabrice Bellard
> from the debian package qemu-kvm (0.11.1+dfsg-1~bpo50+1)
That explain it. 0.11 is really old. Don't have this ability.

> [should we trim the CC list?  If yes, to what?]
Better not to.

--
			Gleb.

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Avi Kivity]

On 07/07/2010 03:47 PM, Peter Palfrader wrote:
> Hey,
>
> Greg KH schrieb am Donnerstag, dem 01. Juli 2010:
>
>    
>> 2.6.32-stable review patch.  If anyone has any objections, please let us know.
>>      
> 2.6.32.16 fails to boot on my KVM domains using qemu-kvm 0.11.1.
>
> Bisecting between 2.6.32.14 which worked and .16 turned up this commit
> as the first culprit[0].
>
> The host is still running 2.6.32.14 and has 8 cores on 2 CPUs.  The
> single-cpu KVM domain hangs just after printing 'Write protecting the
> kernel read-only data: 9492k'[1].   On a successful boot this line would
> usually be followed by 'INIT: version 2.86 booting'.
>
> A 2.6.32.16 with this patch reverted boots fine.
>
> If there's any info you need please just ask.
>    

I'm unable to reproduce.

Can you provide the stack trace where this hangs?  Add 
CONFIG_DEBUG_INFO, and then

- copy vmlinux somewhere on the host filesystem
- start the guest
- when it hangs, type 'gdbserver' in the qemu monitor
- on the host, the 'gdb /path/to/vmlinux'
- 'target remote :1234'
- 'backtrace'

-- 
error compiling committee.c: too many arguments to function

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Peter Palfrader]

On Thu, 08 Jul 2010, Gleb Natapov wrote:

> > > And what if you provide -cpu qemu64,-kvmclock to qemu command line?
> > 
> > Adding that to the glob of options that already were there from libvirt
> > didn't disable it, but using an LD_PRELOAD wrapper[1] to that purpose on
> > the host when starting kvm did help.
> > 
> Strange. -kvmclock should have had the same effect. What qemu is it?
> What is your command line?

| QEMU PC emulator version 0.11.1 (qemu-kvm-0.11.1), Copyright (c) 2003-2008 Fabrice Bellard
from the debian package qemu-kvm (0.11.1+dfsg-1~bpo50+1)

/usr/bin/kvm -S -M pc-0.11 -enable-kvm -m 512 -smp 1 -name zoe -uuid 1885e784-c831-4ef8-9576-8eaa9abf3a8b -monitor unix:/var/lib/libvirt/qemu/zoe.monitor,server,nowait -boot c -drive file=/dev/vg_sookie_system/zoe-boot,if=ide,bus=0,unit=0,boot=on -drive file=/dev/vg_sookie_system/zoe-root,if=virtio -drive file=/dev/vg_sookie_system/zoe-swap,if=virtio -net nic,macaddr=00:16:36:40:00:0b,vlan=0,model=virtio,name=net0 -net tap,fd=25,vlan=0,name=hostnet0 -net nic,macaddr=00:16:36:40:05:0b,vlan=1,model=virtio,name=net1 -net tap,fd=28,vlan=1,name=hostnet1 -serial none -parallel none -usb -vnc 127.0.0.1:10 -vga cirrus -balloon virtio

And I had added "-cpu qemu64,-kvmclock" to that at either the front or the end.

Cheers,
[should we trim the CC list?  If yes, to what?]
-- 
                           |  .''`.  ** Debian GNU/Linux **
      Peter Palfrader      | : :' :      The  universal
 http://www.palfrader.org/ | `. `'      Operating System
                           |   `-    http://www.debian.org/

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Gleb Natapov]

On Wed, Jul 07, 2010 at 10:05:36PM +0200, Peter Palfrader wrote:
> On Wed, 07 Jul 2010, Gleb Natapov wrote:
> 
> > On Wed, Jul 07, 2010 at 04:33:39PM +0200, Peter Palfrader wrote:
> > > On Wed, 07 Jul 2010, Glauber Costa wrote:
> > > 
> > > > > 2.6.32.16 fails to boot on my KVM domains using qemu-kvm 0.11.1.
> > > > > 
> > > > > Bisecting between 2.6.32.14 which worked and .16 turned up this commit
> > > > > as the first culprit[0].
> > > > > 
> > > > > The host is still running 2.6.32.14 and has 8 cores on 2 CPUs.  The
> > > > > single-cpu KVM domain hangs just after printing 'Write protecting the
> > > > > kernel read-only data: 9492k'[1].   On a successful boot this line would
> > > > > usually be followed by 'INIT: version 2.86 booting'.
> > > > > 
> > > > > A 2.6.32.16 with this patch reverted boots fine.
> > > > > 
> > > > > If there's any info you need please just ask.
> > > 
> > > > if you boot with another clocksource, and then switch to kvmclock with the machine already
> > > > running, do you see anything strange or suspicious?
> > > 
> > > Booting with various clocksource=xxx kernel parameters does not change
> > > the behaviour at all, i.e. the boot still hangs.
> > > 
> > And what if you provide -cpu qemu64,-kvmclock to qemu command line?
> 
> Adding that to the glob of options that already were there from libvirt
> didn't disable it, but using an LD_PRELOAD wrapper[1] to that purpose on
> the host when starting kvm did help.
> 
Strange. -kvmclock should have had the same effect. What qemu is it?
What is your command line?

> Now, with kvmclock no longer being available at all the system picks tsc
> and indeed boots 2.6.32.16 successfully.
> 
> - Peter
> 1. http://people.debian.org/~paravoid/kvm-noclock-3.tar.gz
> -- 
>                            |  .''`.  ** Debian GNU/Linux **
>       Peter Palfrader      | : :' :      The  universal
>  http://www.palfrader.org/ | `. `'      Operating System
>                            |   `-    http://www.debian.org/

--
			Gleb.

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Glauber Costa]

On Wed, Jul 07, 2010 at 10:11:58AM -1000, Zachary Amsden wrote:
> On 07/07/2010 08:15 AM, Glauber Costa wrote:
> >On Wed, Jul 07, 2010 at 04:33:39PM +0200, Peter Palfrader wrote:
> >>On Wed, 07 Jul 2010, Glauber Costa wrote:
> >>
> >>>>2.6.32.16 fails to boot on my KVM domains using qemu-kvm 0.11.1.
> >>>>
> >>>>Bisecting between 2.6.32.14 which worked and .16 turned up this commit
> >>>>as the first culprit[0].
> >>>>
> >>>>The host is still running 2.6.32.14 and has 8 cores on 2 CPUs.  The
> >>>>single-cpu KVM domain hangs just after printing 'Write protecting the
> >>>>kernel read-only data: 9492k'[1].   On a successful boot this line would
> >>>>usually be followed by 'INIT: version 2.86 booting'.
> >>>>
> >>>>A 2.6.32.16 with this patch reverted boots fine.
> >>>>
> >>>>If there's any info you need please just ask.
> >>>if you boot with another clocksource, and then switch to kvmclock with the machine already
> >>>running, do you see anything strange or suspicious?
> >>Booting with various clocksource=xxx kernel parameters does not change
> >>the behaviour at all, i.e. the boot still hangs.
> >wow, it is really weird then.
> >
> >that patch shouldn't affect anything outside the pvclock realm.
> 
> Unless you added data which is mistakenly in read-only section, I
> can't see how it would affect anything either.  Of course, you have
> changed the data and text size, it is possible this triggered
> another bug.

Yes, but at least this patch alone, does not touch anything on that
regard (or at least should not).

> What exact section does the per-cpu pvclock data fall into?  It's
> read-only in the kernel, but writeable from the hypervisor.
it is not explicitly marked read only in the kernel, afaik.
we just trust the kernel not to use it.

> 
> Also, did this patch arrive before or after the pvclock reboot
> bugfix?  Because if the hypervisor is still writing the pvclock
> page, clocksource=xxx would not change that behavior without that
> fix.
Dunno in which order they got applied in stable. But from what I understood,
this is not a reboot scenario. It happens on first boot, right?

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Zachary Amsden]

On 07/07/2010 08:15 AM, Glauber Costa wrote:
> On Wed, Jul 07, 2010 at 04:33:39PM +0200, Peter Palfrader wrote:
>    
>> On Wed, 07 Jul 2010, Glauber Costa wrote:
>>
>>      
>>>> 2.6.32.16 fails to boot on my KVM domains using qemu-kvm 0.11.1.
>>>>
>>>> Bisecting between 2.6.32.14 which worked and .16 turned up this commit
>>>> as the first culprit[0].
>>>>
>>>> The host is still running 2.6.32.14 and has 8 cores on 2 CPUs.  The
>>>> single-cpu KVM domain hangs just after printing 'Write protecting the
>>>> kernel read-only data: 9492k'[1].   On a successful boot this line would
>>>> usually be followed by 'INIT: version 2.86 booting'.
>>>>
>>>> A 2.6.32.16 with this patch reverted boots fine.
>>>>
>>>> If there's any info you need please just ask.
>>>>          
>>      
>>> if you boot with another clocksource, and then switch to kvmclock with the machine already
>>> running, do you see anything strange or suspicious?
>>>        
>> Booting with various clocksource=xxx kernel parameters does not change
>> the behaviour at all, i.e. the boot still hangs.
>>      
> wow, it is really weird then.
>
> that patch shouldn't affect anything outside the pvclock realm.
>    

Unless you added data which is mistakenly in read-only section, I can't 
see how it would affect anything either.  Of course, you have changed 
the data and text size, it is possible this triggered another bug.

What exact section does the per-cpu pvclock data fall into?  It's 
read-only in the kernel, but writeable from the hypervisor.

Also, did this patch arrive before or after the pvclock reboot bugfix?  
Because if the hypervisor is still writing the pvclock page, 
clocksource=xxx would not change that behavior without that fix.

Zach

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Peter Palfrader]

On Wed, 07 Jul 2010, Gleb Natapov wrote:

> On Wed, Jul 07, 2010 at 04:33:39PM +0200, Peter Palfrader wrote:
> > On Wed, 07 Jul 2010, Glauber Costa wrote:
> > 
> > > > 2.6.32.16 fails to boot on my KVM domains using qemu-kvm 0.11.1.
> > > > 
> > > > Bisecting between 2.6.32.14 which worked and .16 turned up this commit
> > > > as the first culprit[0].
> > > > 
> > > > The host is still running 2.6.32.14 and has 8 cores on 2 CPUs.  The
> > > > single-cpu KVM domain hangs just after printing 'Write protecting the
> > > > kernel read-only data: 9492k'[1].   On a successful boot this line would
> > > > usually be followed by 'INIT: version 2.86 booting'.
> > > > 
> > > > A 2.6.32.16 with this patch reverted boots fine.
> > > > 
> > > > If there's any info you need please just ask.
> > 
> > > if you boot with another clocksource, and then switch to kvmclock with the machine already
> > > running, do you see anything strange or suspicious?
> > 
> > Booting with various clocksource=xxx kernel parameters does not change
> > the behaviour at all, i.e. the boot still hangs.
> > 
> And what if you provide -cpu qemu64,-kvmclock to qemu command line?

Adding that to the glob of options that already were there from libvirt
didn't disable it, but using an LD_PRELOAD wrapper[1] to that purpose on
the host when starting kvm did help.

Now, with kvmclock no longer being available at all the system picks tsc
and indeed boots 2.6.32.16 successfully.

- Peter
1. http://people.debian.org/~paravoid/kvm-noclock-3.tar.gz
-- 
                           |  .''`.  ** Debian GNU/Linux **
      Peter Palfrader      | : :' :      The  universal
 http://www.palfrader.org/ | `. `'      Operating System
                           |   `-    http://www.debian.org/

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Glauber Costa]

On Wed, Jul 07, 2010 at 04:33:39PM +0200, Peter Palfrader wrote:
> On Wed, 07 Jul 2010, Glauber Costa wrote:
> 
> > > 2.6.32.16 fails to boot on my KVM domains using qemu-kvm 0.11.1.
> > > 
> > > Bisecting between 2.6.32.14 which worked and .16 turned up this commit
> > > as the first culprit[0].
> > > 
> > > The host is still running 2.6.32.14 and has 8 cores on 2 CPUs.  The
> > > single-cpu KVM domain hangs just after printing 'Write protecting the
> > > kernel read-only data: 9492k'[1].   On a successful boot this line would
> > > usually be followed by 'INIT: version 2.86 booting'.
> > > 
> > > A 2.6.32.16 with this patch reverted boots fine.
> > > 
> > > If there's any info you need please just ask.
> 
> > if you boot with another clocksource, and then switch to kvmclock with the machine already
> > running, do you see anything strange or suspicious?
> 
> Booting with various clocksource=xxx kernel parameters does not change
> the behaviour at all, i.e. the boot still hangs.
wow, it is really weird then.

that patch shouldn't affect anything outside the pvclock realm.

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Gleb Natapov]

On Wed, Jul 07, 2010 at 04:33:39PM +0200, Peter Palfrader wrote:
> On Wed, 07 Jul 2010, Glauber Costa wrote:
> 
> > > 2.6.32.16 fails to boot on my KVM domains using qemu-kvm 0.11.1.
> > > 
> > > Bisecting between 2.6.32.14 which worked and .16 turned up this commit
> > > as the first culprit[0].
> > > 
> > > The host is still running 2.6.32.14 and has 8 cores on 2 CPUs.  The
> > > single-cpu KVM domain hangs just after printing 'Write protecting the
> > > kernel read-only data: 9492k'[1].   On a successful boot this line would
> > > usually be followed by 'INIT: version 2.86 booting'.
> > > 
> > > A 2.6.32.16 with this patch reverted boots fine.
> > > 
> > > If there's any info you need please just ask.
> 
> > if you boot with another clocksource, and then switch to kvmclock with the machine already
> > running, do you see anything strange or suspicious?
> 
> Booting with various clocksource=xxx kernel parameters does not change
> the behaviour at all, i.e. the boot still hangs.
> 
And what if you provide -cpu qemu64,-kvmclock to qemu command line?

--
			Gleb.

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Peter Palfrader]

On Wed, 07 Jul 2010, Glauber Costa wrote:

> > 2.6.32.16 fails to boot on my KVM domains using qemu-kvm 0.11.1.
> > 
> > Bisecting between 2.6.32.14 which worked and .16 turned up this commit
> > as the first culprit[0].
> > 
> > The host is still running 2.6.32.14 and has 8 cores on 2 CPUs.  The
> > single-cpu KVM domain hangs just after printing 'Write protecting the
> > kernel read-only data: 9492k'[1].   On a successful boot this line would
> > usually be followed by 'INIT: version 2.86 booting'.
> > 
> > A 2.6.32.16 with this patch reverted boots fine.
> > 
> > If there's any info you need please just ask.

> if you boot with another clocksource, and then switch to kvmclock with the machine already
> running, do you see anything strange or suspicious?

Booting with various clocksource=xxx kernel parameters does not change
the behaviour at all, i.e. the boot still hangs.

Cheers,
Peter
-- 
                           |  .''`.  ** Debian GNU/Linux **
      Peter Palfrader      | : :' :      The  universal
 http://www.palfrader.org/ | `. `'      Operating System
                           |   `-    http://www.debian.org/

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Glauber Costa]

On Wed, Jul 07, 2010 at 02:47:31PM +0200, Peter Palfrader wrote:
> Hey,
> 
> Greg KH schrieb am Donnerstag, dem 01. Juli 2010:
> 
> > 2.6.32-stable review patch.  If anyone has any objections, please let us know.
> 
> 2.6.32.16 fails to boot on my KVM domains using qemu-kvm 0.11.1.
> 
> Bisecting between 2.6.32.14 which worked and .16 turned up this commit
> as the first culprit[0].
> 
> The host is still running 2.6.32.14 and has 8 cores on 2 CPUs.  The
> single-cpu KVM domain hangs just after printing 'Write protecting the
> kernel read-only data: 9492k'[1].   On a successful boot this line would
> usually be followed by 'INIT: version 2.86 booting'.
> 
> A 2.6.32.16 with this patch reverted boots fine.
> 
> If there's any info you need please just ask.
if you boot with another clocksource, and then switch to kvmclock with the machine already
running, do you see anything strange or suspicious?

Re: [patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Peter Palfrader]

Hey,

Greg KH schrieb am Donnerstag, dem 01. Juli 2010:

> 2.6.32-stable review patch.  If anyone has any objections, please let us know.

2.6.32.16 fails to boot on my KVM domains using qemu-kvm 0.11.1.

Bisecting between 2.6.32.14 which worked and .16 turned up this commit
as the first culprit[0].

The host is still running 2.6.32.14 and has 8 cores on 2 CPUs.  The
single-cpu KVM domain hangs just after printing 'Write protecting the
kernel read-only data: 9492k'[1].   On a successful boot this line would
usually be followed by 'INIT: version 2.86 booting'.

A 2.6.32.16 with this patch reverted boots fine.

If there's any info you need please just ask.

Cheers,
Peter

0.  http://asteria.noreply.org/~weasel/volatile/2010-07-07-x9KxN34l17c/fileztnjyZ
1.  http://asteria.noreply.org/~weasel/volatile/2010-07-07-VTRuAQGOKlY/zoe-2.6.32.16.png
2.  http://asteria.noreply.org/~weasel/volatile/2010-07-07-Fq0PVc1ecsc/config-2.6.32.16-dsa-amd64
-- 
                           |  .''`.  ** Debian GNU/Linux **
      Peter Palfrader      | : :' :      The  universal
 http://www.palfrader.org/ | `. `'      Operating System
                           |   `-    http://www.debian.org/

[patch 134/149] x86, paravirt: Add a global synchronization point for pvclock

[Greg KH]

2.6.32-stable review patch.  If anyone has any objections, please let us know.

------------------

From: Glauber Costa <glommer@redhat.com>

In recent stress tests, it was found that pvclock-based systems
could seriously warp in smp systems. Using ingo's time-warp-test.c,
I could trigger a scenario as bad as 1.5mi warps a minute in some systems.
(to be fair, it wasn't that bad in most of them). Investigating further, I
found out that such warps were caused by the very offset-based calculation
pvclock is based on.

This happens even on some machines that report constant_tsc in its tsc flags,
specially on multi-socket ones.

Two reads of the same kernel timestamp at approx the same time, will likely
have tsc timestamped in different occasions too. This means the delta we
calculate is unpredictable at best, and can probably be smaller in a cpu
that is legitimately reading clock in a forward ocasion.

Some adjustments on the host could make this window less likely to happen,
but still, it pretty much poses as an intrinsic problem of the mechanism.

A while ago, I though about using a shared variable anyway, to hold clock
last state, but gave up due to the high contention locking was likely
to introduce, possibly rendering the thing useless on big machines. I argue,
however, that locking is not necessary.

We do a read-and-return sequence in pvclock, and between read and return,
the global value can have changed. However, it can only have changed
by means of an addition of a positive value. So if we detected that our
clock timestamp is less than the current global, we know that we need to
return a higher one, even though it is not exactly the one we compared to.

OTOH, if we detect we're greater than the current time source, we atomically
replace the value with our new readings. This do causes contention on big
boxes (but big here means *BIG*), but it seems like a good trade off, since
it provide us with a time source guaranteed to be stable wrt time warps.

After this patch is applied, I don't see a single warp in time during 5 days
of execution, in any of the machines I saw them before.

Signed-off-by: Glauber Costa <glommer@redhat.com>
Acked-by: Zachary Amsden <zamsden@redhat.com>
CC: Jeremy Fitzhardinge <jeremy@goop.org>
CC: Avi Kivity <avi@redhat.com>
CC: Marcelo Tosatti <mtosatti@redhat.com>
CC: Zachary Amsden <zamsden@redhat.com>
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
(cherry picked from commit 489fb490dbf8dab0249ad82b56688ae3842a79e8)
---
 arch/x86/kernel/pvclock.c |   24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -109,11 +109,14 @@ unsigned long pvclock_tsc_khz(struct pvc
 	return pv_tsc_khz;
 }
 
+static atomic64_t last_value = ATOMIC64_INIT(0);
+
 cycle_t pvclock_clocksource_read(struct pvclock_vcpu_time_info *src)
 {
 	struct pvclock_shadow_time shadow;
 	unsigned version;
 	cycle_t ret, offset;
+	u64 last;
 
 	do {
 		version = pvclock_get_time_values(&shadow, src);
@@ -123,6 +126,27 @@ cycle_t pvclock_clocksource_read(struct
 		barrier();
 	} while (version != src->version);
 
+	/*
+	 * Assumption here is that last_value, a global accumulator, always goes
+	 * forward. If we are less than that, we should not be much smaller.
+	 * We assume there is an error marging we're inside, and then the correction
+	 * does not sacrifice accuracy.
+	 *
+	 * For reads: global may have changed between test and return,
+	 * but this means someone else updated poked the clock at a later time.
+	 * We just need to make sure we are not seeing a backwards event.
+	 *
+	 * For updates: last_value = ret is not enough, since two vcpus could be
+	 * updating at the same time, and one of them could be slightly behind,
+	 * making the assumption that last_value always go forward fail to hold.
+	 */
+	last = atomic64_read(&last_value);
+	do {
+		if (ret < last)
+			return last;
+		last = atomic64_cmpxchg(&last_value, last, ret);
+	} while (unlikely(last != ret));
+
 	return ret;
 }