On Feb 22, 2016, at 10:18 PM, IWAMOTO Toshihiro <[email protected]> wrote: > > on an unrelated note, have you considered going for true > multi-threading (Python's native threading for example) as eventlet > cannot make use of multiple CPUs? >
True multi-threading in Python isn't really, because of the GIL. Eventlet does pretty well, all things considered, despite the single CPU limitation. >> >> + priority, count, (ev, state) = >> self.events.get(timeout=self._event_get_timeout) > > As this timeout does nothing useful in the code, it needs to be > eliminated unless proved to have some positive effect. > I disagree. Given the fact that I have found buggy behavior in Eventlet's synchronized queues, this timeout is a form of insurance. It is intended to *force* an exit from get() on a regular basis. The same thing occurs in the recv_loop of controller.py, but via a socket timeout instead. It's not that the timeout is doing "nothing useful." It implements a trade: slightly more overhead, in order to ensure that the queue does not get wedged in the get() forever somehow. If I felt that I could *trust* Eventlet's queue implementation, I would not be arguing with you. > > Could you make a separate patch for typo fixes? > Certainly. > Moving this config opt from register_cli_opts may break existing > appliciations. > I can move it back - but it's only been in the code for one release, and I added it. I simply figured it would be more consistent to have it be a config file option. Whichever you'd prefer; I don't think that significant breakage would be caused. >> >> def _get_ports(self): >> - if (self.ofproto_parser is not None and >> - self.ofproto_parser.ofproto.OFP_VERSION >= 0x04): >> + if (self.ofproto_parser and >> + (self.ofproto_parser.ofproto.OFP_VERSION >= 0x04)): > > Do you need this change? > No. This is merely a cleanup for consistency/clarity. I could add it to the typo patch, or remove it entirely. >> >> - if (len(ret) == 0) or (self.close_requested): >> - self.socket.close() > > Why do you want to remove this close() call? > There should *only* be one call to close(). I moved it to a central location for consistency and simplicity of the code. It made it easier to structure the code so that shutdown() could be called before close(), and thereby ensuring that recv() properly terminated. Moving the socket close() from that location made the code easier to structure correctly. >> >> + buf = self.send_q.get(timeout=self._send_q_timeout) > > Just like above, this timeout doesn't seem to be necessary. > And, just as above, this is a bit of "insurance." It's here to ensure that the tasklet regularly "wakes up." My tests with heavy event load have caused me to have *serious* doubts about how well the wakeup mechanisms in Eventlet's queues work. I can certainly remove these timeouts, but I have reservations about doing so. I do not think that the *minimal* overhead that they introduce is significant enough to merit their removal. So - the issues, as I see them: 1) We have a fundamental disagreement about the necessity of the timeouts on get(). If I understand your position - you contend that they have no merit whatsoever. I contend (perhaps incorrectly) that they add minimal overhead and ensure that the get() operations have a regular "wakeup" to prevent potential missed wakeups. My desire for regular wakeups is predicated on my testing experiences, which *did* prove a need for the semaphores - but may *not* have proved a need for the timeouts. 2) I need to separate out my typo corrections into a separate patch. 3) Do we believe that the socket-timeout being a CLI option, for a single release, is enough to have caused many users to have integrated it into scripts? If so, I will revert the socket-timeout to being a CLI option. I believe that it was an error on my part to make it a CLI option in the first place - but if there is significant fear of breaking user applications, I can ensure that the behavior remains the same. Personally, I do not believe that many users will have integrated that option into scripts, after a single release. I will await a judgment call regarding issues (1) and (3) before re-submitting this patch and the associated typo patch; I must say, however, that I respectfully disagree with your positions. Best, Victor -- Victor J. Orlikowski <> vjo@[cs.]duke.edu ------------------------------------------------------------------------------ Site24x7 APM Insight: Get Deep Visibility into Application Performance APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month Monitor end-to-end web transactions and take corrective actions now Troubleshoot faster and improve end-user experience. Signup Now! http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140 _______________________________________________ Ryu-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/ryu-devel
