[Spice-devel] [PATCH 5/7] red_worker: reimplement event loop using poll()

[Dan McGee]

On Fri, Feb 17, 2012 at 7:34 AM, Paolo Bonzini <pbonzini at redhat.com> wrote:
> On 02/17/2012 02:18 PM, Alon Levy wrote:
>>> > - guaranteed bitrot in the poll() path :)
>>> >
>> Yeah, I guess you are right. I'm worried since I don't know the
>> difference in performance between epoll and poll
>
> You can assume it to be zero unless you have a lot of descriptors (50 is
> probably not enough) and a lot of wakeups (if you wake up once every 10
> ms, even spending 0.1 ms to process the 50 descriptors would be 1%).
>
> If it turns out to be a problem, you can always "git revert", I doubt
> this part of the code has churn.

There were a lot of responses here, so I'll try to cover all the points raised.

* epoll vs. poll difference: if you throw multiclient mode aside for a
minute, we have a max of *3* file descriptors involved here.
select()/poll()/epoll()/kqueue performance differences will be
non-existent at this level, epoll/kqueue were designed to handle the
C10K problem, and as Paolo stated, there will be no difference when
we're talking single or double-digit amounts of file descriptors.
* epoll with poll fallback: bitrot absolutely stinks, and there is
zero need for it here, given the above information about performance.
ifdef is even worse as it becomes trivial to introduce a compile
failure on another platform when you change something the preprocessor
omits from the to-be-compiled processed code.
* ifdef/vtable: the overhead of a vtable would probably match any
performance difference of switching to poll from epoll.
* libevent: this was my original idea actually, but when I realized
the trivial number of FDs we were dealing with, I realize it made no
sense. Additionally, our event "loop" here is anything but standard,
with all the refcounting business we do post-poll() call. libevent
basically expects to keep running the loop forever, so running all
that code once per loop involves a rather large refactor which is much
more dangerous than the relatively straightforward changes I had to
make here. Let's say I got something to compile but it was a total
mess going to libevent, and I felt way less sure about that then this
patch.

Alon- if multiple client support is experimental, you'll be fine with
dropping MAX_EVENT_SOURCES back to 10, which would still support up to
4 total clients connecting for now; anyone that wanted more can tweak
this as necessary, and when the support becomes non-experimental, this
can turn into a resizable array rather than a hardcoded length.

Thanks!
-Dan