On Mon, Nov 2, 2020 at 9:49 AM Soheil Hassas Yeganeh <soheil@xxxxxxxxxx> wrote:
>
> Thank you for the suggestion. That was the first version I tried, and
> I can confirm it fixes the race because we call ep_send_events() once
> more before returning. Though, I believe, due to time_out=1, we won't
> goto fetch_events to call ep_events_available():
>
> if (!res && eavail &&
> !(res = ep_send_events(ep, events, maxevents)) && !timed_out)
> goto fetch_events;
Right. We won't be repeating the loop, but we will do one final send_events.
Which I think is really the point, no?
> You're spot on that the patch is more complicated than your
> suggestion. However, the downside I observed was a performance
> regression for the non-racy case: Suppose there are a few threads with
> a similar non-zero timeout and no ready event. They will all
> experience a noticeable contention in ep_scan_ready_list, by
> unconditionally calling ep_send_events(). The contention was large
> because there will be 2 write locks on ep->lock and one mutex lock on
> ep->mtx with a large critical section.
Ugh. I really detest the eventpoll code. Afaik, it has no normal users
anywhere, so it gets no real coverage except for the odd cases
(presumably inside google?)
A lot of the work over the last couple of years has been to try to
simplify the code and streamline it, and fix bugs due to recursion.
I really wish we would continue that pattern of trying to simplify
this code rather than add more complexity on top of it, which is why I
reacted to strongly to that patch.
And that whole ep_poll() function is written in just about the most
confusing way possible, with code that looks like loops but aren't
("while (0)") and goto's that _are_ loops ("goto fetch_events").
I'll go stare at it some more.
Linus
