mctpd: replace sd-event timers #123

khangng-ampere · 2025-10-15T03:35:19Z

In tests, replace all sd_event timer usages with trio.testing.MockClock
backed I/O sources.

Because we still have real timeouts (D-Bus call to mctpd subprocess,
mctpd SO_RCVTIMEO wait, ...), autojump_threshold (how much to wait
before skipping Trio timeouts) is set to a reasonable value to take that
into account.

khangng-ampere · 2025-10-15T03:40:42Z

This PR is still missing some crucial pieces:

Communicate to the callback timer what is the next time to wait for.
Calling the timer entrypoint from the test runner. If we can, we should be able to use all the trio timers and time travel functions for free.

khangng-ampere · 2025-10-15T03:44:03Z

src/mctpd.c

-	/*
-	 * Error handling policy:
-	 *
-	 * 1. Any resource management error prior to Treclaim is handled by
-	 *    rescheduling the poll query, unless it is scheduling the poll
-	 *    query itself that fails.
-	 *
-	 * 2. If scheduling the poll query fails then the endpoint is removed.
-	 */


I believe this comment is for when sd_event_source_* fails. It does not applies anymore, so I removed it.

khangng-ampere · 2025-10-16T04:28:57Z

From this rough draft, I managed to cut down test time from 20~ seconds to around ~8 seconds.

In theory, if every timeout is controlled by trio, we would get a deterministic run and the whole test will be heavily CPU-bound without waiting for any timeouts thanks to trio MockClock. However, we still have IPC timeout (D-Bus calls) and socket timeout (via SO_RCVTIMEO in the prior commit), so there is still some timeout that we have to wait for.

That is what reflected in autojump_clock.autojump_threshold

khangng-ampere · 2025-10-16T05:41:01Z

I think there should be a way to update the deadline for the global timer. Maybe instead of registering the timer callback as the API, a timerfd mirrored API in mctp_ops should be better? That way, in both release and test build, we can all use sd_event_add_io. Release uses a real timerfd, while test uses a stream socket.

khangng-ampere · 2025-10-17T04:11:00Z

Instead of a custom callback, I changed the interface to mirror what timerfd do. Now the shared common part uses sd_event_add_io for both tests and release build.

I actually have not tested this on a real mctp system yet, so the real timerfd backend is totally untested.

Still, I think this is roughly what I wanted to see, so I will set the PR to Ready for review.

khangng-ampere · 2025-10-17T04:20:38Z

src/mctpd.c

+int global_time_callback(sd_event_source *source, int timerfd, uint revents,
+			 void *userdata)
+{
+	struct ctx *ctx = userdata;
+	uint64_t n_expireds;
+	size_t i;
+
+	mctp_ops.timerfd.read(timerfd, &n_expireds);
+	mctp_ops.timerfd.gettime(timerfd, &ctx->now);
+
+	for (i = 0; i < ctx->num_peers; i++) {
+		struct peer *p = ctx->peers[i];
+		if (p->recovery.deadline <= ctx->now) {
+			peer_endpoint_recover(p);
+		}
+	}
+
+	return 0;
+}


Question: Now that we use a timerfd-like interface, do we need to do this anymore?

We should be able to just keep all the peer->recovery.source, and now do I/O sources instead of time sources. This option should keep the diff fairly minimal, but I am not sure about now coalescing timers into one global I/O timer (which sd-event does internally AFAIK).

I actually went ahead and did it anyway :^) Looks much clearer IMO, we can do this change later if we want.

khangng-ampere · 2025-10-17T05:51:28Z

For the record, test-mctpd time reduced from ~20s to ~6s. I believe it can be even shorter if we managed to also stub out all the SO_RCVTIMEO.

Previous CI runs:

1/3 test-mctpd          OK             18.45s   44 subtests passed

To:

1/3 test-mctpd          OK              6.60s   44 subtests passed

khangng-ampere · 2025-10-17T07:06:12Z

I also went ahead and remove the SO_RCVTIMEO commit 😄 This PR is now purely replacing sd_event_add_io with sd_event_add_time.

jk-ozlabs · 2025-10-23T02:02:25Z

I can't say I'm super keen on using timerfds. Constraining complexity in the production code takes priority over test implementations, for me.

I'll do some experimentation and see if we can mock with the standard sd_event time handling.

khangng-ampere · 2025-10-23T02:25:10Z

