[Spice-devel] Replace epoll with select in X client
Hans de Goede
hdegoede at redhat.com
Tue Oct 12 04:44:55 PDT 2010
Ack.
I've taken the liberty of pushing this for you.
Thanks & Regards,
Hans
On 09/29/2010 05:28 PM, Alexander Larsson wrote:
> The use of epoll in the client is totally overkill in terms of
> scalability, and its a problem for portability. The OSX port converts
> this to use select, but keeps some of the old complexities in the code.
> This new patch makes it simpler and look much more like the windows
> code.
>
> Review?
>
> diff --git a/client/x11/event_sources_p.h b/client/x11/event_sources_p.h
> index 09703c0..959460c 100644
> --- a/client/x11/event_sources_p.h
> +++ b/client/x11/event_sources_p.h
> @@ -21,24 +21,18 @@
> #include "common.h"
> #include "threads.h"
>
> -#if __GLIBC__> 2 || (__GLIBC__ == 2&& __GLIBC_MINOR__>= 8)
> -#define USING_EVENT_FD
> -#endif
> -
> #define INFINITE -1
>
> -class EventWrapper;
> +class EventSource;
>
> class EventSources_p {
> -public:
> - void remove_wrapper(EventWrapper*);
> +protected:
> + void add_event(int fd, EventSource* source);
> + void remove_event(EventSource* source);
>
> public:
> - int _epoll;
> - typedef std::list<EventWrapper*> Events;
> - Events _events;
> -
> - friend class EventWrapper;
> + std::vector<EventSource*> _events;
> + std::vector<int> _fds;
> };
>
> class Trigger_p {
> @@ -49,9 +43,7 @@ public:
>
> public:
> int _event_fd;
> -#ifndef USING_EVENT_FD
> int _event_write_fd;
> -#endif
> bool _pending_int;
> Mutex _lock;
> };
> diff --git a/client/x11/event_sources_p.cpp b/client/x11/event_sources_p.cpp
> index d59bffd..54d950f 100644
> --- a/client/x11/event_sources_p.cpp
> +++ b/client/x11/event_sources_p.cpp
> @@ -15,189 +15,141 @@
> License along with this library; if not, see<http://www.gnu.org/licenses/>.
> */
>
> -#include<sys/epoll.h>
> +#include<sys/select.h>
> #include<sys/fcntl.h>
>
> #include "event_sources.h"
> #include "debug.h"
> #include "utils.h"
>
> -#ifdef USING_EVENT_FD
> -#include<sys/eventfd.h>
> -#endif
> -
> -#define NUM_EPOLL_EVENTS 10
> -
> -#ifdef USING_EVENT_FD
> -#define WRITE_FD _event_fd
> -#define EVENT_DATA_TYPE eventfd_t
> -#else
> -#define WRITE_FD _event_write_fd
> -#define EVENT_DATA_TYPE uint8_t
> -#endif
> -
> -class EventWrapper {
> -public:
> - EventWrapper(EventSources& owner, EventSource& event)
> - : _owner (owner)
> - , _event (&event)
> - , _refs (1)
> - {
> - }
> -
> - EventWrapper* ref()
> - {
> - _refs++;
> - return this;
> +static void set_non_blocking(int fd)
> +{
> + int flags;
> + if ((flags = ::fcntl(fd, F_GETFL)) == -1) {
> + THROW("failed to set socket non block: %s", strerror(errno));
> }
>
> - void unref()
> - {
> - if (!--_refs) {
> - _owner.remove_wrapper(this);
> - delete this;
> - }
> + if (::fcntl(fd, F_SETFL, flags | O_NONBLOCK) == -1) {
> + THROW("failed to set socket non block: %s", strerror(errno));
> }
> +}
>
> - EventSource* get_event()
> - {
> - return _event;
> +static void set_blocking(int fd)
> +{
> + int flags;
> + if ((flags = ::fcntl(fd, F_GETFL)) == -1) {
> + THROW("failed to clear socket non block: %s", strerror(errno));
> }
>
> - void invalidate()
> - {
> - _event = NULL;
> + if (::fcntl(fd, F_SETFL, flags& ~O_NONBLOCK) == -1) {
> + THROW("failed to clear socket non block: %s", strerror(errno));
> }
> -
> -private:
> - EventSources& _owner;
> - EventSource* _event;
> - int _refs;
> -};
> +}
>
> EventSources::EventSources()
> {
> - _epoll = epoll_create(NUM_EPOLL_EVENTS);
> - if (_epoll == -1) {
> - THROW("create epool failed");
> - }
> }
>
> EventSources::~EventSources()
> {
> - Events::iterator iter = _events.begin();
> - for (; iter != _events.end(); iter++) {
> - delete *iter;
> - }
> - close(_epoll);
> }
>
> -bool EventSources::wait_events(int timeout_ms)
> +void EventSources_p::add_event(int fd, EventSource* source)
> {
> - struct epoll_event events[NUM_EPOLL_EVENTS];
> - int num_events = epoll_wait(_epoll, events, NUM_EPOLL_EVENTS, timeout_ms);
> + int size = _events.size();
> + _events.resize(size + 1);
> + _fds.resize(size + 1);
> + _events[size] = source;
> + _fds[size] = fd;
> +}
>
> - if (num_events == -1) {
> - if (errno == EINTR) {
> - return false;
> +void EventSources_p::remove_event(EventSource* source)
> +{
> + int size = _events.size();
> + for (int i = 0; i< size; i++) {
> + if (_events[i] == source) {
> + for (i++; i< size; i++) {
> + _events[i - 1] = _events[i];
> + _fds[i - 1] = _fds[i];
> + }
> + _events.resize(size - 1);
> + _fds.resize(size - 1);
> + return;
> }
> - THROW("wait error eventfd failed");
> + }
> + THROW("event not found");
> +}
> +
> +bool EventSources::wait_events(int timeout_msec)
> +{
> + int maxfd = 0;
> + fd_set rfds;
> + struct timeval tv;
> + struct timeval *tvp;
> + int ready;
> +
> + FD_ZERO(&rfds);
> +
> + int size = _events.size();
> + for (int i = 0; i< size; i++) {
> + maxfd = MAX(maxfd, _fds[i]);
> + FD_SET(_fds[i],&rfds);
> }
>
> - for (int i = 0; i< num_events; i++) {
> - ((EventWrapper*)events[i].data.ptr)->ref();
> + if (timeout_msec == INFINITE) {
> + tvp = NULL;
> + } else {
> + tv.tv_sec = timeout_msec / 1000;
> + tv.tv_usec = (timeout_msec % 1000) * 1000;
> + tvp =&tv;
> }
>
> - for (int i = 0; i< num_events; i++) {
> - EventWrapper* wrapper;
> - EventSource* event;
> + /* Right now we only use read polling in spice */
> + ready = ::select(maxfd+1,&rfds, NULL, NULL, tvp);
>
> - wrapper = (EventWrapper *)events[i].data.ptr;
> - if ((event = wrapper->get_event())) {
> - event->action();
> + if (ready == -1) {
> + if (errno == EINTR) {
> + return false;
> }
> - wrapper->unref();
> + THROW("wait error select failed");
> + } else if (ready == 0) {
> + return false;
> }
> - return false;
> -}
>
> -void EventSources::add_trigger(Trigger& trigger)
> -{
> - int fd = trigger.get_fd();
> - EventWrapper* wrapper = new EventWrapper(*this, trigger);
> - struct epoll_event event;
> - event.data.ptr = wrapper;
> - event.events = EPOLLIN;
> - if (epoll_ctl(_epoll, EPOLL_CTL_ADD, fd,&event) == -1) {
> - THROW("epoll add failed");
> + for (int i = 0; i< _events.size(); i++) {
> + if (FD_ISSET(_fds[i],&rfds)) {
> + _events[i]->action();
> + return false;
> + }
> }
> - _events.push_back(wrapper);
> + return false;
> }
>
> -void EventSources_p::remove_wrapper(EventWrapper* wrapper)
> +void EventSources::add_trigger(Trigger& trigger)
> {
> - Events::iterator iter = _events.begin();
> - for (;; iter++) {
> - if (iter == _events.end()) {
> - THROW("wrapper not found");
> - }
> - if ((*iter) == wrapper) {
> - _events.erase(iter);
> - return;
> - }
> - }
> + add_event(trigger.get_fd(),&trigger);
> }
>
> void EventSources::remove_trigger(Trigger& trigger)
> {
> - Events::iterator iter = _events.begin();
> - for (;; iter++) {
> - if (iter == _events.end()) {
> - THROW("trigger not found");
> - }
> - if ((*iter)->get_event() ==&trigger) {
> - (*iter)->invalidate();
> - (*iter)->unref();
> - break;
> - }
> - }
> - int fd = trigger.get_fd();
> - if (epoll_ctl(_epoll, EPOLL_CTL_DEL, fd, NULL) == -1) {
> - THROW("epoll remove failed");
> - }
> + remove_event(&trigger);
> }
>
> EventSources::Trigger::Trigger()
> {
> -#ifdef USING_EVENT_FD
> - _event_fd = eventfd(0, 0);
> - if (_event_fd == -1) {
> - THROW("create eventfd failed");
> - }
> -#else
> int fd[2];
> - if (pipe(fd) == -1) {
> + if (::pipe(fd) == -1) {
> THROW("create pipe failed");
> }
> _event_fd = fd[0];
> _event_write_fd = fd[1];
> -#endif
> - int flags;
> - if ((flags = fcntl(_event_fd, F_GETFL)) == -1) {
> - THROW("failed to set eventfd non block: %s", strerror(errno));
> - }
> -
> - if (fcntl(_event_fd, F_SETFL, flags | O_NONBLOCK) == -1) {
> - THROW("failed to set eventfd non block: %s", strerror(errno));
> - }
> + set_non_blocking(_event_fd);
> }
>
> EventSources::Trigger::~Trigger()
> {
> close(_event_fd);
> -#ifndef USING_EVENT_FD
> close(_event_write_fd);
> -#endif
> }
>
> void EventSources::Trigger::trigger()
> @@ -207,8 +159,8 @@ void EventSources::Trigger::trigger()
> return;
> }
> _pending_int = true;
> - static const EVENT_DATA_TYPE val = 1;
> - if (::write(WRITE_FD,&val, sizeof(val)) != sizeof(val)) {
> + const uint8_t val = 1;
> + if (::write(_event_write_fd,&val, sizeof(val)) != sizeof(val)) {
> THROW("write event failed");
> }
> }
> @@ -219,8 +171,8 @@ bool Trigger_p::reset_event()
> if (!_pending_int) {
> return false;
> }
> - EVENT_DATA_TYPE val;
> - if (read(_event_fd,&val, sizeof(val)) != sizeof(val)) {
> + uint8_t val;
> + if (::read(_event_fd,&val, sizeof(val)) != sizeof(val)) {
> THROW("event read error");
> }
> _pending_int = false;
> @@ -239,95 +191,27 @@ void EventSources::Trigger::action()
> }
> }
>
> -static void set_non_blocking(int fd)
> -{
> - int flags;
> - if ((flags = fcntl(fd, F_GETFL)) == -1) {
> - THROW("failed to set socket non block: %s", strerror(errno));
> - }
> -
> - if (fcntl(fd, F_SETFL, flags | O_NONBLOCK) == -1) {
> - THROW("failed to set socket non block: %s", strerror(errno));
> - }
> -}
> -
> -static void set_blocking(int fd)
> -{
> - int flags;
> - if ((flags = fcntl(fd, F_GETFL)) == -1) {
> - THROW("failed to clear socket non block: %s", strerror(errno));
> - }
> -
> - if (fcntl(fd, F_SETFL, flags& ~O_NONBLOCK) == -1) {
> - THROW("failed to clear socket non block: %s", strerror(errno));
> - }
> -}
> -
> -static void add_to_poll(int fd, int epoll, EventWrapper* wrapper)
> -{
> - struct epoll_event event;
> - event.data.ptr = wrapper;
> - event.events = EPOLLIN | EPOLLOUT | EPOLLET;
> - if (epoll_ctl(epoll, EPOLL_CTL_ADD, fd,&event) == -1) {
> - THROW("epoll add failed");
> - }
> -}
> -
> void EventSources::add_socket(Socket& socket)
> {
> - int fd = socket.get_socket();
> - set_non_blocking(fd);
> - EventWrapper* wrapper = new EventWrapper(*this, socket);
> - add_to_poll(fd, _epoll, wrapper);
> - _events.push_back(wrapper);
> -}
> -
> -static bool remove_event(EventSources_p::Events& events, EventSource& event)
> -{
> - EventSources_p::Events::iterator iter = events.begin();
> - for (;; iter++) {
> - if (iter == events.end()) {
> - return false;
> - }
> - if ((*iter)->get_event() ==&event) {
> - (*iter)->invalidate();
> - (*iter)->unref();
> - return true;
> - }
> - }
> + add_event(socket.get_socket(),&socket);
> + set_non_blocking(socket.get_socket());
> }
>
> void EventSources::remove_socket(Socket& socket)
> {
> - if (!remove_event(_events, socket)) {
> - THROW("socket not found");
> - }
> + remove_event(&socket);
> int fd = socket.get_socket();
> - if (epoll_ctl(_epoll, EPOLL_CTL_DEL, fd, NULL) == -1) {
> - THROW("epoll remove failed");
> - }
> set_blocking(fd);
> }
>
> void EventSources::add_file(File& file)
> {
> - int fd = file.get_fd();
> - set_non_blocking(fd);
> - EventWrapper* wrapper = new EventWrapper(*this, file);
> - add_to_poll(fd, _epoll, wrapper);
> - _events.push_back(wrapper);
> + add_event(file.get_fd(),&file);
> }
>
> void EventSources::remove_file(File& file)
> {
> - if (!remove_event(_events, file)) {
> - THROW("file not found");
> - }
> - int fd = file.get_fd();
> - if (epoll_ctl(_epoll, EPOLL_CTL_DEL, fd, NULL) == -1) {
> - THROW("epoll remove failed");
> - }
> - set_blocking(fd);
> + remove_event(&file);
> }
>
> void EventSources::add_handle(Handle& file)
> diff --git a/client/x11/platform.cpp b/client/x11/platform.cpp
> index 1133414..ca5f1d5 100644
> --- a/client/x11/platform.cpp
> +++ b/client/x11/platform.cpp
> @@ -3024,8 +3024,12 @@ void Platform::set_clipboard_listener(ClipboardListener* listener)
> return;
> }
> clipboard_listener = listener;
> - XConvertSelection(x_display, XA_PRIMARY, utf8_atom, clipboard_prop,
> - platform_win, CurrentTime);
> + /* Seems platform_win can be NULL, we'll just ignore that for now.
> + This will be fixed when the rest of cut and paste lands */
> + if (platform_win) {
> + XConvertSelection(x_display, XA_PRIMARY, utf8_atom, clipboard_prop,
> + platform_win, CurrentTime);
> + }
> }
>
> bool Platform::set_clipboard_data(uint32_t type, const uint8_t* data, int32_t size)
>
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
More information about the Spice-devel
mailing list