[PATCH] Add cpu_affinity_map configuration option to bind workers to cores

Add cpu_affinity_map configuration option to bind workers to CPU cores.

Allows the admin to place all workers on individual cores without
writing a lot of if-statements. For example,

     cpu_affinity_map process_numbers=1,2,3,4 cores=1,3,5,7

will have an effect for kids 1 through 4 only and will place them on
even cores starting with core #1.

If there are conflicts for a given process, the latest option wins and a
warning is printed. If the number of specified processes do not match
the number of specified cores, Squid quits with an error. Multiple
cpu_affinity_map options are merged.

Squid builds on systems without Linux CPU affinity calls, but setting
affinity only works if there are sched_getaffinity(2) and
sched_setaffinity(2) available. If there is no OS support but
cpu_affinity options are configured, Squid quits with an error. If there
is OS support but calls fail, Squid prints an error but does not quit.

----------

This is take10 of the patch, addressing Amos' comments as discussed below:

On 09/14/2010 12:00 AM, Amos Jeffries wrote:

> * since there is a gathering collection of SMP CPU handling bits do you
> think its appropriate to add a directory cpu/ to collect these extra SMP
> bits? (we have other cpu-specific code for the profiler that might
> collect there later.)
> ** or to at least use base/ for the new code?

Sorry, I do not.

I struggled with the decision where to put this stuff and have
considered both cpu/ and base/. The former seemed premature because
there is too little code for the whole directory and because the scope
would not be clear. The latter seemed wrong because these are not really
base classes that lots of code would use.

Henrik and I discussed your question on the IRC. We did not have the
same opinion as to what the best location is, but we agreed that this
new code should simmer before it is clear where to put it. To minimize
work and confusion, I left it in src/ (Henrik was suggesting
src/staging/ or similar).

> * CpuAffinityMap::add does not need to use error variable both the
> boolean tests setting error can can return false immediately.
> - or they should output debug ERROR/FATAL about the problem.
> - note also that regardless of this the case where error gets set
> mid-loop theProcesses and/or theCores then gets filled with corrupt data
> (n<0).

Fixed. Good catch.

> * does cpuAffinityMap = NULL in free_CpuAffinityMap not work? NULL is
> not always 0.

Fixed. The code was clearly inconsistent with Squid style.

It is best to use 0 in C++ programs, but we have too much NULL-using
code to fight.

> * The #else case in parse_CpuAffinityMap should be a FATAL: since it
> aborts the process.
> ** also, please add a nice FATAL: explanation for the abort in the #if
> case. You may need to break the compound if() into several for
> self_destruct only prints the annoying "Bungled" message.

Fixed.

IMO, we should be migrating from FATAL to ERROR labels and exceptions in
the parser so that we can nicely skip invalid configurations during
reconfigurations, but that is outside this patch scope.

> * NumberofKids does not exactly match its name. NumberOfProcesses()
> would be better, or omitting the +1 in SMP mode to actually report the
> number of *kids* and adding it explicitly where the coordinator is
> needing to be accounted as well.
> - doing it the second way the would make CpuAffinityCheck and
> CpuAffinityInit only need:
> const int numberOfProcesses = NumberOfKids()+1;

Tried to clarify why NumberOfKids() is correct by adding a few source
code comments.

You might be confusing processes with kids with workers, which is not
surprising given the evolving terminology and overlapping scopes.
Happens to me too, and I wrote or designed most of that code! Kids are
processes started by the Master Process. Watch_child in main.cc of the
Master Process is waiting for them. Coordinator process is one of the kids.

Workers are Squid-like processes with no specialized function or powers.
They do what "squid -N" does, essentially. Depending on configuration,
there may be no kids, but there has to be at least one worker process.

I did not want to add a global NumberOfProcesses() function call because
we do not account for many processes (e.g., helpers) and it would not be
clear that the call counts just the Kid processes.

Why the current code pieces seem correct in isolation, I think we will
change the terminology to something simpler and more coherent once we
have more experience with this stuff. Or perhaps it will feel more
coherent with time :-).

> * The bits being added to tools.cc and protos.h will need to be pulled
> out again very shortly into their own file/lib along with the previous
> SMP additions there. The misc unrelated dependencies pulled in by tools
> is causing mess.

Agreed. It is just not clear where to put them yet. They are
globally-used tools not specific to IPC code or CPU-management. We could
add something like smp/, but I agree with Henrik that it makes sense to
let this "not yet sure what to do with it" stuff to simmer for a while.

> * grumbles about structs.h too. But this is sort of okay since the
> global config is a good places for this to be.

Agreed.

Thank you,

Alex.