pid1: rework handoff timestamp #32441

poettering · 2024-04-23T21:36:45Z

Fixes: #32404

src/core/exec-invoke.c

keszybz

Looks reasonable…

src/core/manager.c

src/core/exec-invoke.c

src/core/execute.c

src/core/manager.c

poettering · 2024-04-24T10:39:54Z

I split out a bunch of commits into their own PRs now, so that they can be review/merged more quickly.

poettering · 2024-04-25T10:16:46Z

Posted a new version, addressing @YHNdnzj's comments. And I dropped the calendar time from the dump output if we can show the monotonic time diff instead, as mentioned.

YHNdnzj

Looks great, but still some comments

YHNdnzj · 2024-04-25T10:27:22Z

src/core/execute.c

+                                FORMAT_TIMESPAN(usec_sub_unsigned(s->exit_timestamp.monotonic, s->handoff_timestamp.monotonic), 1));
+                else if (dual_timestamp_is_set(&s->start_timestamp) && s->exit_timestamp.monotonic > s->start_timestamp.monotonic)
+                        fprintf(f,
+                                "%sExit Timestamp: %s since start\n",


Hmm, I don't think Exit timestamp should be switched to only show delta?

The point of handoff timestamp is to measure the sd-executor latency and such, and that ought to be an extremely short period, hence it makes sense to only show timespan for that. But that reasoning doesn't apply to the exit timestamp?

well, i agree, but the way i understood @keszybz is that he doesn't want to see the calendar time again.

at this point i really don't care anymore, let's just get this merged, i'll change the timestamp display on this to anything people are happy with.

i am happy to drop that commit btw, if it's so hard to agree on an outout acceptable to everyone.

src/core/manager.c

src/core/service.c

src/core/unit.h

assert_se() should not be used here, these checks are paranoia only and have no side-effect after all. hence fix this to use assert(), or in fact ASSERT_PTR()

…dren This adds an AF_UNIX socket pair to the manager that we can collect handoff timestamp messages on. The idea is that forked off children send a datagram with a timestamp and we use its sender PID to match it against the right forked off process. This part only implements the receiving side: a socket is created, and listened on. Received datagrams are parsed, verified and then dispatched to the interested units.

This changes the executor to systematically send handoff timestamps to the service manager if a socket for that is supplied. This drops the code that did this via Type=exec messages, and reverts that part to the old behaviour before 93cb78a. Benefits of this approach: 1. We can collect the handoff for any command we fork off, regardless if it's ExecStart= something else, regardless whether it's Type=exec, Type=simple or some any other service type, regardless of the unit type. 2. We collect both CLOCK_REALTIME and CLOCK_MONOTONIC, as we do for the other process timestamps. 3. It's entirely backwards compatible, as this doesn't change the protocol between service manager and executor, but just extends it.

poettering · 2024-04-25T11:36:09Z

Force pushed a new version now, addressing @YHNdnzj's point.

didn't touch the dump timestamp display, because I think there's disagreement on what to show. while i am personally fine with anything as long as this is merged, i am not sure what that "anything" should be, that makes everyone happy.

bluca · 2024-04-25T11:36:11Z

Github helpfully hides it even if it's unresolved, please see #32441 (comment)

…ing logic Also: rename Handover → Handoff. I think it makes it clearer that this is not really about handing over any resources, but that the executor is out off the game from that point on.

… units

Let's show the runtimes of our commands and preparations for them. It's actually quite interesting, we sometimes are irritatingly slow with our handoffs.

poettering · 2024-04-25T11:43:53Z

Github helpfully hides it even if it's unresolved, please see #32441 (comment)

updated with a comment about this. ptal.

bluca

Thanks, looks good to me

Addresses systemd#32441 (comment)

bobrik · 2024-06-13T00:59:24Z

src/core/exec-invoke.c

+        dual_timestamp dt;
+        dual_timestamp_now(&dt);
+
+        if (send(p->handoff_timestamp_fd, (const usec_t[2]) { dt.realtime, dt.monotonic }, sizeof(usec_t) * 2, 0) < 0) {


This send might not be allowed by seccomp: #33299.

poettering added pid1 dont-merge 💣 labels Apr 23, 2024

github-actions bot added documentation tests please-review PR is ready for (re-)review by a maintainer labels Apr 23, 2024

bluca reviewed Apr 24, 2024

View reviewed changes

src/core/exec-invoke.c Outdated Show resolved Hide resolved

bluca reviewed Apr 24, 2024

View reviewed changes

src/core/exec-invoke.c Outdated Show resolved Hide resolved

bluca added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Apr 24, 2024

yuwata marked this pull request as draft April 24, 2024 04:02

keszybz reviewed Apr 24, 2024

View reviewed changes

src/core/manager.c Outdated Show resolved Hide resolved

src/core/manager.c Outdated Show resolved Hide resolved

src/core/exec-invoke.c Outdated Show resolved Hide resolved

src/core/exec-invoke.c Outdated Show resolved Hide resolved

YHNdnzj reviewed Apr 24, 2024

View reviewed changes

src/core/execute.c Outdated Show resolved Hide resolved

src/core/manager.c Outdated Show resolved Hide resolved

src/core/manager.c Outdated Show resolved Hide resolved

poettering added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks dont-merge 💣 labels Apr 24, 2024

poettering changed the title ~~pid1: rework handoff timestamp (WIP)~~ pid1: rework handoff timestamp Apr 24, 2024

poettering force-pushed the rework-handoff-timestamp branch from 4b960ad to bc22ac7 Compare April 24, 2024 10:16

github-actions bot added the util-lib label Apr 24, 2024

poettering marked this pull request as ready for review April 24, 2024 10:16

poettering force-pushed the rework-handoff-timestamp branch from bc22ac7 to f0c4b3f Compare April 24, 2024 10:24

poettering mentioned this pull request Apr 24, 2024

core: move handover_timestamp from ExecStatus to Service #32405

Closed

poettering added please-review PR is ready for (re-)review by a maintainer and removed needs-rebase labels Apr 25, 2024

YHNdnzj reviewed Apr 25, 2024

View reviewed changes

YHNdnzj added the rc-blocker 🚧 PRs and Issues tagged this way are blocking the upcoming rc release! label Apr 25, 2024

poettering added 3 commits April 25, 2024 13:32

core: fix assertions in manager_dispatch_user_lookup_fd()

33fc8b0

assert_se() should not be used here, these checks are paranoia only and have no side-effect after all. hence fix this to use assert(), or in fact ASSERT_PTR()

poettering force-pushed the rework-handoff-timestamp branch from cd8af95 to ba4f661 Compare April 25, 2024 11:34

poettering added 6 commits April 25, 2024 13:40

manager: switch service unit type over to using new handoff timestamp…

3c1d1ca

…ing logic Also: rename Handover → Handoff. I think it makes it clearer that this is not really about handing over any resources, but that the executor is out off the game from that point on.

socket: hookup handoff timestamps with processes forked off by socket…

1f41b10

… units

mount: hook up with handoff timestamps

19ae898

swap: hook up with handoff timestamps

2b53218

execute: make ExecStatus dump more useful by showing passed time

be3444f

Let's show the runtimes of our commands and preparations for them. It's actually quite interesting, we sometimes are irritatingly slow with our handoffs.

update TODO

3c64aca

poettering force-pushed the rework-handoff-timestamp branch from ba4f661 to 3c64aca Compare April 25, 2024 11:43

bluca approved these changes Apr 25, 2024

View reviewed changes

YHNdnzj approved these changes Apr 25, 2024

View reviewed changes

YHNdnzj added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed please-review PR is ready for (re-)review by a maintainer labels Apr 25, 2024

poettering merged commit 5693208 into systemd:main Apr 25, 2024
42 of 49 checks passed

github-actions bot removed the good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed label Apr 25, 2024

This was referenced Apr 25, 2024

sd-daemon: Set SO_LINGER on AF_VSOCK notify socket fds #32483

Merged

Potential PAM-related memory leak in systemd-executor #32486

Open

YHNdnzj added a commit to YHNdnzj/systemd that referenced this pull request Apr 27, 2024

core: use close_and_replace more when deserializing

0541d29

Addresses systemd#32441 (comment)

bobrik mentioned this pull request Jun 13, 2024

systemd 256 fails seccomp on service startup due to sendto() usage #33299

Open

bobrik reviewed Jun 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pid1: rework handoff timestamp #32441

pid1: rework handoff timestamp #32441

poettering commented Apr 23, 2024 •

edited

keszybz left a comment

poettering commented Apr 24, 2024

poettering commented Apr 25, 2024

YHNdnzj left a comment

YHNdnzj Apr 25, 2024

poettering Apr 25, 2024

poettering commented Apr 25, 2024

bluca commented Apr 25, 2024

poettering commented Apr 25, 2024

bluca left a comment

bobrik Jun 13, 2024

pid1: rework handoff timestamp #32441

pid1: rework handoff timestamp #32441

Conversation

poettering commented Apr 23, 2024 • edited

keszybz left a comment

Choose a reason for hiding this comment

poettering commented Apr 24, 2024

poettering commented Apr 25, 2024

YHNdnzj left a comment

Choose a reason for hiding this comment

YHNdnzj Apr 25, 2024

Choose a reason for hiding this comment

poettering Apr 25, 2024

Choose a reason for hiding this comment

poettering commented Apr 25, 2024

bluca commented Apr 25, 2024

poettering commented Apr 25, 2024

bluca left a comment

Choose a reason for hiding this comment

bobrik Jun 13, 2024

Choose a reason for hiding this comment

poettering commented Apr 23, 2024 •

edited