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.
