Seems reasonable. Some notes inline, mostly nits and questions.
On Tue, Sep 4, 2012 at 10:09 PM, Isaku Yamahata <yamah...@valinux.co.jp>wrote:
> eventlet/gevent doesn't work well with select.poll because select.poll
> blocks
> python interpreter as a whole instead of switching from the current thread
> which is about to block to other runnable thread.
> So ovsdb python binding can't be used with eventlet/gevent.
> Emulate select.poll with select.select because using python means that
> performance isn't so important.
>
> Signed-off-by: Isaku Yamahata <yamah...@valinux.co.jp>
> ---
> change v1 -> v2:
> - replace select.poll with select.select for simplicity instead of
> monkey patching
> ---
> python/ovs/poller.py | 71
> ++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 files changed, 69 insertions(+), 2 deletions(-)
>
> diff --git a/python/ovs/poller.py b/python/ovs/poller.py
> index e459c58..e4294e3 100644
> --- a/python/ovs/poller.py
> +++ b/python/ovs/poller.py
> @@ -13,13 +13,80 @@
> # limitations under the License.
>
> import errno
> -import select
> import ovs.timeval
> import ovs.vlog
> +import select
> +import socket
>
> vlog = ovs.vlog.Vlog("poller")
>
>
>
> +# eventlet/gevent that does monkey patch to select module doesn't support
Is monkey patch still in comment on purpose?
+# select.poll. If select.poll is used, python interpreter is blocked as a
> +# whole instead of switching from the current thread that is about to
> block
> +# to other runnable thread.
> +# So emulate select.poll by select.select because python is used so that
> +# performance isn't so important.
>
> +# If eventlet/gevent isn't used, we can use select.poll by replacing
> +# SelectPoll with select.poll class
> +# SelectPoll = select.poll
>
I would put this comment down by the line that says "SelectPoll =
_SelectSelect"
> +class _SelectSelect(object):
> + """ select.poll emulation by using select.select.
> + Only register and poll are needed at the moment.
> + """
> + def __init__(self):
>
> + super(_SelectSelect, self).__init__()
>
This makes me think you are trying to something clever, when (I believe)
you are not. Was there any particular reason to include it?
> + self.rlist = []
> + self.wlist = []
> + self.xlist = []
> +
> + def register(self, fd, events):
> + if isinstance(fd, socket.socket):
> + fd = fd.fileno()
> + assert isinstance(fd, int)
> + if events & select.POLLIN:
> + self.rlist.append(fd)
>
> + events &= ~select.POLLIN
>
I assume this is more typical to write it as this general form than as
events -= select.POLLIN
which is equivalent since you're already in the conditional
> + if events & select.POLLOUT:
> + self.wlist.append(fd)
> + events &= ~select.POLLOUT
> + if events:
> + self.xlist.append(fd)
> +
> + @staticmethod
> + def _events_update(events_dict, fd, event):
> + """Emulate 'collections.defaultdict(int)[fd] |= event' with
> dict"""
>
> + events = events_dict.setdefault(fd, 0)
> + events_dict[fd] = events | event
>
Seems fine, could also be
events_dict[fd] = events_dict.get(fd, 0) | event
+
> + def poll(self, timeout):
> + if timeout == -1:
>
> + # epoll uses -1 for indifinite timeout, select uses None.
>
I think
s/indifinite/infinite/
> + timeout = None
> + else:
> + timeout = float(timeout) / 1000
> +
> + rlist, wlist, xlist = select.select(self.rlist, self.wlist,
> self.xlist,
> + timeout)
> + # collections.defaultdict is introduced by python 2.5 and
> + # XenServer uses python 2.4. We don't use it for XenServer.
> + # events_dict = collections.defaultdict(int)
> + # events_dict[fd] |= event
> + events_dict = {}
> + for fd in rlist:
> + self._events_update(events_dict, fd, select.POLLIN)
> + for fd in wlist:
> + self._events_update(events_dict, fd, select.POLLOUT)
> + for fd in xlist:
> + self._events_update(events_dict, fd, select.POLLERR |
> + select.POLLHUP | select.POLLNVAL)
>
> + return [(fd, events) for (fd, events) in events_dict.items()]
>
This can just be
return events_dict.items()
and save the noop list comprehension. If you're worried about the
return value being confusing, could add a comment like
# Returns a list of (fd, events) tuples
+
> +
> +SelectPoll = _SelectSelect
> +# SelectPoll = select.poll
>
Did you deliberately not add a _ to SelectPoll? I didn't see any
external users, and they could always check Poller.poll if they needed
to see what version it was using.
> +
> +
> class Poller(object):
> """High-level wrapper around the "poll" system call.
>
> @@ -122,5 +189,5 @@ class Poller(object):
> vlog.dbg("%s on fd %d" % (s, fd))
>
> def __reset(self):
> - self.poll = select.poll()
> + self.poll = SelectPoll()
> self.timeout = -1
> --
> 1.7.1.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
