Re: [PATCH] Solaris /dev/poll support for Squid 3 (how can I contribute)

On Mon, 11 Oct 2010 14:58:34 +0100, Peter Payne <sourceforge_at_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
Received on Mon Oct 11 2010 - 23:36:30 MDT