In article <27763.1594388...@jinx.noi.kre.to>, Robert Elz <k...@munnari.oz.au> wrote: > Date: Tue, 9 Jun 2020 08:23:19 -0000 (UTC) > From: mlel...@serpens.de (Michael van Elst) > Message-ID: <rbngtn$48d$1...@serpens.de> > >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
