Re: [MERGE] SMP implementation, part 1

Alex Rousskov wrote:
> Hello,
>
> Attached is the first part of the project making Squid SMP-scalable.
> The implementation follows SMP design discussed a few months ago (quoted
> below). The patch contains most of the promised Phase1 features. There
> is more code brewing in the lab, but the attached code is ready to be
> synced and merged with trunk. With these changes, you can run Squid in
> SMP mode, utilizing many CPU cores, provided you do not mind cache
> manager and caching inconsistencies.
>
> A brief list of changes and to-dos is provided below. Besides the
> acceptance review, I need help with these two questions:
>
> 1. Should we rename the squid.conf option specifying the number of
> processes from "main_processes" to "worker_processes" or even "workers"?
> The forked processes do almost everything the old daemon process did,
> but that will change as we add more specialized processes. There is also
> a special "Coordinator" process that is launched automatically in SMP
> mode. It does not handle regular HTTP transactions.

Workers brings to mind the right meaning from use by apache and other
software. +1 from me on calling it worker_processes (we will have
threads later).

>
> 2. We had to clone comm_openex into comm_open_uds because Unix Domain
> Sockets do not use IP addresses. They use file names. We can unify and
> simplify related code if we add the file name (struct sockaddr_un) to
> Ip::Address. IIRC, Amos and I discussed this on IRC and decided that it
> is OK to add sockaddr_un to Ip::Address (because it is used as a general
> "network address" anyway), but I would prefer to hear more opinions
> before altering Ip::Address.
>
>
> Change log:
>
> * Added main_processes squid.conf option to specify how many worker
> processes to fork and maintain. Zero means old no-deamon mode. One means
> the old non-SMP mode.
>
> * Added support for process_name and process_number macros and
> if-statement conditionals in squid.conf. Search for .pre changes for
> documented details. These features allow the admin to configure each
> worker process differently if needed.

Regarding the parser, use of un-bracketed assignment in conditionals as
Henrik keeps reminding me these fail on some compile modes.
  You are using a number of if(a=b) and when(a=b) statements,
particularly in the parser. These will need to become if((a=b)) etc.

>
> * Support multiple workers listening on the same HTTP[S] port (port
> sharing). This allows multiple workers to split the load without any
> special rules.

You add a comment question:
   fatal("No HTTP or HTTPS ports configured"); // defaults prohibit this?

... not quite. The defaults make an entry in the config for new
installs, if its removed or an old 2.5 config without port is used that
error case still occurs.

The MacOS people have an open bug requesting the Mac-service -I option
get ported to 3.x which will mean no port configured by default, but an
open socket passed in on stdin to the master process later.

>
> * Support or prohibit port sharing for WCCP, DNS, ICP, HTCP, SMP, and
> Ident protocols, depending on protocol-specific restrictions. Sharing is
> implemented by registering listening socket descriptors with the
> Coordinator process and obtaining them from the Coordinator as needed.
> Here are protocol-specific notes:
>
> WCCP: Restricted to the Coordinator process due to how WCCP works.
> Workers do not need access to the WCCP code.
>
> DNS: Done by each worker with no sharing. Fixed source ports not
> supported unless each worker is given its own outgoing address
> because we do not want to match outgoing queries and incoming
> responses across processes.

This is a good reason for adding the often pondered dns_outgoing_address
directive. It solves a few issues in other low-priority use cases as well.

>
> SNMP: Workers share incoming and outgoing sockets.

Does this really make sense?
   SNMP stats will be different for each worker and particularly in the
client table where things are indexed by the particular client IPs which
connected to each worker.
   I think better a single port managed by the master with a new OID for
child process somewhere. But the design will need a good thinking out
separate to this.

>
> ICP and HTCP _clients_: Cannot be supported in SMP environment
> unless each process has its own address (i.e., unique IP address
> and/or unique [ICP] port) because we do not want to match outgoing
> queries and incoming responses across processes.

Huh? "or unique port" this is entirely doable right?

>
> ICP and HTCP _servers_: share listening sockets.
>
> Ident clients do not need to share sockets because they use
> unique ports.
>
> * Support management signals (squid -k ...) in SMP mode, acting as a
> single Squid instance.
>
> * Refork dying workers, similar to how we reforked dying process in
> non-SMP daemon mode.

Um, have you checked if this new process structure obsoletes the old
kill-parent hack? That would be nice to kill off cleanly.

>
> Detailed change descriptions are at
> https://code.launchpad.net/~rousskov/squid/smp
>
>
> To do (all these projects are in-progress now):
>
> - SMP-aware cache manager.

This may be impacted by the local file server extension changes (PAC
stuff). Making the internal server requests accept http(s):// namespace
will mean cachemgr requests can also be requested direct from a browser.

> - SMP-aware caching.
> - Publish performance testing results.
> - CPU affinity controls.
>
> Thank you,
>
> Alex.
> P.S. The merge bundle is against trunk r10303. I will sync if approved.
> The configure.in change is not meant for merging and will be excluded
> when the code is synced and merged. I do not know how to exclude it in
> the merge bundle.
>

No prob. We need to get together to sync/plan the order of merges for
this month I think. I'll try and get on IRC within the day.

Now the code...

* Note: Careful with allocation methods for addrinfo. IIRC there were
some problems if the sockaddr field was on the stack. Been a while so I
can't point my finger at specifics any more, but it's worth watching for
when this code goes mainstream.

* debugLogKid() looks like it results in the static buffer being filled
with identical content on every debug write.
   - The snprintf() needs to be wrapped by something that checks if the
static buf has been set yet or not. Perhapse allocating buf with nul
content and always returning it.
   - If the process is called a "worker" the log output should probably
be "worker%d" to avoid confusion. Even if the class is Kid internally.
   - given the first point the coordinator could return its
"coordinator" string or somesuch for debugging.

* Ipc::Coordinator::findStrand() should be const.

* CBDATA_CLASS2(Coordinator) needs to be the last entry in the class
definition for consistency and long-term safety the way it plays with
public/private.

* Kids::find() and Kids::get() methods should be const.

* Ipc::UdsSender::UdsSender contains TODO about retries and timeout
being configurable.... Yes please. sub-options to worker_processes N
seems appropriate. With default less than 10s on timeout please. We
already get complaints if total startup takes more than ~15 seconds.

* code formatting is needed in quite a few places. Do you have a machine
with the right version of astyle available? Or you can checkout to
squid-cache.org and format there please. It will likely help the update
merge.

* src/main.cc - while doing this process rework would you mind merging
all those main.cc signals and option flag globals into a single struct
or public class with one static global? or at least make the new flags
you need to add use one such structure which we can later migrate the
old flags into?

* src/main.cc - prog = xstrdup(argv[0]); /* XXX: leak */
   prog should be a stack field of the function yes? which gets strcpy()
filled out. POSIX limits on filename and path length can be used to
define the variable size.

* src/protos.h - please do not add new stuff to this file.

* test-suite/testheaders.sh - whats with those bits as well?

Apart from those small bits it looks good.

Big Picture bits...

* How does the socket sharing mesh with the worker Comm::ListenStateData
accept() classes so far in practice? particularly the AcceptLimiter.

* Do all workers share all http_port directives still?
   I'm contemplating ways to expand a client-contactable port for
ERR_AGENT_CONFIGURE/ERR_AGENT_PACFILE page macros and correct internal
request URLs. The best contender is walking the http_port config lines
to find one without special-mode flags. But this depends on having
access to that global info in the worker.

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.4