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

ioctl: Split latency outputs from debugging feature to show command #764

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/libnvme.map
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ LIBNVME_1_7 {
nvme_init_copy_range_f3;
nvme_insert_tls_key_versioned;
nvme_generate_tls_key_identity;
nvme_set_latency;
nvme_get_latency;
};

LIBNVME_1_6 {
Expand Down
155 changes: 148 additions & 7 deletions src/nvme/ioctl.c
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include "util.h"

static bool nvme_debug;
static bool nvme_latency;

static int nvme_verify_chr(int fd)
{
Expand Down Expand Up @@ -90,8 +91,7 @@ static int nvme_submit_passthru64(int fd, unsigned long ioctl_cmd,
return err;
}

static void nvme_show_command(struct nvme_passthru_cmd *cmd, int err, struct timeval start,
struct timeval end)
static void nvme_show_command(struct nvme_passthru_cmd *cmd, int err)
{
printf("opcode : %02x\n", cmd->opcode);
printf("flags : %02x\n", cmd->flags);
Expand All @@ -112,8 +112,136 @@ static void nvme_show_command(struct nvme_passthru_cmd *cmd, int err, struct tim
printf("timeout_ms : %08x\n", cmd->timeout_ms);
printf("result : %08x\n", cmd->result);
printf("err : %d\n", err);
printf("latency : %lu us\n",
(end.tv_sec - start.tv_sec) * 1000000 + (end.tv_usec - start.tv_usec));
}

const char *nvme_admin_to_string(__u8 opcode)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would these functions be useful to export from libnvme?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comments. Yes the code basically copied from nvme-cli: nvme_cmd_to_string() and duplicated so I will do merge the functionality to export from libnmve.

{
switch (opcode) {
case nvme_admin_delete_sq:
return "Delete I/O Submission Queue";
case nvme_admin_create_sq:
return "Create I/O Submission Queue";
case nvme_admin_get_log_page:
return "Get Log Page";
case nvme_admin_delete_cq:
return "Delete I/O Completion Queue";
case nvme_admin_create_cq:
return "Create I/O Completion Queue";
case nvme_admin_identify:
return "Identify";
case nvme_admin_abort_cmd:
return "Abort";
case nvme_admin_set_features:
return "Set Features";
case nvme_admin_get_features:
return "Get Features";
case nvme_admin_async_event:
return "Asynchronous Event Request";
case nvme_admin_ns_mgmt:
return "Namespace Management";
case nvme_admin_fw_commit:
return "Firmware Commit";
case nvme_admin_fw_download:
return "Firmware Image Download";
case nvme_admin_dev_self_test:
return "Device Self-test";
case nvme_admin_ns_attach:
return "Namespace Attachment";
case nvme_admin_keep_alive:
return "Keep Alive";
case nvme_admin_directive_send:
return "Directive Send";
case nvme_admin_directive_recv:
return "Directive Receive";
case nvme_admin_virtual_mgmt:
return "Virtualization Management";
case nvme_admin_nvme_mi_send:
return "NVMe-MI Send";
case nvme_admin_nvme_mi_recv:
return "NVMe-MI Receive";
case nvme_admin_dbbuf:
return "Doorbell Buffer Config";
case nvme_admin_format_nvm:
return "Format NVM";
case nvme_admin_security_send:
return "Security Send";
case nvme_admin_security_recv:
return "Security Receive";
case nvme_admin_sanitize_nvm:
return "Sanitize";
case nvme_admin_get_lba_status:
return "Get LBA Status";
default:
break;
}

return NULL;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just return NULL instead of break?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my understandig for the coding manner better to use the default case. By the way how about the following implementation instead? Or just I thought also to define the opcode strings as a table to use instead of the function using the switch case statements.

	char *ret = NULL;

	switch (opcode) {
	case nvme_admin_delete_sq:
		ret = "Delete I/O Submission Queue";
		break;
...
	default:
		break;
	}

	return ret;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is terser and easier to follow to just return from the switch case, but doesn't really matter. A simple lookup table could definitely be faster. It might take up more memory unless you only stored entries up to the largest defined opcode.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see so will do try to use the table later.

}

const char *nvme_nvm_to_string(__u8 opcode)
{
switch (opcode) {
case nvme_cmd_flush:
return "Flush";
case nvme_cmd_write:
return "Write";
case nvme_cmd_read:
return "Read";
case nvme_cmd_write_uncor:
return "Write Uncorrectable";
case nvme_cmd_compare:
return "Compare";
case nvme_cmd_write_zeroes:
return "Write Zeroes";
case nvme_cmd_dsm:
return "Dataset Management";
case nvme_cmd_resv_register:
return "Reservation Register";
case nvme_cmd_resv_report:
return "Reservation Report";
case nvme_cmd_resv_acquire:
return "Reservation Acquire";
case nvme_cmd_resv_release:
return "Reservation Release";
case nvme_cmd_verify:
return "Verify";
case nvme_cmd_copy:
return "Copy";
case nvme_zns_cmd_mgmt_send:
return "Zone Management Send";
case nvme_zns_cmd_mgmt_recv:
return "Zone Management Receive";
case nvme_zns_cmd_append:
return "Zone Append";
default:
break;
}

return NULL;
}

const char *nvme_cmd_to_string(bool admin, __u8 opcode)
{
const char *cmd_name;

if (admin)
cmd_name = nvme_admin_to_string(opcode);
else
cmd_name = nvme_nvm_to_string(opcode);

if (!cmd_name)
return "Unknown";

return cmd_name;
}

static void nvme_show_latency(bool admin, __u8 opcode, struct timeval *start, struct timeval *end)
{
struct timeval latency;

timersub(end, start, &latency);
printf("%s Command opcode: %02x (%s) latency: %lu us\n", admin ? "Admin" : "IO", opcode,
nvme_cmd_to_string(admin, opcode), latency.tv_sec * 1000000 + latency.tv_usec);
}

void nvme_set_debug(bool debug)
Expand All @@ -126,23 +254,36 @@ bool nvme_get_debug(void)
return nvme_debug;
}

void nvme_set_latency(bool latency)
{
nvme_latency = latency;
}

bool nvme_get_latency(void)
{
return nvme_latency;
}

static int nvme_submit_passthru(int fd, unsigned long ioctl_cmd,
struct nvme_passthru_cmd *cmd, __u32 *result)
{
struct timeval start;
struct timeval end;
int err;

if (nvme_get_debug())
if (nvme_get_latency())
gettimeofday(&start, NULL);

err = ioctl(fd, ioctl_cmd, cmd);

if (nvme_get_debug()) {
if (nvme_get_latency()) {
gettimeofday(&end, NULL);
nvme_show_command(cmd, err, start, end);
nvme_show_latency(ioctl_cmd == NVME_IOCTL_ADMIN_CMD, cmd->opcode, &start, &end);
}

if (nvme_get_debug())
nvme_show_command(cmd, err);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pre-existing, but curious why the 64-bit ioctls (nvme_submit_passthru64()) don't do this logging. Is it not useful for them? Does it just require more work to implement than it's worth?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes will do implement as same for the 64-bit ioctls also.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could probably also be done in the 64-bit case?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Will change as cmd will be handled for both the 32-bit and 64-bit cases.


if (err >= 0 && result)
*result = cmd->result;

Expand Down
13 changes: 13 additions & 0 deletions src/nvme/ioctl.h
Original file line number Diff line number Diff line change
Expand Up @@ -4059,4 +4059,17 @@ void nvme_set_debug(bool debug);
* Return: false if disabled or true if enabled.
*/
bool nvme_get_debug(void);

/**
* nvme_set_latency - Set NVMe command latency output
* @latency: true to enable or false to disable
*/
void nvme_set_latency(bool latency);

/**
* nvme_get_latency - Get NVMe command latency output
*
* Return: false if disabled or true if enabled.
*/
bool nvme_get_latency(void);
#endif /* _LIBNVME_IOCTL_H */