Bug #647

rough-draft VKERNEL host-initiated shutdown patch

Added by c.turner over 7 years ago. Updated over 7 years ago.

Status:ClosedStart date:
Priority:LowDue date:
Assignee:-% Done:

0%

Category:-
Target version:-

Description

Disclaimer: MESSY!

Took a quick attempt at getting the VKERNEL to shutdown cleanly against
1.8,
and it seems to work just fine as suggested with mailbox signals, etc.

Attached is my 'cumulative' 1.8 vkernel patch against 1.8_Release
/usr/src/sys/platform/vkernel including the previous multi-vk changes.

Of course I will clean up and resubmit against Preview/Head, (along with
a patch for vkernel(7), but I wanted to have the general approach
validated before going too much further.

(plus I don't have a -HEAD / Preview machine set up at the moment,
but am working on it)

status:
- OK for simple shutdown case with init already fork()'ed.
- VK is too fast for me to catch the 'no init yet' shutdown case..
- need to merge against Preview, as things have changed there,
and re-submit. patch here includes previous multivkd work.

changes in:
- platform/vkernel/include/globaldata.h : maibox slot
- platform/vkernel/include/md_var.h : vk_shutdown prototype
- platform/vkernel/platform/kqueue.c : mailbox hook
- platform/vkernel/platform/init.c : vk_shutdown function

questions/notes:
- I didn't think I really needed a separate sigaction in
kqueue_setup..
- should shutdown mailbox handler dispatch happen higher up
(e.g. in platform/vkernel/i386/trap.c)
- should this actually follow
'vke_intr(void *xsc, struct intrframe *frame __unused)' conventions?
- we're not really checking for FD related IO kevents() ..
- this implies a more 'full device' kind of thing , with a struct,
etc.
- should the other init(8) signals for e.g. single user, etc. be
implemented? Currently, these are not implement as shutdown is
~90% of cases, and wanted to verify I'm taking the proper direction.
- mailbox signals aren't really documented in the manual at the
moment..

Comments / Questions / feedback welcome.

Thanks!

- Chris

vkhalt.patch Magnifier (6.84 KB) c.turner, 05/20/2007 09:10 PM

vkhalt-HEAD.patch Magnifier (4.99 KB) c.turner, 06/10/2007 11:46 PM

vkhalt-HEAD.patch Magnifier (3.4 KB) c.turner, 06/16/2007 11:25 PM

vkhalt-HEAD.patch Magnifier (4.55 KB) c.turner, 06/17/2007 04:26 PM

History

#1 Updated by dillon over 7 years ago

:Disclaimer: MESSY!
:
:Took a quick attempt at getting the VKERNEL to shutdown cleanly against
:1.8,
:and it seems to work just fine as suggested with mailbox signals, etc.
:
:Attached is my 'cumulative' 1.8 vkernel patch against 1.8_Release
:/usr/src/sys/platform/vkernel including the previous multi-vk changes.
:...
:status:
: - OK for simple shutdown case with init already fork()'ed.

Looks good.

: - VK is too fast for me to catch the 'no init yet' shutdown case..

Heh.

: - need to merge against Preview, as things have changed there,
: and re-submit. patch here includes previous multivkd work.

Yup, I was just going to say that... multi-vk is already in HEAD.

:changes in:
: - platform/vkernel/include/globaldata.h : maibox slot
: - platform/vkernel/include/md_var.h : vk_shutdown prototype
: - platform/vkernel/platform/kqueue.c : mailbox hook
: - platform/vkernel/platform/init.c : vk_shutdown function
:
:questions/notes:
: - I didn't think I really needed a separate sigaction in
: kqueue_setup..
: - should shutdown mailbox handler dispatch happen higher up
: (e.g. in platform/vkernel/i386/trap.c)
: - should this actually follow
: 'vke_intr(void *xsc, struct intrframe *frame __unused)' conventions?
: - we're not really checking for FD related IO kevents() ..
: - this implies a more 'full device' kind of thing , with a struct,
: etc.

The sigaction initialization for SIGTERM doesn't really belong
in init_kqueue(). Put it in platform/vkernel/i386/exception.c
instead.

Now the question is: should it be a signal mailbox or a real signal?

That's a good question. Well, we have an issue with the small size
of the kernel thread stack, so I think it should be a mailbox.

Now... when to check for and call the function! There are two places
where it needs to be checked:

(1) in go_user().
(2) in cpu_idle().

Check for the mailbox and call a function specific to the shutdown
(which you can also put in platform/vkernel/i386/exception.c). Don't
integrate it into signalmailbox().

: - should the other init(8) signals for e.g. single user, etc. be
: implemented? Currently, these are not implement as shutdown is
: ~90% of cases, and wanted to verify I'm taking the proper direction.

single-user mode on the master host will send a HUP or a TERM
(I forget which) to all processes, including the vkernel. They
should probably be treated as a shutdown by the vkernel.

: - mailbox signals aren't really documented in the manual at the
: moment..

I'll do a quick fix to the sigaction manual page. hmm. I thought
I had documented it.

:Comments / Questions / feedback welcome.
:
:Thanks!
:
: - Chris

#2 Updated by c.turner over 7 years ago

realise I hadn't responsed .. thanks for feeback. will try to cleanup
soonish & resubmit.

how's HEAD stabilizing, or is Preview a better bet for patch testing?

Thanks,

- Chris

#3 Updated by dillon over 7 years ago

:realise I hadn't responsed .. thanks for feeback. will try to cleanup
:soonish & resubmit.
:
:how's HEAD stabilizing, or is Preview a better bet for patch testing?
:
:Thanks,
:
:- Chris

Preview. HEADs gonna be a moving target for a while.

-Matt
Matthew Dillon
<>

#4 Updated by luxh over 7 years ago

Patch not committed. Feature added through a different approach though, see
http://leaf.dragonflybsd.org/mailarchive/commits/2007-05/msg00228.html .

#5 Updated by c.turner over 7 years ago

This patch should't be committed - it's messy and not the way Matt
suggested I clean it up, but the commit referenced is absolutely not
the same thing so this shouldn't be marked resolved - unless people
don't want the feature anymore, in which case I'll continue to maintain
the patch for as long as I'm interested in it.

The functionality implemented for this issue (#647), the *host* system
is able to signal the *guest* VKERNEL to shut down cleanly (e.g. when
the host does a halt and forgets to shut down the guest VK's),

With the committed code above, the VK is able to shut -itself-
down cleanly, but the host continues to have no way of doing so -
Signals from outside will kill the VK and leave the VKD's in an
inconsistant state, possibly resulting in a loss of data within the VK
system

I responded to Matt's commit because I had a second patch in mind that
did the same thing (vkernel *self*-initiated shutdown/reboot) - was
probably going to tackle that later... as I see it, the two
compliment each other..

In any case,
hoping to clean up / resubmit this weekend, more than likely with a
second signal handler to take advantage of Matt's new reboot code.

Thanks,

- Chris

#6 Updated by justin over 7 years ago

On Wed, May 30, 2007 8:47 pm, Chris Turner wrote:

I think it would be useful - it should be possible to start AND stop a
vkernel without having to actually be in the virtual environment. I'd
probably be more antsy about it if I had a spare 1.9 machine to experiment
on right now.

#7 Updated by luxh over 7 years ago

I guess I was too eager to close issues on the tracker, sorry.

Please do! Keep up the good work.

Cheers,

-Max

#8 Updated by dillon over 7 years ago

:
:In any case,
:hoping to clean up / resubmit this weekend, more than likely with a
:second signal handler to take advantage of Matt's new reboot code.
:
:Thanks,
:
:- Chris

Definitely! Having a properly working shutdown signal would be good.
In fact, it might be easiest to just set it up as a mailbox signal and
monitor it in the clock interrupt, then create a kernel thread to initiate
the actual shutdown when the signal is detected. Or something like that.

--

I noticed another thing, which is when you run the vkernel from
a shell script, and you interrupt it with ^\ + ^C (that sequence
will enter ddb, then kill the vkernel), it currently causes the vkernel
to do its shutdown/reboot thing, but also kills the shell script and
disconnects the terminal so you wind up getting a shell prompt with the
vkernel still running in the background. Its very odd and annoying
behavior.

I think there is some sort of termios / tty session magic we have
to do to make the tty interrupt signals only go to the vkernel,
but I'm not sure what it is.

-Matt
Matthew Dillon
<>

#9 Updated by c.turner over 7 years ago

OK.. Updated / cleaned this against -HEAD (as of this morning) and
things appear to be working.

(vk still too fast to catch the 'no init yet' case)

As suggested:

- moved shutdown hook and sigaction setup to
sys/platform/vkernel/i386/exception.c.

- changed mailbox check to :

- /sys/platform/vkernel/i386/trap.c,v : go_user()
- /sys/platform/vkernel/i386/cpu_regs.c,v : cpu_idle()

- kept mailbox flag in platform/vkernel/include/md_var.h

But:

- I'm still not 100% on the how the SIGIO mailbox interlock
works, so the mailbox check hooks might be in the wrong /
non-optimal places within go_user() / cpu_idle().
- There isn't really a clean way of getting at the shutdown function
from oustide 'sys/kern/kern_shutdown.c'. This results in compiler
warnings, but the linker sorts things out.
- Updated vkernel manual page also attached with patch - hopefully
initial meaning was kept.
- Didn't implement any other signals yet as am still getting approach
sorted out.

p.s:

- now that there are two mailbox signals, perhaps signalmailbox()
should and the related mdvar should be renamed e.g. iomailbox() ?
didn't do this as not really directly related to the patch..
- .. and mailbox signals still a bit fuzzy in the manual :)

Thanks

- Chris

#10 Updated by dillon over 7 years ago

:OK.. Updated / cleaned this against -HEAD (as of this morning) and
:things appear to be working.
:
:(vk still too fast to catch the 'no init yet' case)

This looks quite nice. I just realized though that it isn't safe
to run reboot() (or probably ksignal either) from the idle halt
code, so scratch that part.

We have to find a good place to put this. I think what we may have
to do is create a kernel thread whos only job is to shut the kernel
down, and then check the mailbox from inside signalmailbox() and have
it wake the special kernel thread up.

Would you like to have a go at that or would you like me to do it?

-Matt

#11 Updated by c.turner over 7 years ago

thanks. ok.

If the special thread doesn't need to do any other cleanup that I'm
likely unaware of, I can give it another shot this weekend.

Not yet knowing the overall code paths unfortunately means I'm
kind of coding this blind .. but I suppose the point is to learn as I go.

I assume the reason for the thread is that the way things are currently
called, the VK might not be in a consistent internal state when the
signal is delivered to init and/or init is rescheduled to run?

to clarify:

- keep signal handler and shutdown function in the exception setup
- create kthread somewhere ( main() ? platform init? )
- remove checks from both idle loop / task switching areas
- add check / wakeup logic to signalmailbox()

Thanks,

- Chris

#12 Updated by dillon over 7 years ago

:If the special thread doesn't need to do any other cleanup that I'm
:likely unaware of, I can give it another shot this weekend.
:
:Not yet knowing the overall code paths unfortunately means I'm
:kind of coding this blind .. but I suppose the point is to learn as I go.
:
:I assume the reason for the thread is that the way things are currently
:called, the VK might not be in a consistent internal state when the
:signal is delivered to init and/or init is rescheduled to run?

Right. The idle loop isn't allowed to block, and the shutdown
sequence involves a lot of blocking. The go-user code is in a
sane state and it would be ok to block there, but we wouldn't
get consistent checks.

:to clarify:
:
: - keep signal handler and shutdown function in the exception setup
: - create kthread somewhere ( main() ? platform init? )
: - remove checks from both idle loop / task switching areas
: - add check / wakeup logic to signalmailbox()
:
:Thanks,
:
:- Chris

I think the platform init code is too early. The memory subsystem
isn't really initialized until it gets into the middle mi_startup().

What I recommend is that you create a SYSINIT in
platform/vkernel/i38/exception.c with a priority of SI_BOOT2_MACHDEP
to create the thread. This looks kinda messy, but it should work.

static void
sigshutdown_daemon(void)
{
while (mdcpu->gd_shutdown == 0)
tsleep(&mdcpu->gd_shutdown, 0, "sswait", 0);
}
mdcpu->gd_shutdown = 0;
kprintf("Caught SIGTERM from host system. Shutting down...\n");
if (initproc != NULL) {
ksignal(initproc, SIGUSR2);
} else {
reboot(RB_POWEROFF);
}
}

static struct thread *sigshutdown_thread;

static struct kproc_desc sigshut_kp = {
"sigshutdown", sigshutdown_daemon, &sigshutdown_thread
};

SYSINIT(sigshutdown, SI_BOOT2_MACHDEP, SI_ORDER_ANY,
kproc_start, &sigshut_kp);

Then put a check in signalmbox which wakes up the thread:

/*
* we only need to wake up our shutdown thread once. Keep it non-zero
* so the shutdown thread can detect it.
*/
if (mdcpu->gd_shutdown > 0) {
mdcpu->gd_shutdown = -1;
wakeup(&mdcpu->gd_shutdown);
}

-Matt
Matthew Dillon
<>

#13 Updated by c.turner over 7 years ago

Okay. All set (+= roff updates per Sascha).

You'd mostly done all the work.. but now I understand the sysinit
things a bit more so that's good for something - and thanks! Would have
taken me all day to track things down, although my ctags-foo is improving :)

