realtime: wake the interpreter from a dedicated timer thread

[probonopd]

Alternative to #216 with the same goal (drop the idle desktop to ~3% host CPU in realtime mode), but instead of polling the host clock in the interpreter loop it uses a dedicated timer thread that sleeps until the next guest timer deadline and then raises exec_timer, waking the interpreter only to process due timers.

Additional differences from #216:

• The idle throttle now stays disengaged while the user is interacting: the host event poller marks input via mark_host_input and guest_is_idle refuses to sleep right after an input event, so moving the mouse or typing never feels sluggish while throttled.
• The throttle sleep/burst cycle is guarded by g_idle_throttle_active, so the timer thread cannot cut a servicing burst short or race the idle decision (which caused full-speed slices and CPU flapping).
• The confirm/uptime gates are lowered from 15 s to 6 s / 8 s so the throttle engages sooner after the guest settles.
• Realtime globals (exec_timer, g_realtime, g_nanoseconds_base, g_idle_cpu_save) are now atomic since they are touched by both the emulation thread and the timer thread.

Tested end to end on the Power Macintosh G3 machine: normal boot to the desktop at full CPU, settled idle desktop at ~3% host CPU, and full-speed responsiveness while interacting.

[joevt]

Today I learned about std::memory_order_relaxed.

I think the TimerManager changes can be its own commit. I like to split large changes into smaller commits:

• First, commits that don't affect anything (such as cleanup or rename or add a method).
• Second, commits that do the large changes, perhaps using changes from the earlier commits.

So, another commit in the early category that should be added is something like this: ppcexec: Change thread accessed variables to atomic.. This commit doesn't affect the intent of the existing code. It makes the existing code more correct. A benchmark would show the impact of the change. But a negative impact doesn't mean the change shouldn't exist.

The point of the early commits is to make the large commit more focused on the main feature being implemented.

In guest_idle, you have a call to get_virt_time_ns() and cpu_now_ns(). Can cpu_now_ns() be replaced with now_ns? (taking into account that get_virt_time_ns is offset by g_nanoseconds_base)
g_nanoseconds_base is usually never changed during execution. I wonder if there's a way to get rid of it? It exists so that toggling g_realtime can work without affecting the TimerManager queue.

In ppc_exec_until and ppc_exec_dbg, you removed volatile. I think that was added there because of the setjmp. There's a comment in cpu/ppc/CMakeLists.txt:

# The use of volatile is deprecated, but we still need it to avoid function
# parameters being clobbered when using setjmp/longjmp

I don't know for sure that volatile is required. I have not experienced that problem myself.

[probonopd]

Thanks for the detailed review! I've restructured the history and addressed all four points.

1. Split into smaller commits

I rewrote the branch into focused commits:

• core/timermanager: add get_next_timeout_ns() - pure addition, no behavior change (8b94e71)
• ppcexec: change thread-accessed variables to atomic - makes the existing code more correct without changing behavior (1c8b641)
• realtime: wake the interpreter from a dedicated timer thread - the main feature, now only containing the timer thread, the input-wake and the throttle gating (44fc709)

2. cpu_now_ns() vs now_ns

You're right. mark_host_input() now stores get_virt_time_ns() and guest_is_idle() compares against its already-computed now_ns, so the extra clock read is gone.

3. g_nanoseconds_base

It's kept. It is constant during execution (written only at CPU init and on the g_realtime toggle) and exists so that toggling g_realtime does not shift the TimerManager queue, which holds guest-time deadlines. Because it is constant during execution, comparing two guest-time readings as in point 2 is exact.

4. volatile on ppc_exec_until / ppc_exec_dbg

Restored - the removal was unrelated to this PR, and cpu/ppc/CMakeLists.txt documents that volatile is needed to avoid function parameters being clobbered by setjmp/longjmp. Those lines are untouched by the PR now.

[joevt]

Very nice. For changes that affect the cpu execution, some before and after benchmarks would help characterize the changes as benign or not.

There's the bench1 target in dingusppc. It does no I/O so I don't know how the throttling might affect that. But that shouldn't matter since throttling applies mostly to the g_realtime = true mode and this benchmark has g_realtime = false. I suppose what we really want is for the g_realtime = false mode to not decrease in performance drastically since that is the default mode.

I made a benchmark that uses the time of day since that is mostly accurate in Mac OS 9 or earlier on DingusPPC regardless of mode.
https://68kmla.org/bb/threads/lets-see-your-best-disk-speeds-ppc-68k.49268/post-554194

