Page MenuHomeFreeBSD

Move syscall_thread_{enter,exit}() to the slow path
ClosedPublic

Authored by trasz on Oct 28 2020, 2:46 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mar 8 2024, 9:31 PM
Unknown Object (File)
Feb 16 2024, 9:50 AM
Unknown Object (File)
Jan 17 2024, 5:50 PM
Unknown Object (File)
Dec 20 2023, 4:27 AM
Unknown Object (File)
Oct 3 2023, 11:27 PM
Unknown Object (File)
Sep 12 2023, 8:42 PM
Unknown Object (File)
Sep 3 2023, 6:35 AM
Unknown Object (File)
Sep 3 2023, 6:33 AM
Subscribers

Details

Summary

Move syscall_thread_{enter,exit}() to the slow path. This is only
needed for syscalls from unloadable modules.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

trasz requested review of this revision.Oct 28 2020, 2:46 PM

Sigh, submit the right version.

enter is needed for syscalls from _loadable_ modules, not unloadable.

That said, what is the point of the change ? It somewhat pessimizes syscalls from modules, while removing one test for SY_THR_STATIC.

In D26988#602096, @kib wrote:

enter is needed for syscalls from _loadable_ modules, not unloadable.

The point of thread_syscall_enter() is to make it possibly to safely unload modules. You can easily do loading without it.

That said, what is the point of the change ? It somewhat pessimizes syscalls from modules, while removing one test for SY_THR_STATIC.

It removes one test from the fast path, while slightly pessimizing the slow path. That's what fast path/slow path idea is about.

If doing such reorg, it might make sense to de-commission syscall_thread_enter/exit, renaming underscored functions back to syscall_thread_.... Then you would fetch SY_THR_STATIC once and cache it in local var.

I've considered that. There are two reasons I hadn't done that: first is that this complicates the code somewhat, and I'm trying to simplify it instead. Second, looking at the RISC-V assembly output we're already using all the caller-saved registers; adding another local variable to be kept across the function calls would likely make the assembly even worse. Third - this is the slow path; avoiding a flag check in the slow path doesn't really buy us anything.

My long-term plan for this function is to keep expanding the fast path/slow path split, hopefully moving ptrace, ktrace, and KTR checks into the slow path.

I do not think that risc-v assembly has any influence there. Checking the flag once makes things more consistent and logical.

Refactor to use a local flag.

sys/kern/kern_syscalls.c
89 ↗(On Diff #79299)

I suggest to move assert out of the loop.

94 ↗(On Diff #79299)

You may change this to atomic_fcmpset + atomic_thread_fence_acq() in followup commit.

105 ↗(On Diff #79299)

There too.

trasz added inline comments.
sys/kern/kern_syscalls.c
94 ↗(On Diff #79299)

I'll try, I'll need to refresh my understanding of those first.

Move KASSERT out of the loop.

This revision is now accepted and ready to land.Nov 7 2020, 11:51 PM