MCTP Bridge Polling

[faizana-nvidia]

MCTP Bridge Endpoint poll

A MCTP Bridge is assigned range of EIDs for its downstream endpoints depending upon bridge's applicable pool size. MCTP Base Spec introduces MCTP control command AllocateEndpointID to allocate these set of range of EIDs. It's possible to have downstream endpoints detached/undiscovered due to some error in its internal bus or via physical hotplug removal (such as usb bus aligned devices). One way to detect the removal/presence of its downstream devices, make buswoner probes on each bridge's downstream endpoint using the Get Endpoint ID command. If a device fails to respond, it is treated as no longer present on the bus. The corresponding EID (if was assigned) is then released back to the bridge pool for reassignment.

MCTP Reservation of Bridge EID

Once MCTP Bridge is allocated set of Endpoint IDs, it would assign the upcoming discoverable downstream devices among those range of eids. For a discovered downstream device i.e once it responds to poll command, mctpd then creates a peer for it and exposes its D-bus object and marks that eid as used from its EID pool.

This PR is based on :

1. MCTP Bridge Support : Mctp bridge support #71

flowchart TD
    AA[Allocate Endpoint Success] --> BB[For Each EID]
    BB --> A[Start Polling EID]
    A --> B[Send GET_ENDPOINT_ID]
    B --> C{Response?}
    
    C -->|No Response| D[Timeout]
    D --> E[Increment Failure Count]
    E --> F{Count > Threshold?}
    
    F -->|No| G[Schedule Next Poll]
    F -->|Yes| H[Remove Peer]
    H --> G
    
    C -->|Response Received| X[Reset Timeourt]
    X --> I{Peer Exists?}
    I -->|No| J[Create Peer]
    I -->|Yes| M[Continue Polling]
    
    J --> L[Publish to D-Bus]
    L --> M
    
    G --> B
    M --> B
    
    N[Bridge Removed] --> O[Clean Poll Context]
    O --> P[Remove Downstream Peers]
    P --> Q[Stop All Polling]
    
    style M fill:#ffcdd2
    style G fill:#fff3e0
    style O fill:#e8f5e8
    style P fill:#e8f5e8
    style AA fill:#e1f5fe
    style BB fill:#e1f5fe

Loading

[jk-ozlabs]

Bridges allocate EIDs for downstream endpoints but don't create peer structures until endpoints are actively discovered

Why not just store the allocated pool range on the bridge peer? This would be a considerably simpler change, and means the seaprate net->resv_eid_set doesn't get out-of-sync with the peer data.

[faizana-nvidia]

Bridges allocate EIDs for downstream endpoints but don't create peer structures until endpoints are actively discovered

Why not just store the allocated pool range on the bridge peer? This would be a considerably simpler change, and means the seaprate net->resv_eid_set doesn't get out-of-sync with the peer data.

For now we are storing the start of pool and pool size (in some sense it represents allocated pool EID range), would this requirement still needed?

[jk-ozlabs]

For now we are storing the start of pool and pool size (in some sense it represents allocated pool EID range), would this requirement still needed?

Not sure I understand the question - if the struct peer stores the allocated pool range, then we wouldn't need to store them elsewhere.

In your eccc13f change, the peer->pool_start and peer->pool_size already soft-of represent this, but you don't have a clear indication of whether those represent an actual allocation, or a "proposed" pool. If you can clear up that, then you can just use those fields to represent the allocation data directly.

[faizana-nvidia]

For now we are storing the start of pool and pool size (in some sense it represents allocated pool EID range), would this requirement still needed?

Not sure I understand the question - if the struct peer stores the allocated pool range, then we wouldn't need to store them elsewhere.

In your eccc13f change, the peer->pool_start and peer->pool_size already soft-of represent this, but you don't have a clear indication of whether those represent an actual allocation, or a "proposed" pool. If you can clear up that, then you can just use those fields to represent the allocation data directly.