We might want a method to obtain host nanoseconds in the guest environment by creating an unused PPC special purpose register. Or by adding a special register in the emulated mac-io chip. Or by utilizing special guest CPU instructions that are normally illegal ops. That's something to think about for another time.

A third possible benchmark is to measure boot time. Choose a bootable disk image that is easily obtainable such as one from the mihaip/infinite-mac repository (the Releases section has larger disk images that are not in the repository). These may be missing partition tables which infinitemac or my fork adds automatically to make them usable as a harddisk in DingusPPC. I don't know if boot time is a great metric - because the system is usually unusable during that time. The end time used by the boot time measurement should be the timestamp of an event that exists in the DingusPPC log. If no suitable logged event exists, then perhaps a startup app can be made to trigger a logged event.

As for these commits, I would add them in this order (earliest to latest):

• ppcexec: Change thread-accessed variables to atomic
• timermanager: Add get_next_timeout_ns()
• realtime: Add a --realtime command line flag
• realtime: throttle an idle guest in realtime mode to save host CPU
• realtime: wake the interpreter from a dedicated timer thread

Maybe the last two should be combined. Or maybe not.The switch to using a dedicated timer thread for the realtime mode's event process triggering is a big change by itself. Did that dedicated timer thread change necessitate the other changes? Or were the other changes made as corrections/improvements to the previous commit? If the latter then perhaps those changes should be applied to the previous commit.

[probonopd]

These may be missing partition tables which infinitemac or my fork adds automatically to make them usable as a harddisk in DingusPPC.

I came across that topic and was wondering whether https://github.com/dingusdev/dingusppc/ could/should do the same.

[joevt]

I came across that topic and was wondering whether https://github.com/dingusdev/dingusppc/ could/should do the same.

My fork has these commits (from latest to earliest):

• Use MetaImgFile for hard disks.
• metaimagefile: Use direct filesystem operations.
• metaimagefile: Add missing include.
• Add multi-file disk image support.

Looks like I misspelled metaimgfile. I'll do a reword.

infinite-mac also uses Add multi-file disk image support. but it applies the changes only to ATA hard disks - not SCSI hard disks? See Use MetaImgFile to allow ATA-based machines to mount multiple disks

[dingusdev]

The main issue I had with the MetaImgFile stuff was that it uses a header file from Apple directly. As it had no explicit source license, I opted to not include it. An open-source replacement would be accepted.

[joevt]

The main issue I had with the MetaImgFile stuff was that it uses a header file from Apple directly. As it had no explicit source license, I opted to not include it. An open-source replacement would be accepted.

What about versions from Darwin which have the APSL licence? I've made changes to the Add multi-file disk image support. commit.

[dingusdev]

I think those versions would be covered by Apple's terms under "Larger Works", in which case it would be acceptable. We will also need to include a copy of the APSL license.

[probonopd]

Reordered the history to your suggested order (earliest to latest) and folded the corrections into the throttle commit:

• ppcexec: change thread-accessed variables to atomic
• core/timermanager: add get_next_timeout_ns()
• realtime: add a --realtime command line flag
• realtime: throttle an idle guest in realtime mode to save host CPU (now self-contained, with the input-wake, the lowered 6s/8s confirm gates and the IDLE_MAX_SLEEP_NS sleep cap folded in)
• realtime: wake the interpreter from a dedicated timer thread (now just the loop-condition reorder plus the dedicated thread)

To answer your question: the dedicated timer thread change does necessitate two of the others. It is what makes the atomic conversion of exec_timer / g_idle_throttle_active necessary (exec_timer is now written from a third thread), and it is the reason get_next_timeout_ns() exists (the thread needs to peek at the next timer deadline to know how long to sleep). The --realtime flag is independent but small. The remaining changes (host-input wake in guest_is_idle, the lowered confirm/uptime gates, the 16 ms sleep cap) were corrections/improvements to the throttle behavior itself, so I applied them to the throttle commit as you suggested, rather than leaving them in the wake commit.

I also corrected the wake commit message: the previous wording claimed a per-instruction realtime deadline check, which the final diff does not have - it is only the loop-condition reorder plus the dedicated thread. I kept the last two commits separate rather than combining them, since the throttle commit now stands alone and the timer thread is the orthogonal change.

Regarding benchmarks: I have not added before/after numbers for g_realtime = false yet. The throttle/wake paths are gated on g_realtime and g_idle_cpu_save (both false by default), so the default path only sees the atomic conversions plus process_timers() returning the same value it already returned; I did boot-test the final tree in realtime mode with --idle-cpu-save and the throttle engages as before.

[joevt]

