Re: [PATCH v3 06/13] epoll: introduce helpers for adding/removing events to uring

Peter Zijlstra
2019-05-31T13:11:36Z

On Fri, May 31, 2019 at 01:24:07PM +0200, Roman Penyaev wrote:
> On 2019-05-31 11:55, Peter Zijlstra wrote:
> > On Thu, May 16, 2019 at 10:58:03AM +0200, Roman Penyaev wrote:
> > > +#define atomic_set_unless_zero(ptr, flags) \
> > > +({ \
> > > + typeof(ptr) _ptr = (ptr); \
> > > + typeof(flags) _flags = (flags); \
> > > + typeof(*_ptr) _old, _val = READ_ONCE(*_ptr); \
> > > + \
> > > + for (;;) { \
> > > + if (!_val) \
> > > + break; \
> > > + _old = cmpxchg(_ptr, _val, _flags); \
> > > + if (_old == _val) \
> > > + break; \
> > > + _val = _old; \
> > > + } \
> > > + _val; \
> > > +})
> >
> > > +#define atomic_or_with_mask(ptr, flags, mask) \
> > > +({ \
> > > + typeof(ptr) _ptr = (ptr); \
> > > + typeof(flags) _flags = (flags); \
> > > + typeof(flags) _mask = (mask); \
> > > + typeof(*_ptr) _old, _new, _val = READ_ONCE(*_ptr); \
> > > + \
> > > + for (;;) { \
> > > + _new = (_val & ~_mask) | _flags; \
> > > + _old = cmpxchg(_ptr, _val, _new); \
> > > + if (_old == _val) \
> > > + break; \
> > > + _val = _old; \
> > > + } \
> > > + _val; \
> > > +})
> >
> > Don't call them atomic_*() if they're not part of the atomic_t
> > interface.
>
> Can we add those two? Or keep it local is better?

Afaict you're using them on the user visible values; we should not put
atomic_t into shared memory.

Your interface isn't compatible with those 'funny' architectures like
parisc etc. Since you expect userspace to do atomic ops on these
variables too. It is not a one-way interface ...