mi: Introduce asynchronous event message handling #967
Conversation
|
@jk-ozlabs, there's something funky going on with the existing mi-mctp tests. I was hoping you might shed some light here. Also looking for guidance on how we can add new tests to handle these changes. |
|
@igaw, it seems we have build checks that have been added since the last time I upstreamed something. But they're all failing for me here. Any guidance you can provide? |
Added new functionality to mi.c and mi-mctp.c to handle AEMs. Included new example mi-mctp-ae.c for usage. Signed-off-by: Chuck Horkin <[email protected]>
chorkin
force-pushed
the
user/chorkin/aem_work_upstream
branch
from
ac53516 to
90ad63d
Compare
| goto cleanup_get_info; | ||
| } | ||
| //Need to send a AEM Sync to enable things | ||
| #ifndef TEST_CODE |
There was a problem hiding this comment.
@jk-ozlabs , I plan to remove this code, but was going to keep it in if it was somehow useful for testing purposes and we could reuse it somewhere else. I'll delete or move pending other comments I get.
|
|
||
| /* POLLIN has indicated events, notify libnvme. Will likely result in | ||
| * one (or more) of the struct nvme_mi_aem_callbacks being invoked. | ||
| * NOTE: WE NEED TO LOOK AT AEM GENERATION NUMBER HERE. See AEMTI |
| }; | ||
|
|
||
| LIBNVME_MI_1_11 { | ||
| global: | ||
| nvme_mi_control; | ||
| nvme_mi_get_async_message; |
There was a problem hiding this comment.
This is a mistake and should be removed
| pollfds[0].events = POLLIN; | ||
| timeout = 1;//1? | ||
| retry: | ||
| rc = ops.poll(pollfds, 1, timeout);//This is probably unecessary |
There was a problem hiding this comment.
Stale comment. Same for //1? above
| @@ -429,11 +486,6 @@ int nvme_mi_submit(nvme_mi_ep_t ep, struct nvme_mi_req *req, | |||
| return -1; | |||
| } | |||
|
|
|||
| if (req->data_len & 0x3) { | |||
There was a problem hiding this comment.
@jk-ozlabs , correct me if I'm wrong, but this check should only be applicable to admin commands and is actually causing problems with AEM SYNC (Configuration Set: AEM) for some data lengths. Didn't want it to slip under the radar.
There was a problem hiding this comment.
Up to this point, all MI messages have been a multiple of 4 bytes. If that's changing, we can reconsider this check.
|
Neat, I'll take a look! |
|
I think the overall concept here is sound, but we are missing a few implementation details for an overall feasibility check too. There are some style things, but we can address those once the general framework is settled. I have added some comments in a review. I've been thinking about how we should be handling the multiple-endpoint cases, as we can only have one socket listening on incoming NVMe-MI message types. A couple of options there:
or:
I figure the latter is probably best, so will take a look at what's required for that. |
| pollfds[0].events = POLLIN; | ||
| timeout = 1;//1? | ||
| retry: | ||
| rc = ops.poll(pollfds, 1, timeout);//This is probably unecessary |
There was a problem hiding this comment.
Yeah, no need for the poll here. If we have the fd in non-blocking mode, just do the recvmsg directly (and handle the EAGAIN)
| return 0; | ||
| } | ||
|
|
||
| static int nvme_mi_mctp_read(struct nvme_mi_ep *ep, |
There was a problem hiding this comment.
We probably want a name that indicates that this is for the aem messaging.
| Ack, | ||
| None, |
There was a problem hiding this comment.
We probably want better namespacing on these, and consistent SCREAMING_SNAKE_CASE
| * | ||
| * Return: -1 if error, 0 otherwise | ||
| */ | ||
| int nvme_mi_get_pollfd(nvme_mi_ep_t ep, struct pollfd *fs); |
There was a problem hiding this comment.
Unsure if we want the struct pollfd or just the int fd returned here. Any specific reason for the pollfd?
| .async_read = nvme_mi_mctp_read, | ||
| .async_poll_fd = nvme_mi_mctp_async_poll_fd, |
There was a problem hiding this comment.
It's only a minor naming convention, but we probably want to be careful between the terms async (as an IO-handling pattern) and "Async Event Messaging" (as the NVMe-MI facility). I would suggest using aem consistently here.
| std_in_timeout.tv_usec = 0; | ||
|
|
||
| // Check if there's input on stdin | ||
| retval = select(STDIN_FILENO + 1, &read_fds, NULL, NULL, &std_in_timeout); |
There was a problem hiding this comment.
Just include STDIN_FILENO on the set that you're polling on
|
(BTW, I would suggest converting to a draft while still working out details) |
Added new functionality to mi.c and mi-mctp.c to handle AEMs. Included new example mi-mctp-ae.c for usage.