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

[Roman Penyaev <rpenyaev@suse.de>]

On 2019-05-31 18:51, Peter Zijlstra wrote:
> On Fri, May 31, 2019 at 04:21:30PM +0200, Roman Penyaev wrote:
> 
>> The ep_add_event_to_uring() is lockless, thus I can't increase tail 
>> after,
>> I need to reserve the index slot, where to write to.  I can use shadow 
>> tail,
>> which is not seen by userspace, but I have to guarantee that tail is 
>> updated
>> with shadow tail *after* all callers of ep_add_event_to_uring() are 
>> left.
>> That is possible, please see the code below, but it adds more 
>> complexity:
>> 
>> (code was tested on user side, thus has c11 atomics)
>> 
>> static inline void add_event__kernel(struct ring *ring, unsigned bit)
>> {
>>         unsigned i, cntr, commit_cntr, *item_idx, tail, old;
>> 
>>         i = __atomic_fetch_add(&ring->cntr, 1, __ATOMIC_ACQUIRE);
>>         item_idx = &ring->user_itemsindex[i % ring->nr];
>> 
>>         /* Update data */
>>         *item_idx = bit;
>> 
>>         commit_cntr = __atomic_add_fetch(&ring->commit_cntr, 1,
>> __ATOMIC_RELEASE);
>> 
>>         tail = ring->user_header->tail;
>>         rmb();
>>         do {
>>                 cntr = ring->cntr;
>>                 if (cntr != commit_cntr)
>>                         /* Someone else will advance tail */
>>                         break;
>> 
>>                 old = tail;
>> 
>>         } while ((tail =
>> __sync_val_compare_and_swap(&ring->user_header->tail, old, cntr)) != 
>> old);
>> }
> 
> Yes, I'm well aware of that particular problem (see
> kernel/events/ring_buffer.c:perf_output_put_handle for instance).

I'll take a look, thanks.

> But like you show, it can be done. It also makes the thing wait-free, 
> as
> opposed to merely lockless.

You think it's better?  I did not like this variant from the very
beginning because of the unnecessary complexity.  But maybe you're
right.  No busy loops on user side makes it wait-free.  And also
I can avoid c11 in kernel using cmpxchg along with atomic_t.

--
Roman