Skip to content

Improve KeysInUse logging performance during burst of first use events - #174

Draft
Maxwell Moyer-McKee (mamckee) wants to merge 6 commits into
scossl-1.10from
mamckee-keysinuse-perf
Draft

Improve KeysInUse logging performance during burst of first use events#174
Maxwell Moyer-McKee (mamckee) wants to merge 6 commits into
scossl-1.10from
mamckee-keysinuse-perf

Conversation

@mamckee

@mamckee Maxwell Moyer-McKee (mamckee) commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

This PR makes a few small performance improvements to keysinuse.

  • Minimizes the time the logging thread mutex is held during logging to continue ensuring fork safety, but avoid blocking crypto operations if a large number of sign/decrypt operations are fired at once.
  • Set the logging thread signal mask to empty to prevent the logging thread from running any signal handlers.
  • Added config options to allow applications to configure keysinuse forking behavior (run in parent and child by default, or exclusively run in the parent or child process)

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_CLOEXEC when opening the log file descriptor.
  • Reduce logging_thread_mutex hold time to only the critical section that touches pKeysinuseInfo state.
  • Move p_scossl_keysinuse_log_notice calls 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.

Comment thread SymCryptProvider/src/p_scossl_keysinuse.c
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

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 child scope this returns while keysinuse_enabled is false. The RSA/ECDSA sign and decrypt init paths only create keysinuseInfo when p_scossl_keysinuse_running() is true, so a context initialized before fork() 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

Comment thread SymCryptProvider/src/p_scossl_keysinuse.c Outdated
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants