Miscellaneous improvements to mctpd #81
Conversation
Currently, when mctpd adds new addresses to the routing table, unit tests will not receive it. This adds the missing test infrastructure to handle new addresses added to the routing table. Signed-off-by: Khang D Nguyen <khangng@os.amperecomputing.com>
This adds missing code to deallocate an iid after being used for an response. Signed-off-by: Khang D Nguyen <khangng@os.amperecomputing.com>
Currently, mctp-netlink requires passing interface name to its API. (due to mctp.c being written first) However, in mctpd, we always get ifindex from sockaddr. This leads to mctpd having to convert ifindex to interface name, and then mctp-netlink will convert the interface name back to ifindex. This change removes the unnecessary roundtrip. ifindex is now the main thing to pass around. mctp.c will do its own conversion. Signed-off-by: Khang D Nguyen <khangng@os.amperecomputing.com>
Extracts netlink addresses utils from mctp.c into mctp-netlink.c for reuse in mctpd Set EID. Signed-off-by: Khang D Nguyen <khangng@os.amperecomputing.com>
All the usages of reply_message currently have access to the peer physical addresses. Doing this by default should avoid an unnecessary kernel routing table lookup in most cases. Signed-off-by: Khang D Nguyen <khangng@os.amperecomputing.com>
There was a problem hiding this comment.
All looks good - just that the last change is a bit mysterious - why are we trying to bypass the kernel routing tables there?
(I assume this is due to interactions during Set Endpoint ID processing, as the local EID may have changed as a side effect of the command. However, that sounds like something we should special-case in Set Endpint ID, rather than use globally.
Also, won't you need a facility to respond using a zero EID, if the Set Endpoint ID fails?)
| int mctp_nl_addr_add(struct mctp_nl *nl, mctp_eid_t eid, int ifindex) | ||
| { | ||
| struct mctp_addralter_msg msg; | ||
| int rc; | ||
|
|
||
| rc = fill_addralter_args(nl, &msg, NULL, NULL, eid, ifindex); | ||
| if (rc) | ||
| return -1; | ||
|
|
||
| msg.nh.nlmsg_type = RTM_NEWADDR; | ||
|
|
||
| return mctp_nl_send(nl, &msg.nh); | ||
| } | ||
|
|
||
| int mctp_nl_addr_del(struct mctp_nl *nl, mctp_eid_t eid, int ifindex) | ||
| { | ||
| struct mctp_addralter_msg msg; | ||
| int rc; | ||
|
|
||
| rc = fill_addralter_args(nl, &msg, NULL, NULL, eid, ifindex); | ||
| if (rc) | ||
| return -1; | ||
|
|
||
| msg.nh.nlmsg_type = RTM_DELADDR; | ||
|
|
||
| return mctp_nl_send(nl, &msg.nh); | ||
| } |
There was a problem hiding this comment.
With the only difference being the type here, could we simplify this into a parameter? This would also mean you don't need the rtm_command check in mctp.c to choose a function...
There was a problem hiding this comment.
I think API design wise for mctp-netlink.h, separate mctp_nl_addr_add and mctp_nl_addr_del should be superior. I don't think there is any common case that need adding or deleting parameterized. Or i.e, I think
mctp_nl_addr_add(...)
is better than
mctp_nl_addr(..., RTM_DELADDR)
It is also less error-prone to pass something else to it.
I think the problem might be mctp.c. Currently it fuses addr add and addr del into one code path, and then split the code path again with the rtm_command. Let me also clean that up by separating the code path.
Actually, it comes from Get Endpoint ID. mctpd needs to respond to Get Endpoint ID in the case of empty routing table and no EID are assigned. The DSP0236 spec section 12.5 says
But it is true as you said, I could have added a (mctpd is also supposedly the one to manage and create the routing table, so having the reverse dependency of the routing table decides how mctpd works is a bit... tangled.) But either way, I should reword the commit to mention this Get Endpoint ID issue, and a testcase to cover this regardless of what direction we head.
That's true... It should work w.r.t current test facility or MCTP specification. But Linux kernel wise, I might need a patch for it alongside supporting Set Endpoint ID. |
I would prefer if we defaulted to EID-based addressing unless we have a specific reason to do otherwise (ie., in scenarios like your example, where we may not have routing established). By bypassing the kernel routing, we may have divergent behaviour between what the kernel understands to be the correct route, and what mctpd will use.
We have a physical address, which might not be the physical address of that specific endpoint (eg, for not-directly-attached devices), or may not be a valid physical address at that specific point in time. It's slightly less overhead, but slightly more chance of being incorrect. (emphasis on slightly though; the scenarios in which the tx physaddr differs from the rx physaddr are pretty obscure) |
Those commits are spawned out of my effort to support Set Endpoint ID, fixing some issues and papercuts in mctpd.
(See e14eaed for my upcoming Set Endpoint ID commit. It might be too big and harder to merge so I split them off from these misc commits)