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

[Roman Penyaev <rpenyaev@suse.de>]

On 2019-05-31 11:56, Peter Zijlstra wrote:
> On Thu, May 16, 2019 at 10:58:03AM +0200, Roman Penyaev wrote:
>> +static inline bool ep_add_event_to_uring(struct epitem *epi, __poll_t 
>> pollflags)
>> +{
>> +	struct eventpoll *ep = epi->ep;
>> +	struct epoll_uitem *uitem;
>> +	bool added = false;
>> +
>> +	if (WARN_ON(!pollflags))
>> +		return false;
>> +
>> +	uitem = &ep->user_header->items[epi->bit];
>> +	/*
>> +	 * Can be represented as:
>> +	 *
>> +	 *    was_ready = uitem->ready_events;
>> +	 *    uitem->ready_events &= ~EPOLLREMOVED;
>> +	 *    uitem->ready_events |= pollflags;
>> +	 *    if (!was_ready) {
>> +	 *         // create index entry
>> +	 *    }
>> +	 *
>> +	 * See the big comment inside ep_remove_user_item(), why it is
>> +	 * important to mask EPOLLREMOVED.
>> +	 */
>> +	if (!atomic_or_with_mask(&uitem->ready_events,
>> +				 pollflags, EPOLLREMOVED)) {
>> +		unsigned int i, *item_idx, index_mask;
>> +
>> +		/*
>> +		 * Item was not ready before, thus we have to insert
>> +		 * new index to the ring.
>> +		 */
>> +
>> +		index_mask = ep_max_index_nr(ep) - 1;
>> +		i = __atomic_fetch_add(&ep->user_header->tail, 1,
>> +				       __ATOMIC_ACQUIRE);
> 
> afaict __atomic_fetch_add() does not exist.

That is gcc extension.  I did not find any API just to increment
the variable atomically without using/casting to atomic.  What
is a proper way to achieve that?

> 
>> +		item_idx = &ep->user_index[i & index_mask];
>> +
>> +		/* Signal with a bit, which is > 0 */
>> +		*item_idx = epi->bit + 1;
> 
> Did you just increment the user visible tail pointer before you filled
> the data? That is, can the concurrent userspace observe the increment
> before you put credible data in its place?

No, the "data" is the "ready_events" mask, which was updated before,
using cmpxchg, atomic_or_with_mask() call.  All I need is to put an
index of just updated item to the uring.

Userspace, in its turn, gets the index from the ring and then checks
the mask.

> 
>> +
>> +		/*
>> +		 * Want index update be flushed from CPU write buffer and
>> +		 * immediately visible on userspace side to avoid long busy
>> +		 * loops.
>> +		 */
>> +		smp_wmb();
> 
> That's still complete nonsense.

Yes, true.  My confusion came from the simple test, where one thread
swaps pointers in a loop, another thread dereferences pointer and
increments a variable:

     THR#0
     -----------

     unsigned vvv1 = 0, vvv2 = 0;
     unsigned *ptr;

     ptr = &vvv1;
     thr_level2 = &vvv2;

     while (!stop) {
         unsigned *tmp = *thr_level2;
         *thr_level2 = ptr;
         barrier();               <<<< ????
         ptr = tmp;
     }

     THR#1
     -----------

     while (!stop) {
    	ptr = thr_level2;
         (*ptr)++;
     }


At the end I expect `vvv1` and `vvv2` are approximately equally
incremented.  But, without barrier() only one variable is
incremented.

Now I see that barrier() should be defined as a simple compiler
barrier as asm volatile("" ::: "memory"), and there is nothing
related with write buffer as I wrote in the comment.

So indeed garbage and can be removed.  Thanks.

--
Roman