I will include these commits in my fork. I'll place them before @mihaip 's ppc: Model configurable CPU frequencies commit ( #210 ) because that one is still a draft. His commit relates mostly to the g_realtime = false mode.

[joevt]

I have added your commits (as they currently exist) into my fork just before @mihaip 's commits). I have these issues/questions (no testing has been done by me yet):

1. ==================

About this change in ppc_exec.cpp:

        // In realtime mode guest time is the wall clock, so there is no
        // per-instruction time to advance; the instruction budget only
        // bounds the idle burst so a fast host cannot over-run it. The
        // burst is never cut short because neither exec_timer writer (the
        // realtime timer thread and force_cycle_counter_reload) raises it
        // while the throttle is active; the throttle fires due timers
        // itself on wakeup. In non-realtime mode the throttle is never
        // active, so this single condition reduces to plain exec_timer
        // handling and avoids an atomic load and branch on every
        // instruction.
        if (exec_timer.load(std::memory_order_relaxed) || g_icycles++ >= max_cycles) [[unlikely]]
            max_cycles = process_events();

Doesn't the short-circuit evaluation of the || operator mean that g_icycles++ >= max_cycles does not always happen? That means the non-realtime time (or virtual time, which is g_icycles) will be incorrect? My fork has commit ppcexec: Add ppc_exec functions for g_realtime mode (in progress). to fix this by having different ppc_exec_inner functions for realtime mode.

The description in the comment is missing some details (I'm probably missing something)

• In realtime mode guest time is the wall clock, so there is no per-instruction time to advance
  • That makes sense. g_icycles is the virtual time and is only valid for non realtime mode.
• the instruction budget only bounds the idle burst so a fast host cannot over-run it
  • Not sure what an "instruction budget" is. What's the quantity being budgeted? Instructions per second?
  • Not sure what "bounding the idle burst" means. What are the bounds? What's an idle burst? Why should an idle be bursty?
• The burst is never cut short because neither exec_timer writer (the realtime timer thread and force_cycle_counter_reload) raises it while the throttle is active; the throttle fires due timers itself on wakeup.
  • raises it means writing to exec_timer?
  • Isn't writing to exec_timer the only way to fire due timers (because ppc_exec_inner calls process_events() only when exec_timer is true?
• In non-realtime mode the throttle is never active, so this single condition reduces to plain exec_timer handling and avoids an atomic load and branch on every instruction.
  • What single condition? The statement has two conditions exec_timer.load and g_icycles++ >= max_cycles.
  • The atomic load is exec_timer.load? How is it avoided?

2. ==================

You accidentally added a blank link here:

int get_icnt_factor()
{
    return icnt_factor;
}

<-----

3. ==================

Why does this line use g_icycles?
return g_icycles + (burst_ns >> icnt_factor) + 1
The line is only for throttling realtime mode. Is g_icycles used for realtime mode?

Maybe some of the last commits in my fork addresses this issue and some others? These are listed from earliest to latest (the relevant ones are marked with a bullet •):

• • ppcexec: Add ppc_exec functions for g_realtime mode (in progress).
• ppcexec: Add constexpr.
• • timermanager: Add has_next to process_timers.
• ppcexec: Use power_off instead of force_cycle_counter_reload.
• ppcexec: Remove 5 us offset from set_virt_time_ns.
• ppcexec: Don't allow g_instruction_period < 1.
• ppcexec: Change name of po_endian_switch.

These are untested.

4. ==================

What is DP3 in this comment?

// 6 ms per 16 ms window keeps DP3 healthy at ~7% host CPU.

Mac OS X Developer Preview 3 ?

5. ==================

Unnecessary spaces. Spaces for alignment should only be used for lists and tables and similar/related statements. burst_ns and sleep_ns are not related enough to be aligned.

            constexpr uint64_t burst_ns     = 6000000ULL;  // 6 ms
            const uint64_t sleep_ns = (slice_ns > IDLE_MAX_SLEEP_NS) ? IDLE_MAX_SLEEP_NS : slice_ns;

6. ==================

These changes should maybe be grouped together?

// Counter of guest accesses to memory-mapped devices, incremented by
// mmu_read_vmem/mmu_write_vmem. Used by ppcexec.cpp to detect when the
// guest is idling (spinning without touching any device).
extern uint64_t g_mmio_access_count;

/* set_g_idle_cpu_save */
extern void set_g_idle_cpu_save(bool enabled);

/* mark_host_input: the host event poller calls this for every input event */
extern void mark_host_input();

I understand they are defined in different source files but they are used for implementing a single feature.

[dingusdev]

4. Yes, that is Developer Preview 3