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

ikegami-t · 2023-12-27T17:19:02Z

No description provided.

Output only latency since not a debugging purpose but for performance. Signed-off-by: Tokunori Ikegami <[email protected]>

calebsander · 2024-01-11T18:24:02Z

src/nvme/ioctl.c

-	       (end.tv_sec - start.tv_sec) * 1000000 + (end.tv_usec - start.tv_usec));
+}
+
+const char *nvme_admin_to_string(__u8 opcode)


Would these functions be useful to export from libnvme?

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.

calebsander · 2024-01-11T18:24:15Z

src/nvme/ioctl.c

+		break;
+	}
+
+	return NULL;


Just return NULL instead of break?

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;

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.

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

calebsander · 2024-01-11T18:26:44Z

src/nvme/ioctl.c

 	}

+	if (nvme_get_debug())
+		nvme_show_command(cmd, err);


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?

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

To use by nvme-cli since duplicated the function in nvme-cli. Signed-off-by: Tokunori Ikegami <[email protected]>

Also move the functions into util from ioctl. Signed-off-by: Tokunori Ikegami <[email protected]>

calebsander · 2024-01-15T17:39:19Z

src/nvme/ioctl.c

+{
+	const char *cmd_name;
+
+	if (req == NVME_IOCTL_ADMIN_CMD || req == NVME_IOCTL_ADMIN64_CMD)


Use nvme_ioctl_cmd_admin()?

calebsander · 2024-01-17T00:56:18Z

src/nvme/ioctl.c

+{
+	bool ioctl_cmd_admin = false;
+
+	switch (req) {
+	case NVME_IOCTL_ADMIN_CMD:
+		fallthrough;
+	case NVME_IOCTL_ADMIN64_CMD:
+		ioctl_cmd_admin = true;
+		break;
+	default:
+		break;
+	}
+
+	return ioctl_cmd_admin;


Just return req == NVME_IOCTL_ADMIN_CMD || req == NVME_IOCTL_ADMIN64_CMD?

Okay right.

calebsander · 2024-01-17T01:58:52Z

src/nvme/ioctl.c

+	if (nvme_get_debug())
+		nvme_show_command(cmd, err);


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

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

martin-belanger · 2024-01-17T16:54:58Z

src/nvme/ioctl.c

+{
+	bool ioctl_cmd = false;
+
+	switch (req) {


Do you really need to use fallthrough? Why not just:

switch (req) { case NVME_IOCTL_ADMIN_CMD: case NVME_IOCTL_IO_CMD: case NVME_IOCTL_ADMIN64_CMD: case NVME_IOCTL_IO64_CMD: ioctl_cmd = true; break;

The checkpatch.pl tool checks fallthrough and the tool is used by the PR check I think. Also usually used it in Kernel code I think also.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl#n7302

martin-belanger · 2024-01-17T16:56:29Z

src/nvme/ioctl.c

+
+static bool nvme_ioctl_cmd_64(unsigned long req)
+{
+	if (req == NVME_IOCTL_ADMIN64_CMD || req == NVME_IOCTL_IO64_CMD)


Could be simplified to a single line:

return req == NVME_IOCTL_ADMIN64_CMD || req == NVME_IOCTL_IO64_CMD;

Yes looks good. Thank you.

martin-belanger · 2024-01-17T17:00:46Z

src/nvme/util.c

+	return NULL;
+}
+
+const char *nvme_ioctl_to_string(unsigned long req)


Could be simplified as follows:

const char *nvme_ioctl_to_string(unsigned long req) { switch (req) { case NVME_IOCTL_ID: return "ID"; case NVME_IOCTL_RESET: return "Reset"; case NVME_IOCTL_SUBSYS_RESET: return "Subsystem reset"; case NVME_IOCTL_RESCAN: return "Rescan"; case NVME_IOCTL_ADMIN_CMD: return "Admin command"; case NVME_IOCTL_IO_CMD: return "IO command"; case NVME_IOCTL_ADMIN64_CMD: return "Admin64 command"; case NVME_IOCTL_IO64_CMD: return "IO64 command"; default: break; } return NULL; }

This was mentioned by the comment #764 (comment) so changed as the current code. Is there any comment about this?

Sorry just I could understand your mention so fixed as suggested.

Note: The latency logging enabled for other nvme ioctl requests also. Signed-off-by: Tokunori Ikegami <[email protected]>

Only result variable is different between nvme_passthru_cmd and cmd64. Signed-off-by: Tokunori Ikegami <[email protected]>

igaw · 2024-01-18T15:56:15Z

I don't like the whole approach. This is adding debugging code to the library. The commit 17429f7 didn't even consider all the ioctl interfaces (64bit version) and prints just to stdout. I think we should remove it.

Why not just use existing syscall tracers instead?

Simple syscall latencies can be traced with strace and you can go fancy with bpftrace. E.g something like:

#!/usr/bin/bpftrace
#include <sys/ioctl.h>

struct nvme_passthru_cmd {
	__u8	opcode;
	__u8	flags;
	__u16	rsvd1;
	__u32	nsid;
	__u32	cdw2;
	__u32	cdw3;
	__u64	metadata;
	__u64	addr;
	__u32	metadata_len;
	__u32	data_len;
	__u32	cdw10;
	__u32	cdw11;
	__u32	cdw12;
	__u32	cdw13;
	__u32	cdw14;
	__u32	cdw15;
	__u32	timeout_ms;
	__u32	result;
};

/* remember our own pid, so that we can filter it out from the handlers */
BEGIN { }

tracepoint:syscalls:sys_enter_ioctl /comm == "nvme" && args->__syscall_nr == 16/ {
       $cmd = (struct nvme_passthru_cmd *)args->arg;

       printf("opcode %d\n",$cmd->opcode);
       printf("cdw10 %x\n", $cmd->cdw10)
}

END { }

igaw · 2024-03-07T13:51:26Z

With the recent refactoring in logging/debugging the annotation and measuring is done inside application. No need to clutter the library with debugging code.

ikegami-t force-pushed the ioctl-latency branch from 5218d9c to 8171bbe Compare December 27, 2023 17:35

ioctl: Split latency outputs from debugging feature to show command

480b5ea

Output only latency since not a debugging purpose but for performance. Signed-off-by: Tokunori Ikegami <[email protected]>

ikegami-t force-pushed the ioctl-latency branch from 8171bbe to 480b5ea Compare December 27, 2023 17:44

calebsander reviewed Jan 11, 2024

View reviewed changes

ioctl: Export nvme opcode string return functions

72d9f96

To use by nvme-cli since duplicated the function in nvme-cli. Signed-off-by: Tokunori Ikegami <[email protected]>

ikegami-t force-pushed the ioctl-latency branch from 97fc520 to 72d9f96 Compare January 13, 2024 16:08

util: Use lookup table to return nvme opcode string

1a08c34

Also move the functions into util from ioctl. Signed-off-by: Tokunori Ikegami <[email protected]>

calebsander approved these changes Jan 17, 2024

View reviewed changes

ikegami-t force-pushed the ioctl-latency branch from e6e1a9a to 80cac76 Compare January 17, 2024 16:48

martin-belanger reviewed Jan 17, 2024

View reviewed changes

ikegami-t force-pushed the ioctl-latency branch from 5c1e4e6 to c286a46 Compare January 17, 2024 17:37

ikegami-t added 2 commits January 18, 2024 22:09

ioctl: Add support for 64-bit ioctls latency logging

818ef76

Note: The latency logging enabled for other nvme ioctl requests also. Signed-off-by: Tokunori Ikegami <[email protected]>

ioctl: Add debugging feature to show command outputs for 64-bit case

38bb22b

Only result variable is different between nvme_passthru_cmd and cmd64. Signed-off-by: Tokunori Ikegami <[email protected]>

ikegami-t force-pushed the ioctl-latency branch from c286a46 to 38bb22b Compare January 18, 2024 13:09

igaw closed this Mar 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ikegami-t commented Dec 27, 2023

calebsander Jan 11, 2024

ikegami-t Jan 13, 2024

calebsander Jan 11, 2024

ikegami-t Jan 13, 2024

calebsander Jan 13, 2024

ikegami-t Jan 13, 2024

calebsander Jan 11, 2024

ikegami-t Jan 13, 2024

calebsander Jan 15, 2024

ikegami-t Jan 17, 2024

calebsander Jan 17, 2024

ikegami-t Jan 17, 2024

calebsander Jan 17, 2024

ikegami-t Jan 17, 2024

martin-belanger Jan 17, 2024

ikegami-t Jan 17, 2024

martin-belanger Jan 17, 2024

ikegami-t Jan 17, 2024

martin-belanger Jan 17, 2024

ikegami-t Jan 17, 2024

ikegami-t Jan 18, 2024

igaw commented Jan 18, 2024

igaw commented Mar 7, 2024

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

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

Conversation

ikegami-t commented Dec 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igaw commented Jan 18, 2024

igaw commented Mar 7, 2024