Improve KeysInUse logging performance during burst of first use events - #174
Improve KeysInUse logging performance during burst of first use events#174Maxwell Moyer-McKee (mamckee) wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Improve KeysInUse logging thread throughput during bursts of first-use events by narrowing how long logging_thread_mutex is held, reducing caller delays while still avoiding holding key locks across fork().
Changes:
- Add
O_CLOEXECwhen opening the log file descriptor. - Reduce
logging_thread_mutexhold time to only the critical section that touchespKeysinuseInfostate. - Move
p_scossl_keysinuse_log_noticecalls outside the mutex to remove logging I/O from the contended section.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pKeysinuseInfo->firstLogTime = pKeysinuseInfo->lastLogTime == 0 ? now : pKeysinuseInfo->firstLogTime; | ||
| pKeysinuseInfo->lastLogTime = now; | ||
| pKeysinuseInfo->logPending = FALSE; | ||
| p_scossl_keysinuse_log_error("Logging thread failed to accquire mutex,SYS_%d", pthreadErr); |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: e08c0717-91d1-47dc-abd8-e5a8d0ecf076
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
SymCryptProvider/src/p_scossl_keysinuse.c:271
- In
childscope this returns whilekeysinuse_enabledis false. The RSA/ECDSA sign and decrypt init paths only createkeysinuseInfowhenp_scossl_keysinuse_running()is true, so a context initialized beforefork()keeps a null info pointer; using that inherited context in the child is then silently omitted even after its logging thread starts. Tracking must be initialized in the parent independently of whether the parent logging thread runs.
if ((keysinuse_process_scope & KEYSINUSE_PROCESS_SCOPE_MAIN) == 0)
{
status = SCOSSL_SUCCESS;
goto cleanup;
SymCryptProvider/src/p_scossl_keysinuse.c:114
- Correct the spelling and wording in this new helper comment.
// Starts the logging thread and marks keysinuse as enabled. Reainitializes
// lock thread globals and state. On failure caller is repsonsible for cleanup.
SymCryptProvider/src/p_scossl_keysinuse.c:1054
- The phrase “only around during” is ungrammatical; clarify that the mutex is held only during the update.
// Log all pending usage events. logging_thread_mutex is held only around
// during the pKeysinuseInfo update to ensure the logging thread is not
// holding a lock during a fork.
SymCryptProvider/src/p_scossl_base.c:508
- This comment has a typo: “alreday” should be “already.”
// Environment overrides config. Config value is alreday fetched core_get_params above
p_scossl_keysinuse_info_free was called after unlocking logging_thread_mutex. Since the reference release can touch the info's lock (CRYPTO_atomic_add's fallback path), the atfork prepare handler could acquire logging_thread_mutex and fork while that lock was held, letting a child inherit a held lock. Move the reference release inside the mutex so the logging thread never holds an info lock across a fork. The slow log write stays outside the mutex. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b2881f25-ddcb-442e-bb92-49d814cb8300
This PR makes a few small performance improvements to keysinuse.