One thing that I did change (aside from minor <75 column reformatting, etc)
was that in the SYSINIT with the SI_BOOT2_MACHDEP, I'd get a panic before
init.. (see below)

I started to trace through this this but thought 'why not just try setting
this up later on in the sequence' which is why this is now set to
SI_BOOT2_PROC0, which is the lowest-numbered item in the sysinit_sub_id
enum that worked for me (TM).

Also, I'm wondering why you suggested resetting the mailbox flag to -1
when it is already nonzero.. kept this as is, but just for curiosity..

I'll knock out the pidfile thing in a few hours time.

Thanks,

- Chris

below:

real memory = 33554432 (32768K bytes)
avail memory = 30138368 (29432K bytes)
initclocks
panic: zone: invalid zone
Trace beginning at frame 0xbfbff858
(null)(8211300,828cf40,822e45f,bfbff888,0) at 0x80b5c20
(null)() at 0x80b5c20
Debugger("panic")
Stopped at 0x81f9615: movb $0,0x82bb898
db> trace
(null)(8211189,828cf40,822e45f,bfbff888,0) at 0x81f9615
(null)() at 0x80b5c35
db>

#14 Updated by c.turner over 7 years ago

Chris Turner wrote:

updated -u format, per Sacha. Good eyes! (or bad ones on my part :)

Thanks,

- Chris

#15 Updated by dillon over 7 years ago

:
:updated -u format, per Sacha. Good eyes! (or bad ones on my part :)
:
:Thanks,
:
:- Chris

Looks good. I did a quick test and it worked great.

I am committing it now!

-Matt

Also available in: Atom PDF