Re: __force_order usage on x86's CRn accesses

Re: __force_order usage on x86's CRn accesses

[Glauber Costa]

> 
> Pretty soon after the introduction of this via "x86: unify paravirt
> parts of system.h" Jeremy had posted a patch to correct the
> (reversed in direction of access) constraints using that auxiliary
> variable. What happened to that change? Was it dropped for a
> reason?

About the change itself, I don't know. Maybe Jeremy, who proposed it,
can give you a better answer.

> Furthermore, the addition of these constraints happened
> without any real explanation - the code comment that was added
> doesn't really help understand why "volatile" isn't sufficient here.

If my memory dont't fail me, I believe this is because gcc will feel
free to reorder a sequence of instructions that does not access memory.
Specially since it has no knowledge of what's in the inline assembly,
and what are its constraints. It only knows that it is an register
operation, and treats it like one.

Also, I believe what we are concerned with here is not arbitrary reorder
between that and other instructions, which we welcome, but reordering
between a read and a write to the same crX - specially of concern for
things doing read-modify-writes of control registers.

This is something I heavily looked at the time, but never again, so
confirmation from people with more knowledge on the compilers world can
be probably helpful here.

> 
> Next, I also saw it being suggested somewhere to replace the
> static definition of the variable (causing an instance to appear
> everywhere any of the referencing inline function being used)
> by an extern one. That also never got carried out, causing
> about two dozen pointless instances of this variable to be
> scattered around, likely consuming cache bandwidth here and
> there.

I indeed see no reason not to do it.

> Now, rather than exporting an auxiliary variable like this, how
> about using an existing, rarely referenced (at least in affected
> contexts) but already exported one instead?
> empty_zero_page, __supported_pte_mask, x86_platform, or
> high_memory might be candidates.

A similar technique is used to determine a safe address for access
somewhere in the scheduler IIRC,

> Finally (and this is because I lack the explanation why the
> artificial constraint is needed in the first place), why is it that
> clts() doesn't need one too?
> 

Because we're not using it to do read-modify-write of the control register.

But let me disclaim again that I might be wrong here...

Re: __force_order usage on x86's CRn accesses

[Jan Beulich]

>>> On 03.07.12 at 11:55, Glauber Costa <glommer@parallels.com> wrote:
>> Furthermore, the addition of these constraints happened
>> without any real explanation - the code comment that was added
>> doesn't really help understand why "volatile" isn't sufficient here.
> 
> If my memory dont't fail me, I believe this is because gcc will feel
> free to reorder a sequence of instructions that does not access memory.
> Specially since it has no knowledge of what's in the inline assembly,
> and what are its constraints. It only knows that it is an register
> operation, and treats it like one.
> 
> Also, I believe what we are concerned with here is not arbitrary reorder
> between that and other instructions, which we welcome, but reordering
> between a read and a write to the same crX - specially of concern for
> things doing read-modify-writes of control registers.

But such sequences can't be re-ordered when the asm-s are
volatile qualified.

>> Finally (and this is because I lack the explanation why the
>> artificial constraint is needed in the first place), why is it that
>> clts() doesn't need one too?
>> 
> 
> Because we're not using it to do read-modify-write of the control register.

Sequences of read_cr0()/clts()/write_cr0() certainly exist (in
the xor implementations), so if a read_cr0()/write_cr0() can
have issues (which is impossible if the value written depends on
the one read), read_cr0()/clts() would be even more affected,
as there's no compiler visible dependency between the two.

But I don't think any such can happen in reality, and I was
hoping to be provided with an example of proving me wrong
(and understand the whole situation).

Jan

__force_order usage on x86's CRn accesses

[Jan Beulich]

Pretty soon after the introduction of this via "x86: unify paravirt
parts of system.h" Jeremy had posted a patch to correct the
(reversed in direction of access) constraints using that auxiliary
variable. What happened to that change? Was it dropped for a
reason?

Furthermore, the addition of these constraints happened
without any real explanation - the code comment that was added
doesn't really help understand why "volatile" isn't sufficient here.

Next, I also saw it being suggested somewhere to replace the
static definition of the variable (causing an instance to appear
everywhere any of the referencing inline function being used)
by an extern one. That also never got carried out, causing
about two dozen pointless instances of this variable to be
scattered around, likely consuming cache bandwidth here and
there.

Now, rather than exporting an auxiliary variable like this, how
about using an existing, rarely referenced (at least in affected
contexts) but already exported one instead?
empty_zero_page, __supported_pte_mask, x86_platform, or
high_memory might be candidates.

Finally (and this is because I lack the explanation why the
artificial constraint is needed in the first place), why is it that
clts() doesn't need one too?

Thanks, Jan