On Mon, 11 Oct 2010 14:58:34 +0100, Peter Payne <sourcefo...@pirosa.co.uk> wrote: > Hello Amos, > > thank you for your response. Please find attached the four files in > question. > > Companies to mention as sponsors are BBC (UK), Siemens IT Solutions and > Services (UK). > > Credit to Peter Payne, Pirosa Limited UK (no e-mail please).
Sure. No problem. > > Squid bug 3057 was raised by another member of Siemens IT Solutions and > Services during testing of Squid 3.1.8 on Solaris - that bug was not > produced on the 32-bit compile, just the 64-bit compile. It is not > thought this is related to the /dev/poll support added here. Okay. On to the audit. This is just a 3.x style and consistency check. I'll take it as tested and working. comm_devpoll.cc: please make the following updates - use "#if USE_DEVPOLL" instead of #ifdef. - move all #includes except squid.h inside the USE_DEVPOLL wrapper. - move the "/* Solaris /dev/poll support */" to its own line or remove completely. - remove the big "XXXX...XXXX" lines. - use /** for the function documentation. If you know doxygen syntax please markup the text in any other relevant ways as well. - replace all debugs "DEBUG_DEVPOLL ? 0 : 8" with "5" or "7" to match the other comm loops display density. - replace comm_update_fd the #if DEBUG_DEVPOLL with (?:) constructs in the debugs() ie << (events & POLLIN ? " IN":"") - in commSetSelect please remove the no-op else cases. "// else: we want ... " comments are fine. - comm_select please use HERE macro in debugs() instead of the text name of the function. squid-include.diff all done by autoconf during packaging. This can be dropped. squid-src.diff: - automake changes need to be made in Makefile.am. This can be done on commit. - unlinkd.cc changes okay. squid-compat.diff: - changes okay. - but can you mention why /dev/poll limit is hard-coded? compat area needs enough details so we can track the when and why for each hack. The aim being to remove them when the problems have been resolved. squid-root.diff: - okay as-is for 3.1. - for 3.2 and later we have simpler configure.in logics with some helper macros. That can be cut-n-pasted on commit. - RUNIFELSE is a bigger problem. It breaks cross-compiling and I notice its not present in 2.7. Is the configure-time run test marked "Verify that /dev/poll really works" actually needed? What will happen to builds made on non-Solaris with forced devpoll and the kernel headers present? In configure.in the "magic" if statement (hunk: @@ -3013,6 +3066,8 @@) has a specific order from fastest to slowest. Can you please point me/us at any reliable /dev/poll benchmarking comparisons with epoll/kqueue to justify placing it where you did? Thank you. > > Note that while porting comm_devpoll.cc I have suspicions that the > comm_epoll.cc commResetSelect() function does nothing (i.e., it calls > commSetSelect with no type flag, which I suspect is a no-op for all > intents and purposes). I haven't confirmed this, however, so haven't > raised it as a bug. Someone may want to double-check. > Hmm. Yes. It stops the event FD being polled, but does not unset the handler like all the other loops. I've not yet learned the details of epoll well, if you think doing a no-op there is incorrect please make the bug report about it. Amos