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

On 11/09/10 18:10, Alex Rousskov wrote:
> 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.

  * 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?

* 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).

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

* 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.

* 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;

* 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.

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

Amos

-- 
Please be using
   Current Stable Squid 2.7.STABLE9 or 3.1.8
   Beta testers wanted for 3.2.0.2