On Wed, 2016-04-20 at 09:59 +0200, Jakub Hrozek wrote:
On Tue, Apr 05, 2016 at 02:54:10PM -0400, Simo Sorce wrote:
> On Tue, 2016-04-05 at 12:57 -0400, Simo Sorce wrote:
> > Thanks, IIRC the int-instead of enum use is intentional, I will look
> > at the others.
>
> The last coverity/clang thing is a false positive, but I initialized
> reply to NULL anyway, I expect now it will start complaining of possible
> NULL dereference :-)
>
> Attached find patches that fixes all other issues (hopefully), one of
> them simply dropped an entire function as it turned out I wasn't using
> it.
>
> Simo.
>
> --
> Simo Sorce * Red Hat, Inc * New York
Ugh, it took me too long to get to this review properly. Sorry about
this..
Since this patchset is large, I will send replies in batches.
> From e215e5534bb56f3887521443ce6c77d13ea3518d Mon Sep 17 00:00:00 2001
> From: Simo Sorce <simo(a)redhat.com>
> Date: Tue, 12 Jan 2016 20:07:59 -0500
> Subject: [PATCH 01/15] Util: Add watchdog helper
ACK
I know Pavel Brezina also reviewed this patch earlier in a separate thread,
so note to self: Add his RB :-)
> From 0dff46755af6063ed4b0339020ae5bb686692de1 Mon Sep 17 00:00:00 2001
> From: Simo Sorce <simo(a)redhat.com>
> Date: Tue, 12 Jan 2016 20:13:28 -0500
> Subject: [PATCH 02/15] Server: Enable Watchdog in all daemons
>
> This allows the services to self monitor.
>
> Related:
>
https://fedorahosted.org/sssd/ticket/2921
Is it intentional that we also enable the watchdog in monitor?
I wasn?t intentional in the sense that i really wanted at all cost to
have this behavior, but I see nothing wrong on sssd watchdogging itself.
I haven't
seen the sssd process being stuck and if it does, we probably have
bigger issues, so it's probably fine, I just need to remember to not
SIGSTOP sssd when testing anymore :)
New debugging trick, as soon as you attach or execute a process run:
p teardown_watchdog()
I have a gdb cmd file where I execute this as a matter of fact when
attached to a sssd process.
Otherwise ack.
Thanks.
> From c576e37c188ded932996c9714b18a71251ef1d63 Mon Sep 17
00:00:00 2001
> From: Simo Sorce <simo(a)redhat.com>
> Date: Wed, 13 Jan 2016 11:51:09 -0500
> Subject: [PATCH 03/15] Monitor: Remove ping infrastructure
Can we also remove the force_timeout option and the functions that use
it? Looking at monitor.c, they are only called from a single failure
situation which is ENOMEM, so I think it's actually a dead code, the
probability we ever call it is very low.
If you agree with removing the force_timeout but you're busy, let me know
and I can prepare a patch atop yours.
Please add patch on top.
The only code that proved to be useful in the field of all this we
are
about to remove is the diag_cmd(). We could use it to run pstack on the
'stuck' child to learn where it was actually stuck. But I'm not sure
it's possible to implement it since the watchdog handler is a "plain"
signal handler and the diag_cmd() forks and execs...
signal(7) sasys fork()/execve() are async-signal-safe functions, so it
could be run from the handler directly, the other option is to
kill(SIGXXX) the monitor, and then wait a few seconds before aborting,
and hope the monitor has time to run pstack on the child for us.
Either way I'd consider writing a new patch for this later, and would
consider internally dumping a stack trace to a debug file instead of
executing an external process.
That said, I still think it's worth removing the pings in favor
of the
watchdog, the pings proved to be too fragile..
Not only fragile but also caused churn and wakeups in the system, as
every watchdog beat forced at least a context switch and 2 processes to
run.
Maybe it would be better to file a ticket to check again if systemd
watchdog can be used in the future?
We can look at it, I didn't want to get too far down the systemd road
with this patchset as the goal was another.
To be continued..
Ty,
Simo.
--
Simo Sorce * Red Hat, Inc * New York