Re: pg_jobc going negative?

[christos%astron.com@localhost (Christos Zoulas)]

In article <27763.1594388500%jinx.noi.kre.to@localhost>,
Robert Elz  <kre%munnari.OZ.AU@localhost> wrote:
>    Date:        Tue, 9 Jun 2020 08:23:19 -0000 (UTC)
>    From:        mlelstv%serpens.de@localhost (Michael van Elst)
>    Message-ID:  <rbngtn$48d$1%serpens.de@localhost>
>
>I have spent a little time looking at this now, and I think
>it is just all a mess.
>
>  | pg_jobc is not a reference counter.
>
>Maybe not technically a "reference" counter, as what it is counting isn't
>strictly references, but anything that has x++ and if (--x == 0) do_stuff()
>is essentially a reference counter.   What it is counting references to
>isn't clear (particularly here), but that is what it is doing, or trying
>to do (it has all the same issues as things which really are ref counters).
>
>  | The assertion probably stopped
>  | a bug in a different place by coincidence.
>
>I doubt that, this code is not at all good.   There is no question but
>that the counter does not count properly.
>
>As best I can work out, and someone correct me if I'm wrong,
>the whole purpose of pg_jobc is so that orphanpg() can be called
>when a process group is orphaned (no longer has a session leader).
>
>If it has any other use, I cannot see it.
>
>What's there now simply doesn't work for this purpose.   It was
>suggested that the FreeBSD code has been modified from what we
>have, and that simply adopting that might work.   I went to look
>at their code, but before I did that, I saw that a month ago
>(that is, just around the time of the original discussion here)
>they copied maxv's KASSERTs into their code.  A week ago they removed
>them again, as they were occasionally firing.   That tells me,
>even without looking at their code, that they (still) have similar
>bugs to what we do, and thus that just importing their code won't
>help us.
>
>I see 3 ways forward...   simply drop the KASSERT the way that FreeBSD
>have done, and let things return to the semi-broken but rarely bothersome
>code that was there before.   That's not really a good idea, as the
>sanitizers apparently find problems with the code the way it was (not
>too surprising, deleting the KASSERT won't fix the bugs, it just stops
>noticing them explicitly).
>
>Or, we could properly define what pg_jobc is counting, and then make sure
>that it counts whatever that is properly - is incremented in all the
>appropriate places, and decremented properly as well.   Currently
>the comment about it in proc.h is:
>        /*
>         * Number of processes qualifying
>	 * pgrp for job control 
>         */
>which makes it clear that it is a reference counter (not necessarily
>counting the number of something which exists, so that something can be 
>deleted, but it is counting references to processes).   Unfortunately
>I have no idea what "qualifying pgrp for job control" is supposed to mean.
>
>That could be done, but it seems like a lot of work to me, and not easy.
>
>Another (more radical) approach would be to simply drop orphanpg()
>completely, and thus no longer need pg_jobc at all.   The system
>wouldn't be bothered by this at all - all orphanpg() does is locate
>stopped members of the process group, and send then SIGCONT (to restart)
>and SIGHUP (to probably kill them - or at least inform them they their
>environment has changed).   If the system wasn't doing this, users manually
>(or a script run from cron or something) could do it instead,  If not done
>at all, badly behaving session leaders (processes which don't clean up
>their stopped jobs before exiting - including ones with bugs that causes
>them to abort) would over time cause a growing number of stopped jobs to
>simply clutter the system, consuming various resources, but otherwise
>harmless (there is nothing special about the processes, they can be killed,
>or continued - it is just that the process which would normally do that
>is no longer around).
>
>Third, and the option I'd suggest, is to revert to more basic principles,
>remove the pg_jobc attempt to detect when a session leader has exited,
>or changed to a different process group, and instead at candidate events
>(any time a process leaves a process group, for any reason) check if that
>process was the session leader, and if it is, clean up any now orphaned
>stopped processes.   This is likely to be slower than the current attempt,
>but is much easier to make correct (and much less likely to cause problems,
>other than dangling orphaned stopped processes, if incorrect).
>
>As best I can tell, all the data needed exists already, all that will be
>needed is to modify the code.   We can even leave pg_jobc in the pgrp
>struct, to avoid needing a kernel version bump (and for reasons I cannot
>imagine, pg_jobc is copied into kinfo and einfo structs for sysctl and /proc
>access to the process data, so leaving it around avoids needing to version
>those interfaces as well ... the value would be a meaningless 0, always, but
>I really find it hard to believe that anything would ever care, or even 
>notice).
>
>Opinions on any of this before I start banging on the code?

I think that the suggestion to perform the cleanup when the session leader
exits, or changes process group is the simplest one.

christos