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

Xapi thread classification - part 2 #6154

Conversation

GabrielBuica
Copy link
Contributor

@GabrielBuica GabrielBuica commented Dec 4, 2024

Adds more granurality in the xapi thread classification. Now, an
individual thread can be mapped to the following:

  • {xapi_cgroup}/internal/SM;
  • {xapi_cgroup}/internal/cli;
  • {xapi_cgroup}/external/intrapool;
  • {xapi_cgroup}/external/unauthenticated;
  • {xapi_cgroup}/external/authenticated/{identity};

where {identity} is a {auth_user_sid}/{user_agent}.
Both {auth_user_sid} and {user_agent} strings are sanitized when
making the identity through Identity.make, by allowing only
alphanumeric characters and a maximum length of 16 characters each.

This should allow for proper thread classification in xapi.

Note: Threads calling back into xapi from xenopsd are yet to be classified.

e.g. Cgroup hierarchy based on the new classification
image

BVT: 208947 & BST: 208972

@GabrielBuica GabrielBuica force-pushed the private/dbuica/feature/perf/tgroups-part-2 branch 2 times, most recently from f91bdb1 to 62a903a Compare December 5, 2024 16:49
@GabrielBuica GabrielBuica marked this pull request as ready for review December 5, 2024 17:25
ocaml/libs/tgroup/tgroup.ml Outdated Show resolved Hide resolved
ocaml/libs/tgroup/tgroup.ml Outdated Show resolved Hide resolved
ocaml/libs/tgroup/tgroup.ml Outdated Show resolved Hide resolved
@lindig
Copy link
Contributor

lindig commented Jan 7, 2025

@mg12 can we get a review? I don't like large PRs lingering here. And @GabrielBuica what about comments?

@GabrielBuica
Copy link
Contributor Author

@lindig I talked over with @mg12 and I was in the middle of addressing his comments.

Copy link
Member

@mg12 mg12 left a comment

Choose a reason for hiding this comment

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

it looks good to me, thanks

Adds more granurality in the xapi thread classification. Now, an
individual thread can be mapped to the following:

- {xapi_cgroup}/internal/SM;
- {xapi_cgroup}/internal/cli;
- {xapi_cgroup}/external/intrapool;
- {xapi_cgroup}/external/unauthenticated;
- {xapi_cgroup}/external/authenticated/{identity};

where {identity} is a {auth_user_sid}/{user_agent}.
Both {auth_user_sid} and {user_agent} strings are sanitized when
making the identity through `Identity.make`, by allowing only
alphanumenric characters and a maximum length of 16 characters each.

This is only the library change.

This should allow for proper thread classification in xapi.

Signed-off-by: Gabriel Buica <[email protected]>
Adds unit test for:
- the correct thread classification of `of_creator`;
- sanitation of `Identity.make`.

Signed-off-by: Gabriel Buica <[email protected]>
Classifies the threads at the time of session creation and inside
`do_dispatch`.

This ensures that new threads created by current session/request inherit
the propper classification.

Note: threads created by xenopsd calling back into xapi are yet to be
classified.

Signed-off-by: Gabriel Buica <[email protected]>
@GabrielBuica GabrielBuica force-pushed the private/dbuica/feature/perf/tgroups-part-2 branch from f29001c to 63391ba Compare January 8, 2025 08:41
@mg12 mg12 merged commit 6589d9a into xapi-project:feature/perf Jan 8, 2025
15 checks passed
edwintorok added a commit that referenced this pull request Jan 13, 2025
Continuing from #6208

The conflict resolution can be reviewed with:
`git log --remerge-diff -1 8b8af63`

```diff
diff --git a/ocaml/libs/xapi-stdext/lib/xapi-stdext-threads/scheduler.ml b/ocaml/libs/xapi-stdext/lib/xapi-stdext-threads/scheduler.ml
remerge CONFLICT (content): Merge conflict in ocaml/libs/xapi-stdext/lib/xapi-stdext-threads/scheduler.ml
index 7bace30dae..27cf306995 100644
--- a/ocaml/libs/xapi-stdext/lib/xapi-stdext-threads/scheduler.ml
+++ b/ocaml/libs/xapi-stdext/lib/xapi-stdext-threads/scheduler.ml
@@ -33,49 +33,8 @@ let (queue : t Ipq.t) = Ipq.create 50 queue_default

 let lock = Mutex.create ()

-<<<<<<< 6589d9a (Xapi thread classification - part 2 (#6154))
 let add_to_queue_span name ty start_span newfunc =
   let ( ++ ) = Mtime.Span.add in
-||||||| 4f3f08f
-module Clock = struct
-  let span s = Mtime.Span.of_uint64_ns (Int64.of_float (s *. 1e9))
-
-  let span_to_s span =
-    Mtime.Span.to_uint64_ns span |> Int64.to_float |> fun ns -> ns /. 1e9
-
-  let add_span clock secs =
-    (* return mix or max available value if the add overflows *)
-    match Mtime.add_span clock (span secs) with
-    | Some t ->
-        t
-    | None when secs > 0. ->
-        Mtime.max_stamp
-    | None ->
-        Mtime.min_stamp
-end
-
-let add_to_queue name ty start newfunc =
-  let ( ++ ) = Clock.add_span in
-=======
-module Clock = struct
-  let span s = Mtime.Span.of_uint64_ns (Int64.of_float (s *. 1e9))
-
-  let span_to_s span = Mtime.Span.to_float_ns span |> fun ns -> ns /. 1e9
-
-  let add_span clock secs =
-    (* return mix or max available value if the add overflows *)
-    match Mtime.add_span clock (span secs) with
-    | Some t ->
-        t
-    | None when secs > 0. ->
-        Mtime.max_stamp
-    | None ->
-        Mtime.min_stamp
-end
-
-let add_to_queue name ty start newfunc =
-  let ( ++ ) = Clock.add_span in
->>>>>>> 8c9b754 (SDK fixes including CP-53003 (#6210))
   let item =
     {
       Ipq.ev= {func= newfunc; ty; name}
```

This code got deleted in `feature/perf`
6b02474, where `span_to_s` got replaced
with `Clock.Timer.span_to_s`, and changed in master by
e68cda7.
Both achieve the same result: `span_to_s` uses `Mtime.Span.to_float_ns`,
except the commit on feature/perf also removes code duplication by
reusing the other clock module.
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.

4 participants