I see, I think I understand what your ask is here, peer->pool_start and peer->pool_size should represent "allocated" information (coming as response to ALLOCATE_EID command whatever bridge's final say is). We are initially storing the "proposed" pool start information and minimum of "proposed" pool size and actual pool_size (response from SET ENDPOINT ID) over these two vars and request is sent for ALLOCATION based on the final proposed data, but once we get a response, whatever has been allocated by bridge, we are keeping it as pool_start and pool_size inside peer. Then later marking this range as reserved in the new reserve_eid_set per network.

But I see your point now, our bridge could have any pool_size as actual size coming in response to SET_ENDPOINT_ID but the proposed size could differ and so could finally allocated size too(response of ALLOCATE_ENDPOINT_ID)

May be we can introduce a new var representing actual_pool_size (response of SET_ENDPOINT ID) and call out these two vars, peer->pool_start and peer->pool_size as allocated data while network reserved_eid_set continues to interact with these vars as it has been doing.

[faizana-nvidia]

I also have added a TODO comment here todo

in our current implementation of Busowner pool and local eid assignment, we don't have proper check to see, if localEID which is being set via mctp tool (mctp addr add <eid> dev <interface>) for any network, has already been used by Busowner when it assigned dynamic/static EID to any MCTP device/endpoint.

Reference: I have two interfaces mctpusb0 and mctpspi0_2. I try to add another local EID to mctspi0_2 (EID 20) . Here EID 20 was already used up via Bridge Downstream EID under mctpusb0 thus the error message, I still see mctp link map retaining the eid 20 even though we had error in setting local eid (not seen in mctp tool but mctpd) Not sure if this was intentionally done?

mctpd[22871]: Adding local eid 20 net 1
mctpd[22871]: mctpd: Local eid 20 net 1 already exists?  <-----------
mctpd[22871]: mctpd: Error handling netlink update         <-----------
mctpd[22871]: 1 changes:
mctpd[22871]:   0 ADD_EID      ifindex   4 (mctpspi0_2          ) eid  20 old_net    0 old_up 5
mctpd[22871]: linkmap
mctpd[22871]:    1: lo, net 1 up local addrs []
mctpd[22871]:    4: mctpspi0_2, net 1 up local addrs [12, 20]
mctpd[22871]:    7: mctpusb0, net 1 up local addrs [8]

$mctp addr show
eid 12 net 1 dev mctpspi0_2
eid 20 net 1 dev mctpspi0_2
eid 8 net 1 dev mctpusb0

Since we are getting into the reservation of EIDs, our new localEID could very well be from the reserved EID space. So should we introduce restriction of such manner in mctp tool/mctpd?

[jk-ozlabs]

May be we can introduce a new var representing actual_pool_size (response of SET_ENDPOINT ID) and call out these two vars, peer->pool_start and peer->pool_size as allocated data while network reserved_eid_set continues to interact with these vars as it has been doing.

Yes, that's along the lines of what I was thinking. But maybe with the names adjusted: pool_start & pool_size would represent actually-allocated values, req_pool_size and req_pool_start for the (temporarily-valid) request details.

And then as an improvement: since the req_ values are transient, is there any better way to pass them from the Set Endpoint ID response handler to the Allocate Endpoint IDs command? (ie., rather than setting transient values on the struct peer)

I also have added a TODO comment here

There's not much we can do about that case; mctpd isn't (currently) managing that local EID, so there is not much we can do on conflicts - as you have noted.

So should we introduce restriction of such manner in mctp tool/mctpd?

No, not while mctpd is not responsible for assigning local EIDs.

I see the best way to prevent this case is a facility to explicitly configure mctpd with the range of dynamically-allocatable EIDs, and it's up to the platform integrator to ensure that that range does not conflict with the potential set of local EIDs.

(that's one of the reasons for the configuration file support, for exactly this use-case)

[faizana-nvidia]

And then as an improvement: since the req_ values are transient, is there any better way to pass them from the Set Endpoint ID response handler to the Allocate Endpoint IDs command? (ie., rather than setting transient values on the struct peer)

1. To add further onto this, we don't know how many birdges on what bindings would turn up on network. Predicting their numbers would rather be tedious, so would be passing each pool size to their respective Allocate EndpointID. So in worse case scenario assuming 255 bridges per network(similar to peer numbers) we can have a separate array structure representing bridge data/pool_size which would be filled right after SET_ENDPOINT_ID.

2. One another way (though not tested) is somehow find the capacity of bridge just before asking ALLOCATE_ENDPOINT_ID. Unfortunately we need to know this because we want to avoid asking endpoints number in request which would be more that Bridge's dynamic pool size.

Table 23 – Allocate Endpoint IDs message

An error completion code (ERROR_INVALID_DATA should be returned) shall be returned if the number of EIDs being allocated (Number of Endpoint IDs) exceeds the Dynamic Endpoint ID Pool size. (This error condition does not apply to when the number of endpoint IDs passed in the request is 0x00).

3. We can also think this response of pool_size in SET_EDNPOINT_ID (transient) to be the capacity of bridge while the allocated pool size and start representing allocated values

I also have added a TODO comment here

There's not much we can do about that case; mctpd isn't (currently) managing that local EID, so there is not much we can do on conflicts - as you have noted.

So should we introduce restriction of such manner in mctp tool/mctpd?

No, not while mctpd is not responsible for assigning local EIDs.

I see the best way to prevent this case is a facility to explicitly configure mctpd with the range of dynamically-allocatable EIDs, and it's up to the platform integrator to ensure that that range does not conflict with the potential set of local EIDs.

(that's one of the reasons for the configuration file support, for exactly this use-case)

Acknowledged.

[jk-ozlabs]

we can have a separate array structure representing bridge data/pool_size which would be filled right after SET_ENDPOINT_ID.

If there is no way to better handle the transient requested peer size, I would prefer to just store it on the struct peer.

One another way (though not tested) is somehow find the capacity of bridge just before asking ALLOCATE_ENDPOINT_ID

We have the capacity of the brige, from the Set Endpoint ID response, no? As you say:

We can also think this response of pool_size in SET_EDNPOINT_ID (transient) to be the capacity of bridge while the allocated pool size and start representing allocated values

I would suggest:

1. record the requested pool size from the Set Endpoint ID response
2. attempt to allocate that requested pool size (pending any configured limits), by querying the struct peer array
3. record the allocated pool range on that peer
4. perform Allocate Endpoint IDs from that allocated pool

If the allocation is not possible in (2), just fail for now. We can look at fallbacks in a follow-up.

I think this it fine to do without a new dbus call, but am open to ideas about why we may need one.

[faizana-nvidia]

we can have a separate array structure representing bridge data/pool_size which would be filled right after SET_ENDPOINT_ID.

If there is no way to better handle the transient requested peer size, I would prefer to just store it on the struct peer.

One another way (though not tested) is somehow find the capacity of bridge just before asking ALLOCATE_ENDPOINT_ID

We have the capacity of the brige, from the Set Endpoint ID response, no? As you say:

We can also think this response of pool_size in SET_EDNPOINT_ID (transient) to be the capacity of bridge while the allocated pool size and start representing allocated values

I would suggest:

1. record the requested pool size from the Set Endpoint ID response
2. attempt to allocate that requested pool size (pending any configured limits), by querying the struct peer array
3. record the allocated pool range on that peer
4. perform Allocate Endpoint IDs from that allocated pool

If the allocation is not possible in (2), just fail for now. We can look at fallbacks in a follow-up.

I think this it fine to do without a new dbus call, but am open to ideas about why we may need one.

Please let me know if I understood you correctly on steps

1. Fetch out the transient pool size (via SET_ENDPOINT_ID).
2. Try to allocate (find the pool sized chunked of peer structure which are available to be later used by downstream EID via get_pool_start())
3. If found then update the peer structure with req_pool_size = capacity and req_pool_start = get_pool_start()
4. Also mark all such eids to be reserved
5. From dbus api AssignBridgeStatic get pool_size and try to allocate that size with req_pool_start and whatever is coming as response from ALLOCATE_EID command update peer structure with alloc_pool_size = response(pool_size) and alloc_pool_start=response(pool_start).

Introduces req_pool_size and req_pool_start, updates pool_size -> alloc_pool_size and pool_start ->alloc_pool_start.

If we are going by this then do we have to get pool_start argument from dbus method AssignBridgeStatic if we have already decided what pool start will be from req_pool_start?

Or do you mean not to use any new dbus method AssignBridgeStatic at all?

I think this it fine to do without a new dbus call, but am open to ideas about why we may need one.

[jk-ozlabs]

Since the allocation mechanism is now in #71, I'll shift this discussion to there.

[faizana-nvidia]

Hello Jeremy,
I've rebased the PR and updated the logic based on other PR 71 comments. Please help me with the review.

[jk-ozlabs]

I'm going to hold off on reviewing this one until we have the core bridge support done, given that's a hard dependency for this.

[faizana-nvidia]

I'm going to hold off on reviewing this one until we have the core bridge support done, given that's a hard dependency for this.

understood, sounds good