Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CA-391656: OCaml C stubs: release runtime lock around all xenctrl calls #4916

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

edwintorok
Copy link
Contributor

There was a commit in the past that added enter/leave around all stubs, however since then more stubs got added, and not all of them released the runtime.

This is opened as draft, since it is a riskier change that needs a bit more stress testing too (not just a BST), and potentially also an improved static analyzer.

This PR builds on top of the C stub bugfix PR

Review only these 3 commits here:

854574107858b4443b5144d8e13de8bb8e2b334b (HEAD -> private/edvint/toolstack/improve-enter-leave, edwintorok/private/edvint/toolstack/improve-enter-leave) C stubs: add a .clang-format
6b386b8c6dc87d799bdbc23377042f1f9f649e33 CA-375277: unixpwd_stubs.c: factor out common code and use enter/leave blocking section where possible
a7cf4ff74db9a49a1aed670fb776e5c89ec00e38 CA-375277: xenctrlext_stubs.c: add missing enter/leave blocking section

@edwintorok edwintorok changed the base branch from master to feature/epoll February 15, 2024 10:58
@edwintorok edwintorok changed the base branch from feature/epoll to feature/perf February 15, 2024 14:21
@edwintorok
Copy link
Contributor Author

This PR has been open for more than a year with 0 comments.

@edwintorok
Copy link
Contributor Author

I should fix the conflicts here, and run the static analyzer. I'll keep the PR open meanwhile though.

@edwintorok edwintorok changed the title OCaml C stubs: release runtime lock around all xenctrl calls CA-391656: OCaml C stubs: release runtime lock around all xenctrl calls Apr 15, 2024
@edwintorok edwintorok closed this Apr 15, 2024
@robhoes robhoes reopened this Apr 16, 2024
ocaml/xenopsd/c_stubs/xenctrlext_stubs.c Outdated Show resolved Hide resolved
ocaml/xenopsd/c_stubs/xenctrlext_stubs.c Outdated Show resolved Hide resolved
unixpwd/c/unixpwd_stubs.c Outdated Show resolved Hide resolved
unixpwd/c/unixpwd_stubs.c Outdated Show resolved Hide resolved
unixpwd/c/unixpwd_stubs.c Show resolved Hide resolved
Copy link
Member

@robhoes robhoes left a comment

Choose a reason for hiding this comment

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

This needs to be updated, as above.

edwintorok and others added 3 commits August 5, 2024 17:22
Most of these calls pass only integers or pointers allocated in C,
so it is safe to release the OCaml runtime lock.

Replace direct passing of Xfm_val/Caml_ba_data_val with storing in a temporary
local variable (the result of Xfm_val or Caml_ba_data_val won't move since they
are C pointers, but the OCaml value passed in as params to these macros might!)

Signed-off-by: Edwin Török <[email protected]>
…e blocking section where possible

unshadow is not thread safe, but the other ones should be.

TODO: double check all C API calls in unixpwd.c with the MT-safe portion of the manpage

Signed-off-by: Edwin Török <[email protected]>
We may intend to upstream the xenctrlext stubs to Xen, so it should follow
the Xen CODING_STYLE.
The .clang-format here is based on a version I sent upstream that tries to
follow that coding style.

Signed-off-by: Edwin Török <[email protected]>
@edwintorok edwintorok force-pushed the private/edvint/toolstack/improve-enter-leave branch from 8545741 to 24d650a Compare August 5, 2024 16:23
@edwintorok edwintorok changed the base branch from feature/perf to master August 5, 2024 16:23
@edwintorok
Copy link
Contributor Author

Changed the base to master because feature/perf doesn't build now due to the change in xs-opam.
Will change the target back to feature/perf, once the feature branch has been updated in #5913

@lindig
Copy link
Contributor

lindig commented Aug 6, 2024

According to the OCaml manual the *blocking_section names are superseded by acquire/release, which I find more meaningful. So I would suggest to use those.

  • caml_release_runtime_system() The calling thread releases the domain lock and other OCaml resources, enabling other threads to run OCaml code in parallel with the execution of the calling thread.
  • caml_acquire_runtime_system() The calling thread re-acquires the domain lock and other OCaml resources. It may block until no other thread in the same domain uses the OCaml run-time system.
  caml_acquire_runtime_system();
  /* Resolve OCaml function vfun to be invoked */
  /* Build OCaml argument varg to the callback */
  vres = callback(vfun, varg);
  /* Copy relevant parts of result vres to C data structures */
  caml_release_runtime_system();

Note: the acquire and release functions described above were introduced in OCaml 3.12. 
Older code uses the following historical names, declared in <caml/signals.h>:

    caml_enter_blocking_section as an alias for caml_release_runtime_system
    caml_leave_blocking_section as an alias for caml_acquire_runtime_system 

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.

3 participants