[squid-dev] SMP scaling in no_daemon mode?

[Alex Rousskov]

On 06/09/2017 09:21 AM, Andreas Weigel wrote:
> Alex Rousskov wrote:

>> I would _not_ change that terminology now because
>> all the renaming will obfuscate your true fix and make it more difficult
>> to understand/review.

> I have to disagree to that one.

Renaming obfuscated your true fix. It is a fact. It is not an opinion
that we can agree or disagree with.


> Looking at the Code for 3.5 I was very confused by the combination of
> Config.workers and opt_no_daemon. The names of options and functions did
> their best to add to the confusion. 

I agree that a lot of things can be fixed and improved. Just do not do
those fixes and improvements in the tiny patch fixing --foreground. Do
it separately, after the --foreground code is fixed. This is _not_ one
of those cases where fixing something is impossible without
rewriting/renaming a lot of related code.

Moreover, if rewriting/renaming a lot of related code was necessary for
you to arrive at the correct fix, it still does not imply that you
cannot separate the two after the fact. Yes, that separation can be
viewed as "overhead", but that "overhead" may be necessary to move
things along in a collaborative project. Convincing others that your
changes are desirable is an (expensive) part of the Squid game. FWIW, I
often spend more resources convincing others that a patch should be
committed then on actually making that patch!


> I think we agreed that due to
> historical reasons, the -N option is doing something different now
> (v4/trunk) from what its description is implying. My (v4/trunk) patch
> therefore changes the names of this option and the function to retrieve
> its value to be more clear, as well as the description of the feature.

What is more clear for you may look wrong to others. This is a separate
issue/problem. It should be solved in a separate patch (which I, FWIW,
would welcome and promise to review).


> I do think that my changes are all related to the same thing, namely 
> allowing a "no daemon" mode with --foreground while retaining the old
> -N functionality, but calling the latter by a more apt name.

They are related but they hurt understanding of the core fix and they
can be done separately. Thus, they should be done separately. See my
patch review for details.


> I do not expect my patch to be applicable to v3.5. Too many
> changes there plus the --foreground option is not even available here.

Ah, I thought it was. Thank you for correcting my understanding. There
is no need to change v3.5 then!

This change does not affect the core of my "divide and conquer" arguments.


>> When daemonized, Squid master process does not support reconfiguration
>> at all. There are plans to change that, but those difficult changes are
>> completely outside your project scope.

> My impression was that the current v4/trunk branches do already allow
> exactly that

They do not. You are confusing worker reconfiguration with master
reconfiguration. Kid/worker reconfiguration is supported. Master
reconfiguration is not supported (unless that master is also a worker).

IIRC, the context of this part of the discussion is whether one can
reconfigure the number of kids. One cannot. You did not change that, and
you do not need to worry about that.


> With the --foreground option working as expected, however, a supervising
> process may send signals to its initial child process, which currently
> (v4/trunk) will always be the squid master. Thereby, the PID file (and
> all possible race conditions associated with it) are not even necessary.

This is a common misunderstanding. The PID file is necessary for many
things beyond supervisor's signaling needs. For example, it is
impossible to reliably start an SMP Squid instance without a PID file.


> While this does not seem to be the intended interface, it IMHO is an
> improvement to determining the process ID by reading some PID file and
> works great for supervision schemes that do not like daemons.

If you know the master PID and just need to send it some signal, then
you do not need a PID file. In many other use cases, you (and Squid
itself) do need that file. (OT: The right signal to do X is not a part
of Squid's interface, so a supervisor sending raw signals to Squid is
using private Squid details that may change. The public interface is
"squid -k X". That -k interface has its own problems, but those problems
do not change the fact that -k is the official signaling interface.)


> PS: I do understand your reluctance to change function/variable names
> and/or description of command line options. I still think it is better
> to have those updated,

Agreed. We disagree _where_ that update should happen, not whether it
should happen.


> even if it may slightly obfuscate the actual
> behavioral change. I also believe in the broken window theorem and try
> to at least fix some of those from time to time.

Your patch was replacing broken windows during a shootout with a
murderer. Not a good strategy, even if you believe in the broken window
theory!


> I once [...] encountered diverse
> variables/macros referring to the same concept but used differently
> throughout the code (Squid_MaxFd, SQUID_MAXFD), eventually caused a
> dangerous segfault (see bugs.squid-cache.org/show_bug.cgi?id=4650 if you
> are interested -- the patch I proposed there also tried to fix the issue
> by introducing various renames, which is probably the reason that no one
> ever cared to look at it ;-/ )

FYI: IIRC, I did look at that patch but could not comment because the
patch scope was defined as "the changes I proposed [in a large comment
discussing various problems]" and the patch itself appeared to contain a
mix of renames and bug fixes. It would take me a long time to understand
the issue and to separate the core changes from the renames. I do not
have that time, unfortunately.

The lack of progress on that bug does not mean that your changes were
wrong or ignored! It means that you need to do more than to improve
code. You need to convince others that your improvements should be
committed. That takes more than writing code. The associated overheads
are unfortunate, but there is no other way that works here AFAIK.


Thank you,

Alex.