I pushed changes to: 1. use PIPE_BUF from limits.h (if available). 2. use apr_file_pipe_timeout_set(<pipe>, 0) on both ends -- just to make absolutely certain that the writes from the critical section are always non-blocking. 3. use apr_poll() at the read end of the pipe to get my read timeout back. 4. make sure any critical-section logging is with APLOG_DEBUG.
http://code.google.com/p/mod-sflow/source/browse/trunk/mod_sflow.c?r=17 Neil On Jan 9, 2011, at 11:03 PM, Neil McKee wrote: > Great feedback, thanks. > > I have several questions, if you don't mind... > > 1. What is the most portable way to make the pipe non-blocking? That way we > can just drop any samples that get EWOULDBLOCK or EAGAIN on writing to the > pipe (and increment the drops counter). apr_file_pipe_create_ex(.... > APR_FULL_NONBLOCK...) was my first choice, but it only seems to have > appeared fairly recently, in APR version 1.3. > > 2. It's very unlikely that we would ever be sending a message on the pipe > greater than about 200 bytes, so if we ever see (msgBytes > PIPE_BUF) then > I'm OK with just dropping that message. PIPE_BUF seems to be defined > without explicitly adding "limits.h" as another include. Can I assume it > will always be defined, or is there something like "APR_PIPE_BUF" I should > use instead? > > 3. Making sure we can't block in the critical section is obviously the most > important thing, but let me know if you still think we need to refine it > further after that. It is my assumption that the sampling-rate setting will > be at least 1-in-100 (and quite possibly 1-in-50000) so the critical path is > really just a few counter increments and a sampling decrement-and-test. If > anyone ever wanted to run it flat out at 1-in-10 or 1-in-1 then obviously > we'd have to look closer at this, but there are other tools for that kind of > logging. > > The sFlow standard allows for multiple independent "sub-agents" to maintain > their own sequence numbers and send their own datagrams (an important > property for a large network switch), so theoretically we could do without > the pipe and have each child process send his own sFlow. We could even have a > separate sub-agent for each worker thread, and run with no locks at all. > However we decided against that because it could mean generating an > uncontrolled number of these sub-agents, which would be antisocial for the > collector. > > Atomic operations might help to avoid thread-locking here, but I was hoping > to avoid the "dark arts". > > 4. The pipe is anonymous. The docs say that some platforms don't allow that > and a filename is required. Do I need to bother with that, or are those > platforms obsolete anyway? > > I apologize if any of these are FAQs / RTFMs. I'm new to APR. > > Neil > > > On Jan 9, 2011, at 12:17 PM, Ben Noordhuis wrote: > >> On Sat, Jan 8, 2011 at 00:06, Neil McKee <neil.mc...@inmon.com> wrote: >>> This module is designed to work in both "prefork" and "worker" models. I >>> would really >>> appreciate it if someone could review the design to make sure I made >>> appropriate choices >>> about where to use pipes, shared-memory, mutex locking, and so on(!) >>> These choices >>> are documented in the comment at the top of the mod_sflow.c file, here: >>> http://code.google.com/p/mod-sflow/source/browse/trunk/mod_sflow.c?r=14 >> >> Neil, two points of critique: >> >> 1. You are doing way too much in your critical section, including >> potentially blocking actions like logging. >> >> 2. Assuming it's safe to write up to 4K to the pipe is dangerous for >> several reasons: PIPE_BUF may be < 4096, the pipe may not be empty, >> etc. This ties in with #1 since you are doing it from within the >> critical section. >
