Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FR][NVME-MI] Command level timeout #553

Open
drakedog2008 opened this issue Jan 9, 2023 · 10 comments
Open

[FR][NVME-MI] Command level timeout #553

drakedog2008 opened this issue Jan 9, 2023 · 10 comments
Assignees
Labels
enhancement New feature or request

Comments

@drakedog2008
Copy link
Contributor

The current timeout implement for libnvme-mi is to attach the timeout to mctp endpoint, i.e. all transactions under same EP share the same timeout.

To comparison, the inband implemented the timeout on io level (i.e. per transaction).

The user case is some time-consuming cmd (e.g. FW commit) will not share the global timeout. The difference between inband and oob makes the client hard to implement a unified solution.

@jk-ozlabs

Is it possible to implement the timeout the same way as inband? So the timeout should be part of the input arg of each cmd?

@jk-ozlabs
Copy link
Collaborator

Should be possible, yep.

In the case where an API call involves multiple NVMe-MI commands (ie, multiple messages sent & received): are you looking for a timeout on the whole call, or changing the timeout on individual messages of that command?

@jk-ozlabs jk-ozlabs self-assigned this Jan 10, 2023
@jk-ozlabs jk-ozlabs added the enhancement New feature or request label Jan 10, 2023
@drakedog2008
Copy link
Contributor Author

Can you give an example function what is the multi-msg cmd?

@drakedog2008
Copy link
Contributor Author

The log page with mi chuck?

@drakedog2008
Copy link
Contributor Author

If that is the case, the API should define the unified timeout for all involving mctp transactions, given the timeout is on mctp packet level, not message/command level.

@jk-ozlabs
Copy link
Collaborator

Can you give an example function what is the multi-msg cmd?

Yep, anything requiring chunking; currently Get Log Page, but we should define a general policy.

If that is the case, the API should define the unified timeout for all involving mctp transactions, given the timeout is on mctp packet level, not message/command level.

The MCTP packet level is not really suitable here: there are no timeouts on individual packets, as there are no per-packet responses (and the packetisation is not visible to the socket interface). There may be many packets in flight for each MCTP message. So, we would be defining the timeout at the NVMe-MI command level (ie, corresponding to the MCTP message level) here.

In this case: the timeout would apply separately to each NVMe-MI command + response, rather than the nvme_mi_* API call.

As an example:

  • calling a nvme_mi_admin_get_log function, specifying a timeout of 2 sec, requesting 12288 (4096 * 3) bytes of data
  • that call gets chunked into three NVMe-MI Get Log Page commands, each requesting 4096 bytes of data
  • each NVMe-MI command (consisting of 1 MCTP request message and 1 MCTP response message, themselves consisting of an arbitrary amount of MCTP packets) is completed in 1 sec
  • so, total call time is 3 sec

this would not timeout, because each command+response is completed before the 2-sec timeout.

Is that what you're intending?

@drakedog2008
Copy link
Contributor Author

Yeah, the timeout attached on submit is on message level, not the packet level.

And the mechanism you described works for me.

@jk-ozlabs
Copy link
Collaborator

Sounds good! I'll put something together.

@igaw
Copy link
Collaborator

igaw commented Jul 3, 2024

I assume we close this one, due no progress.

FWIW, the nvme-cli learned a global --timeout command line argument, though I don't know if this relevant for this here.

@jk-ozlabs
Copy link
Collaborator

If it's okay with you, I'd like to keep this open - seems like a worthwhile feature in general.

However, if you'd prefer to keep the issues tidier, I can track elsewhere instead :)

@igaw
Copy link
Collaborator

igaw commented Jul 4, 2024

I am fine with keeping it here open. I was just not sure what's the status is and also too stupid to check the label which says 'enhancement'...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants