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

On Mon, 20 Sep 2010 17:44:17 -0600, Alex Rousskov
<rousskov_at_measurement-factory.com> wrote:
<snip>
>
> This is take10 of the patch, addressing Amos' comments as discussed
below:
>
> On 09/14/2010 12:00 AM, Amos Jeffries wrote:
>
<snip>
>> * 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.

Easy enough to fix with a grep/sed. Do we make '0' a coding style
requirement?
The 'NULL not always 0' is relevant to Win32 builds with MS Visual Studio
which sets NULL == 0xCDCDCDCD (the kernels invalid RAM pattern, somewhere
out in invalid memory space). I'm not sure if the newer VS still do this.

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

I agree we should be recovering where possible with 'soft' ERROR/WARNING
instead of halting.

It is labeling hard exit(!=0)/abort()/self_destruct() halting as anything
other than FATAL that I disagree with.

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

Okay. I was confusing workers with kids.

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

Ok.

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

Now is good. Before too many people have played with it and got their
minds around the current naming.

How about:
 kid - "a" process started by Squid for doing "stuff".
 worker - a specialist kid performing HTTP request handling.
 coordinator - specialized process for managing a collection of kids

For now we have a special case where coordinator==worker +
unlinkd/pinger/helper specialized kids.

Which may or may not be a good thing to carry on into the future, but
certainly simplifies debugging and testing.

If you agree on that the numberOf*() function would be closer to
numberOfWorkers() with no special cases needing mention. It scales to Kids
if you set the affinity for helpers as well.

+1 on what you have now. Discussion points are all out of scope.

Amos
Received on Tue Sep 21 2010 - 05:24:32 MDT