[squid-dev] SMP scaling in no_daemon mode?

[Alex Rousskov]

On 06/09/2017 05:34 AM, Andreas Weigel wrote:

> - a SIGTERM  to the master process makes it wait for its children to
> shutdown for 30 seconds
>     --> Is that desired behavior?

The question is out of scope, but the answer is "yes". See
shutdown_lifetime in squid.conf.documented.


> I would greatly appreciate feedback to the proposed patch.

Your --foreground fix itself (~10 lines) looks good to me, thank you!
The rest is problematic. Details below.


Please undo the following two changes and all their side effects:

> -bool InDaemonMode(); // try using specific Iam*() checks above first
> +bool UsingDedicatedMaster(); // try using specific Iam*() checks above first

> -extern int opt_no_daemon; /* 0 */
> +extern int opt_use_single_process; /* 0 */

These renames obscure functionality changes, make v3.5 acceptance less
appealing, and I disagree with the new names and some of the new
descriptions. I agree that things can be and perhaps should be renamed,
but it would be better to do that separately (in v5?).


> -            "       -N        No daemon mode.\n"
> +            "       -N        Use a single process (combining master/worker) running in "
> +            "                 foreground; incompatible with --foreground.\n"
>              "       --foreground\n"
> -            "                 Parent process does not exit until its children have finished.\n"
> +            "                 \"No daemon\" mode: master process stays in foreground.\n"

Unlike the changes discussed earlier, these changes are (barely) in
scope because we may want to clarify what --foreground means since its
initial implementation was broken, potentially because the developer did
not know what --foreground should mean.

The "single process" terminology is misleading because Squid may start
lots of processes (helpers) even with -N. The "stays" terminology is
inaccurate because a given process cannot go into background itself.

I suggest the following (without proper formatting):

-N: Master process runs in foreground and is a worker. No kids.
--foreground: Master process runs in foreground and creates worker kids.

>From the admin perspective, the only difference between -N and
--foreground is kids creation/maintenance. My version emphasizes that
difference rather than obscuring it.

If we end up removing -N someday, the above --foreground description
will change to "No daemon: Master process runs in foreground".

Please note that --help is not the manual page (yet?) but a quick
reminder/summary. We should document details in --help. Things like
options incompatibility do not really belong to --help, especially since
Squid quickly detects and explains the conflict if both options are used.


> +<p>The squid binary now has a new <em>--foreground</em> command line option,
> +   which prevents the process from daemonizing, i.e., launches only one process,
> +   which will become the master process. Unlike the old <em>-N</em> option,

I suggest (without proper formatting):

<p>The squid binary has a new <em>--foreground</em> command line option
which (only) prevents daemonizing the master process. Unlike the old
<em>-N</em> option,



>  IamWorkerProcess()
>  {
>      // when there is only one process, it has to be the worker
> -    if (opt_no_daemon || Config.workers == 0)
> +    if (opt_use_single_process)
>          return true;
>  
>      return TheProcessKind == pkWorker;

Why did you remove the (Config.workers == 0) part? What breaks if you
keep it? Is fixing that in scope?


>  bool
> -InDaemonMode()
> +UsingDedicatedMaster()
>  {
> -    return !opt_no_daemon && Config.workers > 0;
> +    return !opt_use_single_process;
>  }

Why did you remove the (Config.workers > 0) part? What breaks if you
keep it? Is fixing that in scope?

There may be bugs in the above code, but if they are not related to
fixing --foreground then we should not fix them here...


> +        if ((pid = fork()) < 0) {
> +            int xerrno = errno;
> +            syslog(LOG_ALERT, "fork failed: %s", xstrerr(xerrno));
> +        } else if (pid > 0) {
> +            // finished daemonizing
> +            exit(0);
> +        }
> +
> +        // makes only sense after daemonizing
> +        if (setsid() < 0) {
> +            int xerrno = errno;
> +            syslog(LOG_ALERT, "setsid failed: %s", xstrerr(xerrno));
> +        }

Please restore the "parent" comment and add "daemonized child" comment
to improve code readability. I would also clarify what happens when
fork() fails.

Please note that your "makes only sense after daemonizing" comment kind
of contradicts the code because setsid() is also called when fork() fails.

Here is a polished version, taking the above comments into account:

    if ((pid = fork()) < 0) {
        const int xerrno = errno;
        syslog(LOG_ALERT, "fork failed: %s", xstrerr(xerrno));
        // continue anyway, mimicking --foreground mode (XXX?)
    } else if (pid > 0) {
        // parent
        exit(0); // successfully daemonized; TODO: Use EXIT_SUCCESS
    } else {
        // daemonized child (now the master process)
        // create a new session with the master process as the leader
        if (setsid() < 0) {
            const int xerrno = errno;
            syslog(LOG_ALERT, "setsid failed: %s", xstrerr(xerrno));
        }
    }

I recommend moving the above code into a dedicated GoIntoBackground()
static void function:

    if (!opt_foreground)
        GoIntoBackground();


> +        // makes only sense after daemonizing
> +        if (setsid() < 0) {

Does setsid() have any effect if called in --foreground mode? I suspect
it does. In other words, I suspect that it is possible to detect
setsid() call presence in some environments where --foreground is used.
Please note that I am not asking whether those effects are
useful/good/etc.. I am only asking whether they exist.

If you are not sure that setsid() has no effect in --foreground, then
please do not change this aspect; move the setsid() call outside the
opt_foreground check. Add a TODO comment if this needs further
investigation/work.


Thank you,

Alex.