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

[Roman Penyaev]

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