mctpd: Support MCTP Discovery Notify command - #165
Conversation
9274ea2 to
f6fbc3b
Compare
|
I see there's been a few updates; let me know when this is stable and you'd like a review. |
The code is ready for review, please take a look at this. Thanks. |
jk-ozlabs
left a comment
jk-ozlabs
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I have a few comments.
On the commit message: it reads like a marketing pitch rather than an explanation of the design, or rationale for the implementation. While you don't need to describe the individual changes, I would appreciate some background on the approach, and non-obvious parts of the implementation (like, why the new NLM_F flags?)
In MCTP networks, endpoints broadcast or send a Discovery Notify control
request (0x0D) to inform the Bus Owner when they boot up, reset, or are
hot-plugged. Currently, mctpd relies on active bus scanning or static
configurations,
or primarily: hot-plug events, where the transport provides them
leaving newly online endpoints undiscovered until the next poll cycle.
What poll cycle?
193bacb to
b554b2f
Compare
- Defer EID assignment/discovery to event loop to keep D-Bus unblocked (ack immediately). - Tear down existing peer state to allow clean re-enumeration on re-registration. - Add per-link 32-entry LRU rate limiter (5 req/sec per physical address) to prevent memory exhaustion attacks. - Add unit tests validating Discovery Notify workflows. Assisted-by: Antigravity:Gemini-Next Signed-off-by: Jasmine Cha <chajasmine@google.com>
b554b2f to
71201d5
Compare
|
Hi Jeremy, thanks for catching the issues from the previous code and I've learned a lot from your feedback. I’ve made the updates, so please take a look when you have a chance. |
|
Will do, thanks for the updates! I'm travelling at the moment, so might be a couple of days before I can get to it, but I'll get a review done shortly. |
|
Hi. |
I expect we need to support both modes (DSP0233 v1.0.1 § 5.1.3):
|
|
Hi @jk-ozlabs and @amboar, could you please let me know if my understanding is correct, or if any further changes are needed for this commit? |
|
I think we've converged on the desired design here, I just need to do an actual review. I will get on to that shortly, but there are a couple of other fixes that I need to look at first. |
| } | ||
| warnx("reply_message: EID %d specified without valid physical address info", | ||
| reply_addr.smctp_addr.s_addr); | ||
| return -EINVAL; |
There was a problem hiding this comment.
What is this change needed for?
This seems to be adding a physical-addressed reply fallback, but that's explicitly not what reply_message() is for (as suggested by the function comment), as we have reply_message_phys() for that.
This commit only adds a new call directly to reply_message_phys() , so why the new fallback?
| oldest_time) { | ||
| oldest_time = | ||
| link_data->rate_limits[i].last_discovery_time_us; | ||
| oldest_idx = i; |
There was a problem hiding this comment.
Why store a pointer for the matching entry, but an index for the oldest? Can this be made consistent?
| warnx("Failed to defer EID assignment event for %s", | ||
| dest_phys_tostr(&phys)); | ||
| } | ||
| ctx->pending_discoveries = dctx->next; |
There was a problem hiding this comment.
Might be easier to add to the pending list on success, rather than adding and the removing on failure.
| return rc; | ||
| } | ||
|
|
||
| /* Deduplicate incoming Discovery Notify for both existing and new physical endpoints */ |
There was a problem hiding this comment.
Minor, but please try to keep under 80 cols where possible. The C strings going over length are okay though.
| warnx("Discovery Notify received for existing peer %s (EID %d); tearing down for re-discovery", | ||
| peer_tostr(peer), peer->eid); | ||
| } | ||
| remove_peer(peer); |
There was a problem hiding this comment.
More of a question than a change request, but: Is it possible that we could be doing this non-destructively? (say, if the re-discovered endpoint has the same UUID, and ends up with the same EID allocated)
My concern is that if we see a stray Discovery Notify (for whatever reason) that will interrupt connectivity to the endpoint. As I mentioned earlier, we may be able to use the recovery path for this.
Or, if we decide that receiving a Discovery Notify is due cause to re-initialise completely, then that's also OK, but we should probably document that somewhere.
|
|
||
| process_deferred_discovery(ctx, dctx); | ||
| free(dctx); | ||
| } |
There was a problem hiding this comment.
In your commit message, you have:
- Defer EID assignment/discovery to event loop to keep D-Bus unblocked (ack immediately).
But this loop seems to contradict this objective. An enqueued set of deferred discoveries will now block until all of them are processed, which would seem to have worse dbus behaviour than just processing immediately on Discovery Notify receive.
I do see the point of deferring, in order to meet message response timing, but if you're doing it for dbus-blocking reasons, this seems to be exactly the wrong approach.
4 participants
Implement support for the MCTP Discovery Notify control command in mctpd. When an MCTP endpoint issues a Discovery Notify control request to the Bus Owner, mctpd immediately acknowledges the request over the physical socket and defers EID assignment to the main systemd event loop.
This avoids blocking the event thread during control message processing and safely handles EID re-assignments via change_peer_eid(), keeping D-Bus object paths and netlink kernel routing tables synchronized. Also include unit test coverage for Discovery Notify in the test suite.
Assisted-by: Antigravity:Gemini-Next