Since this is essentially a walk of the CAR of a list, which should be done in a few pointer walks, is there a giant lock I can just use to lock the pid structs and get 'er done? I think anything more is is overkill.
All that we should do is send a SIGINT and get out. WIth luck this all goes away in a little while anyway. But I'm going to redo per your suggestion. I was just not sure of the rules on locking the pid structs, and I knew I'd get the straight info from you quickly :-) ron On Mon, Mar 28, 2016 at 8:46 AM Barret Rhoden <[email protected]> wrote: > Hi - > > On 2016-03-25 at 16:14 ron minnich <[email protected]> wrote: > > The following changes since commit > > 24c7dd20c8a5a6983083c97d7492a994c67355d0: > > > > Upgrade parlib fp state handling, use proc_global_info (XCC) > > (2016-03-22 16:54:53 -0400) > > > > are available in the git repository at: > > > > [email protected]:rminnich/akaros killkids > > > From c361c5f817d55b54be8a5b4f16c0ac9c42da4110 Mon Sep 17 00:00:00 2001 > > From: "Ronald G. Minnich" <[email protected]> > > Date: Fri, 25 Mar 2016 08:59:42 -0700 > > Subject: Add a control file in #cons to support killing children. > > > > This is needed for ssh support for ^C. > > The ssh server, when it sees a ^C, opens and writes a command > > to #cons/killkids. > > > > Right now the command is ignored, but that might change. > > Is this a reference to CMkillkid? How does that actually work? As far > as I can tell from the code, the CMkillkid case only gets called when > there is a write to #cons/reboot (Qreboot). We might not need any of > the CMkillkid stuff. > > > diff --git a/kern/drivers/dev/cons.c b/kern/drivers/dev/cons.c > > enum { > > @@ -619,6 +625,8 @@ static struct dirtab consdir[] = { > > {"config", {Qconfig}, 0, 0444}, > > {"cons", {Qcons}, 0, 0660}, > > {"consctl", {Qconsctl}, 0, 0220}, > > + // FIXME -- we don't have real permissions yet so we set it to > 222, not 220 > > + {"killkid", {Qkillkid}, 0, 0220 | /* BOGUS */ 2}, > > I guess you needed the extra 2 for it to work at runtime? > > > > + > > +/* > > + * quick depth first search for somone to kill. This is a reasonable > > + * standin for "kill most distant child" that is also reasonably fast. > > + * No locking, totaly unsafe. yep, this is ugly. Once we have cmux we > > + * might not need it. But consider that most alternatives involve > > + * walking proc in user mode ... yuck! > > + */ > > +void killkid(void) > > +{ > > + struct proc *victim = NULL, *kid; > > + > > + kid = current; > > + while (kid) { > > + victim = kid; > > + kid = TAILQ_FIRST(&victim->children); > > + } > > + if (victim) { > > + printk("killkid: killing %d\n", victim->pid); > > + send_posix_signal(victim, SIGINT); > > + } > > +} > > Yeah, this is horribly racy. If a child exits or anything during this, > we'll blow up the kernel. The children list is protected by > > cv_lock(&parent->child_wait); > > (that's a spinlock internally btw). It protects the children list, > which is a source of references for all of a process's children. > > I think it'd be safe to grab the locks as you went down the list, > recursively. We'd be establishing a lock ordering from parent->child, > but that's probably okay. Especially if we get rid of this code > eventually. Another approach would be to lock briefly and up the refcnt > at each step through the parent-child chain. This might be better. > Here's why: > > It's probably safe to hold the locks while signalling. Though if we > ever change things, like call proc_destroy() (a more serious 'kill') > while holding the locks, then we'll have deadlock issues. If we use > refcnts to "hold our place" as we walk down the line, we'll be safe from > deadlock. > > So maybe: > > find_first_kid: // helper, 0 on failure, incref'd child on success > grab my lock > kid = TAILQ_FIRST(&me->children); > if (kid) > proc_incref(kid, 1); > unlock my lock > return kid > > kill_kid: // do you want to kill the caller? or only kids? > victim = me; > > /* start the loop with an incref'd victim */ > proc_incref(victim, 1); > > loop: > child = find_first_kid(victim) > if (!child) > break; > /* no longer need old victim, we have the ref on child */ > proc_decref(victim) > victim = child; > > /* now we have a victim. might be 'me' still. we have a ref on it > and we need to decref when done */ > > if (!(victim == me)) // optional 'me' check > send_sig_int > > proc_decref(victim) /* decref no matter what */ > > > I think that will work and be relatively safe. =) > > Barret > > -- > You received this message because you are subscribed to the Google Groups > "Akaros" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > To post to this group, send email to [email protected]. > For more options, visit https://groups.google.com/d/optout. > -- You received this message because you are subscribed to the Google Groups "Akaros" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. To post to this group, send email to [email protected]. For more options, visit https://groups.google.com/d/optout.