I can't say I'm super keen on using timerfds. Constraining complexity in the production code takes priority over test implementations, for me.

I'll do some experimentation and see if we can mock with the standard sd_event time handling.

I see, that makes sense. I think I can change the API to that of sd_event on top of this though... I will also try to see if I can do anything

khangng-ampere · 2025-10-23T03:53:10Z

Maybe this is what you meant? The current issue is that because mctp_ops is now dependent on sd_event, that requires linking to libsystemd, which is not available on the tests for mctp. I guess we need to introduce some HAVE_LIBSYSTEMD, but let me know if this is on the right track.

khangng-ampere · 2025-10-23T06:19:06Z

Terribly sorry for the noise, but in the end, I could not even get it to work 💀 My extremely ugly HAVE_LIBSYSTEMD addition made it compile, but I am not sure why the tests failed on CI but passed on my local machine. I will leave it as-is for now.

(Also, is this a bug in the new commit to upload the log artifact? Since I see the upload log step is skipped)

khangng-ampere · 2025-10-28T06:29:58Z

Aha, it was a memory leak when creating the event source in floating mode. CI should be fixed now, hopefully...

jk-ozlabs · 2025-10-31T04:20:57Z

I think this looks better, but would like to see if we can decouple the test suite timer from mctpd's own concept of time. This would mean replacing the trio autojump timer with one that we could explicitly advance in the test cases.

The challenge there is syncronisation between the advance and the registration of a new timer though...

khangng-ampere · 2025-10-31T04:30:44Z

I think this looks better, but would like to see if we can decouple the test suite timer from mctpd's own concept of time. This would mean replacing the trio autojump timer with one that we could explicitly advance in the test cases.

The challenge there is syncronisation between the advance and the registration of a new timer though...

I did not include it inside this PR, but I am pretty sure that we got that for free though.

I believe we can just do

@pytest.fixture
def manual_clock():
    """
    Default clock with manual advance, and no autojump
    """
    return trio.testing.MockClock()

In tests that use it, you can manually add that fixture to tests and...

manual_clock.tick(10)

to advance 10 seconds, and all trio.sleep in that tests shall be triggered.

Do you have any tests in mind? I can try to add that to this MR as a proof that it should work.

jk-ozlabs · 2025-10-31T07:00:44Z

OK, that looks pretty neat. I did not have anything specific, just that we may need that level of control at some point.

I would like to investigate decoupling the trio clock from mctpd's clock, but that could come as a later change.

jk-ozlabs · 2025-11-05T08:20:05Z

What's the general plan here? The final HAVE_LIBSYSTEMD change looks like a temporary fix - or is that something you're planning to include as-is?

khangng-ampere · 2025-11-05T08:25:05Z

Ouch sorry, totally forget about that, I intended to ask if there is any better way to achieve that (building mctp without libsystemd linked, despite the mctp_ops having some references to sd_event) or if you are fine with this way. If you are fine with adding -DHAVE_LIBSYSTEMD=0 like I did here, I will squash all into one commit

khangng-ampere · 2025-11-05T08:30:06Z

