Project

General

Profile

Actions

Feature #12477

closed

ctxops should use stack ordering for save/restore

Added by Patrick Mooney about 2 years ago. Updated about 2 years ago.

Status:
Closed
Priority:
Normal
Category:
kernel
Start date:
Due date:
% Done:

100%

Estimated time:
Difficulty:
Medium
Tags:
Gerrit CR:

Description

Upstreaming OS-7034 for bhyve:

While investigating an issue related to FPU state management for OS-7012, I discovered that the savectx/restorectx behavior of ctxop is somewhat counter-intuitive. As some background information, bhyve uses ctxop functions to ensure that guest FPU state is maintained on the CPU when the thread is running (and is properly stashed when a context switch occurs). When savectx runs for a bhyve thread, the ctxop ordering is as expected: vmm_savectx runs first (being added most recently in the life of the thread), followed eventually by the already-present fpsave_ctxt. The general expectation would be for the reverse order to be used for restorectx, but that is not the case. The ctxop handlers always run from most recent to least recent. In the past (prior to bhyve and eagerFPU, especially) there weren't ordering constraints between ctxop handlers, since they were largely independent of one another. Bhyve makes the case that now, they should be associated with the thread in such a way that allows stack-like traversal for savectx/restorectx. That is: most->least recent for save, least->most recent for restore.

The ctxop chain for the bhyve thread in question when it ran afoul of those ordering conditions while testing a OS-7012 wad:

> fffffe5af5cc8040::print kthread_t t_ctx | ::list "struct ctxop" next | ::print "struct ctxop" 
{
    save_op = vmm_savectx
    restore_op = vmm_restorectx
    fork_op = 0
    lwp_create_op = 0
    exit_op = 0
    free_op = 0
    arg = 0xfffffe008044e700
    next = 0xfffffe5afd90e140
    save_ts = 0x137e89ac2e8c
    restore_ts = 0x137e89aca474
}
{
    save_op = schedctl_save
    restore_op = schedctl_restore
    fork_op = schedctl_fork
    lwp_create_op = 0
    exit_op = 0
    free_op = 0
    arg = 0xfffffe007bd71160
    next = 0xfffffe5b2f901a60
    save_ts = 0x137e89ac4ae9
    restore_ts = 0x137e89acc0ac
}
{
    save_op = xsaveopt_ctxt
    restore_op = xrestore_ctxt
    fork_op = fp_new_lwp
    lwp_create_op = fp_new_lwp
    exit_op = 0
    free_op = fp_free
    arg = 0xfffffe5af5c6b840
    next = 0xfffffe5b2f7dd050
    save_ts = 0x137e89ac4d93
    restore_ts = 0x137e89acc2dc
}
{
    save_op = sep_save
    restore_op = sep_restore
    fork_op = 0
    lwp_create_op = 0
    exit_op = 0
    free_op = 0
    arg = 0xfffffe008044ef10
    next = 0xfffffe5b2f8b2aa8
    save_ts = 0x137e89ac51fc
    restore_ts = 0x137e89acc5d0
}
{
    save_op = lwp_segregs_save
    restore_op = lwp_segregs_restore
    fork_op = 0
    lwp_create_op = 0
    exit_op = 0
    free_op = 0
    arg = 0xfffffe5af5c6b840
    next = 0
    save_ts = 0x137e89ac54a5
    restore_ts = 0x137e89acc82f
}

The OS-7012 wad uncovered this because it increased the frequency of context switches inside the VMRUN critical section, where the guest FPU data is "active" (loaded into the physical FPU).

To confirm the reverse order during testing, I traced (via fbt, for those available) ctxops for threads as they went off-cpu/on-cpu. The expect reverse order was present when performing the restorectx tasks. Additionally, I traced the other generic handlers for other ctx operations (exit, fork, etc) to make sure those were still being called as expected.

For the updated mdb modules, I made sure that it correctly listed handlers on both a patched system and an older crash dump (featuring the "old" NULL-terminated format).


Related issues

Related to illumos gate - Feature #12478: installctx needs kpreempt_disable protectionClosedPatrick Mooney

Actions
Actions #1

Updated by Patrick Mooney about 2 years ago

To test this (without the bhyve conditions which lead to it) I booted a PI which featured the change. First I checked that the mdb ctxop walker was still functional. Using the output from that to identify "interesting" save/restore ctxops, I dtraced those handlers for a given thread (in this case bash) and issues work to wake it up and put it back to sleep. The output from dtrace showed the stack-ordered save/restore operations.

Actions #2

Updated by Patrick Mooney about 2 years ago

  • Related to Feature #12478: installctx needs kpreempt_disable protection added
Actions #3

Updated by Electric Monk about 2 years ago

  • Status changed from In Progress to Closed
  • % Done changed from 0 to 100

git commit 6e2e67256d436ef900becfa771aee283e7e55430

commit  6e2e67256d436ef900becfa771aee283e7e55430
Author: Patrick Mooney <pmooney@pfmooney.com>
Date:   2020-04-16T19:20:32.000Z

    12477 ctxops should use stack ordering for save/restore
    12478 installctx needs kpreempt_disable protection
    Reviewed by: John Levon <john.levon@joyent.com>
    Reviewed by: Robert Mustacchi <rm@joyent.com>
    Reviewed by: Jerry Jelinek <jerry.jelinek@joyent.com>
    Reviewed by: Andy Fiddaman <andy@omniosce.org>
    Approved by: Dan McDonald <danmcd@joyent.com>

Actions

Also available in: Atom PDF