[squid-dev] SMP scaling in no_daemon mode?

[Alex Rousskov]

On 06/16/2017 05:11 AM, Amos Jeffries wrote:

> However, the content of that change is enough for me to -1 veto this patch.

I am afraid you misunderstood what the change does.


> It is explicitly adding a regression in a very necessary behaviour for
> startup sequences running what is effectively "squid --foreground -z &&
> squid".

AFAICT, "squid --foreground -z && squid" (or equivalent) works the same
in patched and unpatched Squid as far as waiting for -z to end is
concerned. Both patched and unpatched --foreground Squids wait for the
child processes to finish:

Unpatched --foreground Squid:
* the initial process starts and waits for the master process
* the master process starts and waits for the kids

Patched --foreground Squid:
* the initial process _is_ the master process
* the master process starts and waits for the kids


> We will regress to the state of having race conditions as
> multiple independent sets of workers are spawned and collide with each
> others /dev/shm and whether or not the disk cache is formatted before
> Squid starts accepting client connections.

Patched Squid has the same protection from startup race conditions as
the unpatched Squid. That protection comes from the PID file management
code, and the patch does not change that code.


> The original notes wording (and --foreground design) was intended to be
> clear that in that made a Squid SMP-enabled instance operate as a
> consistent / atomic whole. **No background parts still operating after
> the main process exit()'s**.

That is still the case. AFAICT, the original --foreground design was
correct. The proposed patch fixes its implementation. It does not change
the design.


> Also, there are (or will be) installations containing "squid
> --foreground -N "

The patch does not change the effects of that combination AFAICT and,
hence, should not be responsible for those effects except for the
documentation changes.


>  - the updated documentation implies that --foreground actively creates
> workers. Which will leave anyone with that config confused about what
> happens if they are both required (--foreground "and creates worker
> kids") and prohibited (-N).

Squid tells exactly what happens in that malformed configuration case:

  WARNING: --foreground command-line option has no effect with -N.

If this part of your complaint is about patched documentation, then
please suggest a better wording (without turning --help or release notes
into a user manual!). I actually like the conflict between -N and
--foreground behavior described by --help. That conflict highlights the
fact that the two options are not compatible and should not be used
together!

Unpatched descriptions:

* -N is accurate but requires terminology lookup
* --foreground is vague to the point of being useless
* the combination is conflicting if one knows what "daemon mode" is

Patched descriptions:

* -N is accurate but requires terminology lookup
* --foreground is accurate but requires terminology lookup
* the combination is conflicting even without terminology knowledge


> Yet Squid runs with nary a message about problems.

IMHO, Squid should quit with an ERROR instead of running with a WARNING,
but the patch does not change that behavior so we should not discuss it
here.


I hope my explanation of what the patch does will allow you to change
your vote.


Thank you,

Alex.



>> On 15.06.2017 17:35, Alex Rousskov wrote:
>>> On 06/15/2017 04:57 AM, Andreas Weigel wrote:
>>>>  From discussion on squid-dev, the following behavior is implemented by
>>>> this patch:
>>>>
>>>> * -N: The initial process is a master and a worker process.
>>>>    No kids.
>>>>    No daemonimization.
>>>>
>>>> * --foreground: The initial process is the master process.
>>>>    One or more worker kids (depending on workers=N).
>>>>    No daemonimization.
>>>>
>>>> * neither: The initial process double-forks the master process.
>>>>    One or more worker kids (depending on workers=N).
>>>>    Daemonimization.