meson.build

 executable('mctp',
    sources: ['src/mctp.c'] + netlink_sources + util_sources + ops_sources,
    install: true,
+    c_args: ['-DHAVE_LIBSYSTEMD=0'],


I mainly feel hesitant here. It feels like I am doing something that configure_file supposed to solve. Maybe a separate config.h for each target?

jk-ozlabs · 2025-11-05T08:46:44Z

I intended to ask if there is any better way to achieve that

I'll take a look. Maybe we can add some logic to override for tests only.

In tests, replace all sd_event timer usages with trio.testing.MockClock backed I/O sources. Because we still have real timeouts (D-Bus call to mctpd subprocess, mctpd SO_RCVTIMEO wait, ...), autojump_threshold (how much to wait before skipping Trio timeouts) is set to a reasonable value to take that into account. Signed-off-by: Khang D Nguyen <[email protected]>

khangng-ampere · 2025-11-05T08:59:06Z

Just in case, to reduce back and forth, I squashed the change into one commit. This is my final take

jk-ozlabs · 2025-11-06T04:50:33Z

Any thoughts on something like this: jk-ozlabs@e691647 ?

khangng-ampere · 2025-11-06T05:16:09Z

Any thoughts on something like this: jk-ozlabs@e691647 ?

The name clarification looks good to me. I have no comments on -DOPS_SD_EVENT=1, seem like that is the only way to achieve this.

I assume I don't need to pick the commits into this PR because you will merge your branch along with my commit, but let me know if I should do so!

jk-ozlabs · 2025-11-06T05:37:15Z

Yep, all good there. Thank you for the contribution!

khangng-ampere marked this pull request as draft October 15, 2025 03:36

khangng-ampere changed the title ~~Draft: mctpd: replace sd-event timers~~ mctpd: replace sd-event timers Oct 15, 2025

khangng-ampere commented Oct 15, 2025

View reviewed changes

khangng-ampere force-pushed the khangng/push-qqstxwtxyuny branch 2 times, most recently from 1dfa72c to 37586d0 Compare October 17, 2025 04:06

khangng-ampere marked this pull request as ready for review October 17, 2025 04:11

khangng-ampere commented Oct 17, 2025

View reviewed changes

khangng-ampere force-pushed the khangng/push-qqstxwtxyuny branch 2 times, most recently from 14f61d5 to bae46e3 Compare October 17, 2025 05:43

khangng-ampere force-pushed the khangng/push-qqstxwtxyuny branch from bae46e3 to 3fa2dd3 Compare October 17, 2025 07:04

khangng-ampere force-pushed the khangng/push-qqstxwtxyuny branch from 3fa2dd3 to fa81ea0 Compare October 23, 2025 03:46

khangng-ampere force-pushed the khangng/push-qqstxwtxyuny branch 3 times, most recently from e8fe015 to 3d78f8f Compare October 23, 2025 06:12

khangng-ampere force-pushed the khangng/push-qqstxwtxyuny branch 4 times, most recently from 8b180b8 to fbea32b Compare October 28, 2025 06:28

khangng-ampere force-pushed the khangng/push-qqstxwtxyuny branch from fbea32b to 9213b7b Compare October 29, 2025 08:47

khangng-ampere mentioned this pull request Oct 31, 2025

MCTP Bridge Polling #85

Open

khangng-ampere force-pushed the khangng/push-qqstxwtxyuny branch from 9213b7b to 0cd7990 Compare October 31, 2025 04:03

khangng-ampere commented Nov 5, 2025

View reviewed changes

khangng-ampere force-pushed the khangng/push-qqstxwtxyuny branch from 0cd7990 to 5f38f94 Compare November 5, 2025 08:58

jk-ozlabs merged commit 5f38f94 into CodeConstruct:main Nov 6, 2025
3 checks passed

mctpd: replace sd-event timers #123

mctpd: replace sd-event timers #123

Uh oh!

Conversation

khangng-ampere commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

khangng-ampere commented Oct 15, 2025

Uh oh!

khangng-ampere Oct 15, 2025

Choose a reason for hiding this comment

Uh oh!

khangng-ampere commented Oct 16, 2025

Uh oh!

khangng-ampere commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

khangng-ampere commented Oct 17, 2025

Uh oh!

khangng-ampere Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

khangng-ampere Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

khangng-ampere commented Oct 17, 2025

Uh oh!

khangng-ampere commented Oct 17, 2025

Uh oh!

jk-ozlabs commented Oct 23, 2025

Uh oh!

khangng-ampere commented Oct 23, 2025

Uh oh!

khangng-ampere commented Oct 23, 2025

Uh oh!

khangng-ampere commented Oct 23, 2025

Uh oh!

khangng-ampere commented Oct 28, 2025

Uh oh!

jk-ozlabs commented Oct 31, 2025

Uh oh!

khangng-ampere commented Oct 31, 2025

Uh oh!

jk-ozlabs commented Oct 31, 2025

Uh oh!

jk-ozlabs commented Nov 5, 2025

Uh oh!

khangng-ampere commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

khangng-ampere Nov 5, 2025

Choose a reason for hiding this comment

Uh oh!

jk-ozlabs commented Nov 5, 2025

Uh oh!

khangng-ampere commented Nov 5, 2025

Uh oh!

jk-ozlabs commented Nov 6, 2025

Uh oh!

khangng-ampere commented Nov 6, 2025

Uh oh!

Uh oh!

jk-ozlabs commented Nov 6, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

khangng-ampere commented Oct 15, 2025 •

edited

Loading

khangng-ampere commented Oct 16, 2025 •

edited

Loading

khangng-ampere Oct 17, 2025 •

edited

Loading

khangng-ampere commented Nov 5, 2025 •

edited

Loading