On Mon, 20 Sep 2010 17:44:17 -0600, Alex Rousskov <rouss...@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
