Re: [Xen-devel] [PATCH v3 1/2] atomic: add atomic_and operations

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.02.2020 09:43, Roger Pau Monne wrote:> --- a/xen/include/asm-x86/atomic.h
> +++ b/xen/include/asm-x86/atomic.h
> @@ -224,6 +224,14 @@ static inline int atomic_add_unless(atomic_t *v, int a, 
> int u)
>      return c;
>  }
>  
> +static inline void atomic_and(unsigned int m, atomic_t *v)
> +{
> +    asm volatile (
> +        "lock; andl %1,%0"

I realize this is in sync with other atomic_*(), but I'd prefer if
the stray semicolon after "lock" was dropped. Without it the
assembler can actually diagnose a bad use (the destination not
being a memory operand). I'm unaware of reasons why the semicolons
would have been put there.

> +        : "=m" (*(volatile int *)&v->counter)
> +        : "ir" (m), "m" (*(volatile int *)&v->counter) );

Similarly despite its use elsewhere I'm afraid "i" is not the best
choice here for the constraint. Together with switching to plain
int as the function's first parameter type, "e" would seem better
even if the difference would only manifest for atomic64_t. As to
the choice of parameter type, Linux too uses plain int, so while
I agree with your reasoning I think I also agree with Julien to
use plain int here for consistency.

Finally, yet another improvement over existing code would be to
use just a single output "+m", with no paralleling input "m".

With the first and last, but not necessarily the middle one taken
care of (and I'd be happy to take care of them while committing)
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
If, otoh, you disagree on some, then let's see where we can
find common grounds.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel