[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v3] x86/atomic: Improvements and simplifications to assembly constraints

On 08.08.2019 17:29, Andrew Cooper wrote:
  * Constraints in the form "=r" (x) : "0" (x) can be folded to just "+r" (x)
  * Switch to using named parameters (mostly for legibility) which in
    particular helps with...
  * __xchg(), __cmpxchg() and __cmpxchg_user() modify their memory operand, so
    must list it as an output operand.  This only works because they each have
    a memory clobber to give the construct full compiler-barrier properties.
  * Every memory operand has an explicit known size.  Letting the compiler see
    the real size rather than obscuring it with __xg() allows for the removal
    of the instruction size suffixes without introducing ambiguity.
  * Misc style changes.

Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

In principle I would give this my R-b, but ...

The reason the volatile cast in __cmpxchg_user() can't be dropped is because
without it, the compiler uses a stack copy rather than the in-memory copy,
which ends up tripping:

   /* Allowed to change in Accessed/Dirty flags only. */
   BUG_ON((t ^ old) & ~(intpte_t)(_PAGE_ACCESSED|_PAGE_DIRTY));

... this is concerning me, seeing that elsewhere you eliminate
casts to pointers to volatile (and often you actively cast away
the attribute from the function parameters), which was one of
the effects of __xg(). Therefore only with "volatile" restored
everywhere where it was in effect before

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Alternatively justification for the removal would need to be
added to the description.

As to the description: The dropping of the semicolons after
the LOCK prefixes would perhaps be worth another bullet point?
It's not really just a "misc style change" imo.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.