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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Feb 25, 2020 at 04:12:56PM +0100, Jan Beulich wrote:
> 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.

Well, "e" is indeed specific to x86 32bit integers, but since we are
already using "andl" I guess using "i" is equally fine?

I don't have a preference really, so if you prefer "e" I'm fine to
have it changed.

> 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.

Ack.

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

Oh, sure.

> 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.

Thanks, that's fine, please take care while committing if you don't
mind.

Roger.

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