* [PATCH 0/3] text representation of opcode in trace
@ 2022-04-25 9:36 Dylan Yudaken
2022-04-25 9:36 ` [PATCH 1/3] io_uring: add io_uring_get_opcode Dylan Yudaken
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Dylan Yudaken @ 2022-04-25 9:36 UTC (permalink / raw)
To: io-uring; +Cc: axboe, asml.silence, Kernel-team, Dylan Yudaken
This series adds the text representation of opcodes into the trace. This
makes it much quicker to understand traces without having to translate
opcodes in your head.
Patch 1 is the translation function.
Patch 2 is a small cleanup
Patch 3 uses the translator in the trace logic
Dylan Yudaken (3):
io_uring: add io_uring_get_opcode
io_uring: rename op -> opcode
io_uring: use the text representation of ops in trace
fs/io_uring.c | 91 +++++++++++++++++++++++++++++++++
include/linux/io_uring.h | 5 ++
include/trace/events/io_uring.h | 42 +++++++++------
3 files changed, 121 insertions(+), 17 deletions(-)
base-commit: 155bc9505dbd6613585abbf0be6466f1c21536c4
--
2.30.2
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/3] io_uring: add io_uring_get_opcode 2022-04-25 9:36 [PATCH 0/3] text representation of opcode in trace Dylan Yudaken @ 2022-04-25 9:36 ` Dylan Yudaken 2022-04-25 12:38 ` Jens Axboe 2022-04-25 9:36 ` [PATCH 2/3] io_uring: rename op -> opcode Dylan Yudaken 2022-04-25 9:36 ` [PATCH 3/3] io_uring: use the text representation of ops in trace Dylan Yudaken 2 siblings, 1 reply; 9+ messages in thread From: Dylan Yudaken @ 2022-04-25 9:36 UTC (permalink / raw) To: io-uring; +Cc: axboe, asml.silence, Kernel-team, Dylan Yudaken In some debug scenarios it is useful to have the text representation of the opcode. Add this function in preparation. Signed-off-by: Dylan Yudaken <[email protected]> --- fs/io_uring.c | 91 ++++++++++++++++++++++++++++++++++++++++ include/linux/io_uring.h | 5 +++ 2 files changed, 96 insertions(+) diff --git a/fs/io_uring.c b/fs/io_uring.c index e57d47a23682..326695f74b93 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -1255,6 +1255,97 @@ static struct kmem_cache *req_cachep; static const struct file_operations io_uring_fops; +const char *io_uring_get_opcode(u8 opcode) +{ + switch (opcode) { + case IORING_OP_NOP: + return "NOP"; + case IORING_OP_READV: + return "READV"; + case IORING_OP_WRITEV: + return "WRITEV"; + case IORING_OP_FSYNC: + return "FSYNC"; + case IORING_OP_READ_FIXED: + return "READ_FIXED"; + case IORING_OP_WRITE_FIXED: + return "WRITE_FIXED"; + case IORING_OP_POLL_ADD: + return "POLL_ADD"; + case IORING_OP_POLL_REMOVE: + return "POLL_REMOVE"; + case IORING_OP_SYNC_FILE_RANGE: + return "SYNC_FILE_RANGE"; + case IORING_OP_SENDMSG: + return "SENDMSG"; + case IORING_OP_RECVMSG: + return "RECVMSG"; + case IORING_OP_TIMEOUT: + return "TIMEOUT"; + case IORING_OP_TIMEOUT_REMOVE: + return "TIMEOUT_REMOVE"; + case IORING_OP_ACCEPT: + return "ACCEPT"; + case IORING_OP_ASYNC_CANCEL: + return "ASYNC_CANCEL"; + case IORING_OP_LINK_TIMEOUT: + return "LINK_TIMEOUT"; + case IORING_OP_CONNECT: + return "CONNECT"; + case IORING_OP_FALLOCATE: + return "FALLOCATE"; + case IORING_OP_OPENAT: + return "OPENAT"; + case IORING_OP_CLOSE: + return "CLOSE"; + case IORING_OP_FILES_UPDATE: + return "FILES_UPDATE"; + case IORING_OP_STATX: + return "STATX"; + case IORING_OP_READ: + return "READ"; + case IORING_OP_WRITE: + return "WRITE"; + case IORING_OP_FADVISE: + return "FADVISE"; + case IORING_OP_MADVISE: + return "MADVISE"; + case IORING_OP_SEND: + return "SEND"; + case IORING_OP_RECV: + return "RECV"; + case IORING_OP_OPENAT2: + return "OPENAT2"; + case IORING_OP_EPOLL_CTL: + return "EPOLL_CTL"; + case IORING_OP_SPLICE: + return "SPLICE"; + case IORING_OP_PROVIDE_BUFFERS: + return "PROVIDE_BUFFERS"; + case IORING_OP_REMOVE_BUFFERS: + return "REMOVE_BUFFERS"; + case IORING_OP_TEE: + return "TEE"; + case IORING_OP_SHUTDOWN: + return "SHUTDOWN"; + case IORING_OP_RENAMEAT: + return "RENAMEAT"; + case IORING_OP_UNLINKAT: + return "UNLINKAT"; + case IORING_OP_MKDIRAT: + return "MKDIRAT"; + case IORING_OP_SYMLINKAT: + return "SYMLINKAT"; + case IORING_OP_LINKAT: + return "LINKAT"; + case IORING_OP_MSG_RING: + return "MSG_RING"; + case IORING_OP_LAST: + return "LAST"; + } + return "UNKNOWN"; +} + struct sock *io_uring_get_socket(struct file *file) { #if defined(CONFIG_UNIX) diff --git a/include/linux/io_uring.h b/include/linux/io_uring.h index 1814e698d861..24651c229ed2 100644 --- a/include/linux/io_uring.h +++ b/include/linux/io_uring.h @@ -10,6 +10,7 @@ struct sock *io_uring_get_socket(struct file *file); void __io_uring_cancel(bool cancel_all); void __io_uring_free(struct task_struct *tsk); void io_uring_unreg_ringfd(void); +const char *io_uring_get_opcode(u8 opcode); static inline void io_uring_files_cancel(void) { @@ -42,6 +43,10 @@ static inline void io_uring_files_cancel(void) static inline void io_uring_free(struct task_struct *tsk) { } +static inline const char *io_uring_get_opcode(u8 opcode) +{ + return ""; +} #endif #endif -- 2.30.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] io_uring: add io_uring_get_opcode 2022-04-25 9:36 ` [PATCH 1/3] io_uring: add io_uring_get_opcode Dylan Yudaken @ 2022-04-25 12:38 ` Jens Axboe 2022-04-25 13:21 ` Dylan Yudaken 0 siblings, 1 reply; 9+ messages in thread From: Jens Axboe @ 2022-04-25 12:38 UTC (permalink / raw) To: Dylan Yudaken, io-uring; +Cc: asml.silence, Kernel-team On 4/25/22 3:36 AM, Dylan Yudaken wrote: > In some debug scenarios it is useful to have the text representation of > the opcode. Add this function in preparation. > > Signed-off-by: Dylan Yudaken <[email protected]> > --- > fs/io_uring.c | 91 ++++++++++++++++++++++++++++++++++++++++ > include/linux/io_uring.h | 5 +++ > 2 files changed, 96 insertions(+) > > diff --git a/fs/io_uring.c b/fs/io_uring.c > index e57d47a23682..326695f74b93 100644 > --- a/fs/io_uring.c > +++ b/fs/io_uring.c > @@ -1255,6 +1255,97 @@ static struct kmem_cache *req_cachep; > > static const struct file_operations io_uring_fops; > > +const char *io_uring_get_opcode(u8 opcode) > +{ > + switch (opcode) { > + case IORING_OP_NOP: > + return "NOP"; > + case IORING_OP_READV: > + return "READV"; > + case IORING_OP_WRITEV: > + return "WRITEV"; > + case IORING_OP_FSYNC: > + return "FSYNC"; > + case IORING_OP_READ_FIXED: > + return "READ_FIXED"; > + case IORING_OP_WRITE_FIXED: > + return "WRITE_FIXED"; > + case IORING_OP_POLL_ADD: > + return "POLL_ADD"; > + case IORING_OP_POLL_REMOVE: > + return "POLL_REMOVE"; > + case IORING_OP_SYNC_FILE_RANGE: > + return "SYNC_FILE_RANGE"; > + case IORING_OP_SENDMSG: > + return "SENDMSG"; > + case IORING_OP_RECVMSG: > + return "RECVMSG"; > + case IORING_OP_TIMEOUT: > + return "TIMEOUT"; > + case IORING_OP_TIMEOUT_REMOVE: > + return "TIMEOUT_REMOVE"; > + case IORING_OP_ACCEPT: > + return "ACCEPT"; > + case IORING_OP_ASYNC_CANCEL: > + return "ASYNC_CANCEL"; > + case IORING_OP_LINK_TIMEOUT: > + return "LINK_TIMEOUT"; > + case IORING_OP_CONNECT: > + return "CONNECT"; > + case IORING_OP_FALLOCATE: > + return "FALLOCATE"; > + case IORING_OP_OPENAT: > + return "OPENAT"; > + case IORING_OP_CLOSE: > + return "CLOSE"; > + case IORING_OP_FILES_UPDATE: > + return "FILES_UPDATE"; > + case IORING_OP_STATX: > + return "STATX"; > + case IORING_OP_READ: > + return "READ"; > + case IORING_OP_WRITE: > + return "WRITE"; > + case IORING_OP_FADVISE: > + return "FADVISE"; > + case IORING_OP_MADVISE: > + return "MADVISE"; > + case IORING_OP_SEND: > + return "SEND"; > + case IORING_OP_RECV: > + return "RECV"; > + case IORING_OP_OPENAT2: > + return "OPENAT2"; > + case IORING_OP_EPOLL_CTL: > + return "EPOLL_CTL"; > + case IORING_OP_SPLICE: > + return "SPLICE"; > + case IORING_OP_PROVIDE_BUFFERS: > + return "PROVIDE_BUFFERS"; > + case IORING_OP_REMOVE_BUFFERS: > + return "REMOVE_BUFFERS"; > + case IORING_OP_TEE: > + return "TEE"; > + case IORING_OP_SHUTDOWN: > + return "SHUTDOWN"; > + case IORING_OP_RENAMEAT: > + return "RENAMEAT"; > + case IORING_OP_UNLINKAT: > + return "UNLINKAT"; > + case IORING_OP_MKDIRAT: > + return "MKDIRAT"; > + case IORING_OP_SYMLINKAT: > + return "SYMLINKAT"; > + case IORING_OP_LINKAT: > + return "LINKAT"; > + case IORING_OP_MSG_RING: > + return "MSG_RING"; > + case IORING_OP_LAST: > + return "LAST"; > + } > + return "UNKNOWN"; > +} My only worry here is that it's another place to touch when adding an opcode. I'm assuming the compiler doesn't warn if you're missing one since it's not strongly typed? In any case, the LAST one is just a sentinel, both that and beyond should return eg INVALID. -- Jens Axboe ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] io_uring: add io_uring_get_opcode 2022-04-25 12:38 ` Jens Axboe @ 2022-04-25 13:21 ` Dylan Yudaken 2022-04-25 14:29 ` Jens Axboe 0 siblings, 1 reply; 9+ messages in thread From: Dylan Yudaken @ 2022-04-25 13:21 UTC (permalink / raw) To: [email protected], [email protected] Cc: Kernel Team, [email protected] On Mon, 2022-04-25 at 06:38 -0600, Jens Axboe wrote: > On 4/25/22 3:36 AM, Dylan Yudaken wrote: > > In some debug scenarios it is useful to have the text representation > > of > > the opcode. Add this function in preparation. > > > > Signed-off-by: Dylan Yudaken <[email protected]> > > --- > > fs/io_uring.c | 91 > > ++++++++++++++++++++++++++++++++++++++++ > > include/linux/io_uring.h | 5 +++ > > 2 files changed, 96 insertions(+) > > > > diff --git a/fs/io_uring.c b/fs/io_uring.c > > index e57d47a23682..326695f74b93 100644 > > --- a/fs/io_uring.c > > +++ b/fs/io_uring.c > > @@ -1255,6 +1255,97 @@ static struct kmem_cache *req_cachep; > > > > static const struct file_operations io_uring_fops; > > > > +const char *io_uring_get_opcode(u8 opcode) > > +{ > > + switch (opcode) { > > + case IORING_OP_NOP: > > + return "NOP"; > > + case IORING_OP_READV: > > + return "READV"; > > + case IORING_OP_WRITEV: > > + return "WRITEV"; > > + case IORING_OP_FSYNC: > > + return "FSYNC"; > > + case IORING_OP_READ_FIXED: > > + return "READ_FIXED"; > > + case IORING_OP_WRITE_FIXED: > > + return "WRITE_FIXED"; > > + case IORING_OP_POLL_ADD: > > + return "POLL_ADD"; > > + case IORING_OP_POLL_REMOVE: > > + return "POLL_REMOVE"; > > + case IORING_OP_SYNC_FILE_RANGE: > > + return "SYNC_FILE_RANGE"; > > + case IORING_OP_SENDMSG: > > + return "SENDMSG"; > > + case IORING_OP_RECVMSG: > > + return "RECVMSG"; > > + case IORING_OP_TIMEOUT: > > + return "TIMEOUT"; > > + case IORING_OP_TIMEOUT_REMOVE: > > + return "TIMEOUT_REMOVE"; > > + case IORING_OP_ACCEPT: > > + return "ACCEPT"; > > + case IORING_OP_ASYNC_CANCEL: > > + return "ASYNC_CANCEL"; > > + case IORING_OP_LINK_TIMEOUT: > > + return "LINK_TIMEOUT"; > > + case IORING_OP_CONNECT: > > + return "CONNECT"; > > + case IORING_OP_FALLOCATE: > > + return "FALLOCATE"; > > + case IORING_OP_OPENAT: > > + return "OPENAT"; > > + case IORING_OP_CLOSE: > > + return "CLOSE"; > > + case IORING_OP_FILES_UPDATE: > > + return "FILES_UPDATE"; > > + case IORING_OP_STATX: > > + return "STATX"; > > + case IORING_OP_READ: > > + return "READ"; > > + case IORING_OP_WRITE: > > + return "WRITE"; > > + case IORING_OP_FADVISE: > > + return "FADVISE"; > > + case IORING_OP_MADVISE: > > + return "MADVISE"; > > + case IORING_OP_SEND: > > + return "SEND"; > > + case IORING_OP_RECV: > > + return "RECV"; > > + case IORING_OP_OPENAT2: > > + return "OPENAT2"; > > + case IORING_OP_EPOLL_CTL: > > + return "EPOLL_CTL"; > > + case IORING_OP_SPLICE: > > + return "SPLICE"; > > + case IORING_OP_PROVIDE_BUFFERS: > > + return "PROVIDE_BUFFERS"; > > + case IORING_OP_REMOVE_BUFFERS: > > + return "REMOVE_BUFFERS"; > > + case IORING_OP_TEE: > > + return "TEE"; > > + case IORING_OP_SHUTDOWN: > > + return "SHUTDOWN"; > > + case IORING_OP_RENAMEAT: > > + return "RENAMEAT"; > > + case IORING_OP_UNLINKAT: > > + return "UNLINKAT"; > > + case IORING_OP_MKDIRAT: > > + return "MKDIRAT"; > > + case IORING_OP_SYMLINKAT: > > + return "SYMLINKAT"; > > + case IORING_OP_LINKAT: > > + return "LINKAT"; > > + case IORING_OP_MSG_RING: > > + return "MSG_RING"; > > + case IORING_OP_LAST: > > + return "LAST"; > > + } > > + return "UNKNOWN"; > > +} > > My only worry here is that it's another place to touch when adding an > opcode. I'm assuming the compiler doesn't warn if you're missing one > since it's not strongly typed? It doesn't complain, but we could strongly type it to get it to? I don't think it will break anything (certainly does not locally). What about something like this: diff --git a/fs/io_uring.c b/fs/io_uring.c index 326695f74b93..90ecd656cc13 100644 --- a/fs/io_uring.c +++ b/fs/io_uring.c @@ -1257,7 +1257,7 @@ static const struct file_operations io_uring_fops; const char *io_uring_get_opcode(u8 opcode) { - switch (opcode) { + switch ((enum io_uring_op)opcode) { case IORING_OP_NOP: return "NOP"; case IORING_OP_READV: diff --git a/include/uapi/linux/io_uring.h b/include/uapi/linux/io_uring.h index 980d82eb196e..a10b216ede3e 100644 --- a/include/uapi/linux/io_uring.h +++ b/include/uapi/linux/io_uring.h @@ -103,7 +103,7 @@ enum { #define IORING_SETUP_R_DISABLED (1U << 6) /* start with ring disabled */ #define IORING_SETUP_SUBMIT_ALL (1U << 7) /* continue submit on error */ -enum { +enum io_uring_op { IORING_OP_NOP, IORING_OP_READV, IORING_OP_WRITEV, > > In any case, the LAST one is just a sentinel, both that and beyond > should return eg INVALID. > Will do ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] io_uring: add io_uring_get_opcode 2022-04-25 13:21 ` Dylan Yudaken @ 2022-04-25 14:29 ` Jens Axboe 2022-04-25 14:48 ` Dylan Yudaken 0 siblings, 1 reply; 9+ messages in thread From: Jens Axboe @ 2022-04-25 14:29 UTC (permalink / raw) To: Dylan Yudaken, [email protected] Cc: Kernel Team, [email protected] On 4/25/22 7:21 AM, Dylan Yudaken wrote: > On Mon, 2022-04-25 at 06:38 -0600, Jens Axboe wrote: >> On 4/25/22 3:36 AM, Dylan Yudaken wrote: >>> In some debug scenarios it is useful to have the text representation >>> of >>> the opcode. Add this function in preparation. >>> >>> Signed-off-by: Dylan Yudaken <[email protected]> >>> --- >>> fs/io_uring.c | 91 >>> ++++++++++++++++++++++++++++++++++++++++ >>> include/linux/io_uring.h | 5 +++ >>> 2 files changed, 96 insertions(+) >>> >>> diff --git a/fs/io_uring.c b/fs/io_uring.c >>> index e57d47a23682..326695f74b93 100644 >>> --- a/fs/io_uring.c >>> +++ b/fs/io_uring.c >>> @@ -1255,6 +1255,97 @@ static struct kmem_cache *req_cachep; >>> >>> static const struct file_operations io_uring_fops; >>> >>> +const char *io_uring_get_opcode(u8 opcode) >>> +{ >>> + switch (opcode) { >>> + case IORING_OP_NOP: >>> + return "NOP"; >>> + case IORING_OP_READV: >>> + return "READV"; >>> + case IORING_OP_WRITEV: >>> + return "WRITEV"; >>> + case IORING_OP_FSYNC: >>> + return "FSYNC"; >>> + case IORING_OP_READ_FIXED: >>> + return "READ_FIXED"; >>> + case IORING_OP_WRITE_FIXED: >>> + return "WRITE_FIXED"; >>> + case IORING_OP_POLL_ADD: >>> + return "POLL_ADD"; >>> + case IORING_OP_POLL_REMOVE: >>> + return "POLL_REMOVE"; >>> + case IORING_OP_SYNC_FILE_RANGE: >>> + return "SYNC_FILE_RANGE"; >>> + case IORING_OP_SENDMSG: >>> + return "SENDMSG"; >>> + case IORING_OP_RECVMSG: >>> + return "RECVMSG"; >>> + case IORING_OP_TIMEOUT: >>> + return "TIMEOUT"; >>> + case IORING_OP_TIMEOUT_REMOVE: >>> + return "TIMEOUT_REMOVE"; >>> + case IORING_OP_ACCEPT: >>> + return "ACCEPT"; >>> + case IORING_OP_ASYNC_CANCEL: >>> + return "ASYNC_CANCEL"; >>> + case IORING_OP_LINK_TIMEOUT: >>> + return "LINK_TIMEOUT"; >>> + case IORING_OP_CONNECT: >>> + return "CONNECT"; >>> + case IORING_OP_FALLOCATE: >>> + return "FALLOCATE"; >>> + case IORING_OP_OPENAT: >>> + return "OPENAT"; >>> + case IORING_OP_CLOSE: >>> + return "CLOSE"; >>> + case IORING_OP_FILES_UPDATE: >>> + return "FILES_UPDATE"; >>> + case IORING_OP_STATX: >>> + return "STATX"; >>> + case IORING_OP_READ: >>> + return "READ"; >>> + case IORING_OP_WRITE: >>> + return "WRITE"; >>> + case IORING_OP_FADVISE: >>> + return "FADVISE"; >>> + case IORING_OP_MADVISE: >>> + return "MADVISE"; >>> + case IORING_OP_SEND: >>> + return "SEND"; >>> + case IORING_OP_RECV: >>> + return "RECV"; >>> + case IORING_OP_OPENAT2: >>> + return "OPENAT2"; >>> + case IORING_OP_EPOLL_CTL: >>> + return "EPOLL_CTL"; >>> + case IORING_OP_SPLICE: >>> + return "SPLICE"; >>> + case IORING_OP_PROVIDE_BUFFERS: >>> + return "PROVIDE_BUFFERS"; >>> + case IORING_OP_REMOVE_BUFFERS: >>> + return "REMOVE_BUFFERS"; >>> + case IORING_OP_TEE: >>> + return "TEE"; >>> + case IORING_OP_SHUTDOWN: >>> + return "SHUTDOWN"; >>> + case IORING_OP_RENAMEAT: >>> + return "RENAMEAT"; >>> + case IORING_OP_UNLINKAT: >>> + return "UNLINKAT"; >>> + case IORING_OP_MKDIRAT: >>> + return "MKDIRAT"; >>> + case IORING_OP_SYMLINKAT: >>> + return "SYMLINKAT"; >>> + case IORING_OP_LINKAT: >>> + return "LINKAT"; >>> + case IORING_OP_MSG_RING: >>> + return "MSG_RING"; >>> + case IORING_OP_LAST: >>> + return "LAST"; >>> + } >>> + return "UNKNOWN"; >>> +} >> >> My only worry here is that it's another place to touch when adding an >> opcode. I'm assuming the compiler doesn't warn if you're missing one >> since it's not strongly typed? > > It doesn't complain, but we could strongly type it to get it to? I > don't think it will break anything (certainly does not locally). What > about something like this: I think this would be fine. Would probably be cleaner if you just make io_uring_get_opcode() take an enum io_uring_op and just fwd declare it in io_uring.h? -- Jens Axboe ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] io_uring: add io_uring_get_opcode 2022-04-25 14:29 ` Jens Axboe @ 2022-04-25 14:48 ` Dylan Yudaken 2022-04-25 14:53 ` Jens Axboe 0 siblings, 1 reply; 9+ messages in thread From: Dylan Yudaken @ 2022-04-25 14:48 UTC (permalink / raw) To: [email protected], [email protected] Cc: Kernel Team, [email protected] On Mon, 2022-04-25 at 08:29 -0600, Jens Axboe wrote: > On 4/25/22 7:21 AM, Dylan Yudaken wrote: > > On Mon, 2022-04-25 at 06:38 -0600, Jens Axboe wrote: > > > On 4/25/22 3:36 AM, Dylan Yudaken wrote: > > > > In some debug scenarios it is useful to have the text > > > > representation > > > > of > > > > the opcode. Add this function in preparation. > > > > > > > > Signed-off-by: Dylan Yudaken <[email protected]> > > > > --- > > > > fs/io_uring.c | 91 > > > > ++++++++++++++++++++++++++++++++++++++++ > > > > include/linux/io_uring.h | 5 +++ > > > > 2 files changed, 96 insertions(+) > > > > > > > > diff --git a/fs/io_uring.c b/fs/io_uring.c > > > > index e57d47a23682..326695f74b93 100644 > > > > --- a/fs/io_uring.c > > > > +++ b/fs/io_uring.c > > > > @@ -1255,6 +1255,97 @@ static struct kmem_cache *req_cachep; > > > > > > > > static const struct file_operations io_uring_fops; > > > > > > > > +const char *io_uring_get_opcode(u8 opcode) > > > > +{ > > > > + switch (opcode) { > > > > + case IORING_OP_NOP: > > > > + return "NOP"; > > > > + case IORING_OP_READV: > > > > + return "READV"; > > > > + case IORING_OP_WRITEV: > > > > + return "WRITEV"; > > > > + case IORING_OP_FSYNC: > > > > + return "FSYNC"; > > > > + case IORING_OP_READ_FIXED: > > > > + return "READ_FIXED"; > > > > + case IORING_OP_WRITE_FIXED: > > > > + return "WRITE_FIXED"; > > > > + case IORING_OP_POLL_ADD: > > > > + return "POLL_ADD"; > > > > + case IORING_OP_POLL_REMOVE: > > > > + return "POLL_REMOVE"; > > > > + case IORING_OP_SYNC_FILE_RANGE: > > > > + return "SYNC_FILE_RANGE"; > > > > + case IORING_OP_SENDMSG: > > > > + return "SENDMSG"; > > > > + case IORING_OP_RECVMSG: > > > > + return "RECVMSG"; > > > > + case IORING_OP_TIMEOUT: > > > > + return "TIMEOUT"; > > > > + case IORING_OP_TIMEOUT_REMOVE: > > > > + return "TIMEOUT_REMOVE"; > > > > + case IORING_OP_ACCEPT: > > > > + return "ACCEPT"; > > > > + case IORING_OP_ASYNC_CANCEL: > > > > + return "ASYNC_CANCEL"; > > > > + case IORING_OP_LINK_TIMEOUT: > > > > + return "LINK_TIMEOUT"; > > > > + case IORING_OP_CONNECT: > > > > + return "CONNECT"; > > > > + case IORING_OP_FALLOCATE: > > > > + return "FALLOCATE"; > > > > + case IORING_OP_OPENAT: > > > > + return "OPENAT"; > > > > + case IORING_OP_CLOSE: > > > > + return "CLOSE"; > > > > + case IORING_OP_FILES_UPDATE: > > > > + return "FILES_UPDATE"; > > > > + case IORING_OP_STATX: > > > > + return "STATX"; > > > > + case IORING_OP_READ: > > > > + return "READ"; > > > > + case IORING_OP_WRITE: > > > > + return "WRITE"; > > > > + case IORING_OP_FADVISE: > > > > + return "FADVISE"; > > > > + case IORING_OP_MADVISE: > > > > + return "MADVISE"; > > > > + case IORING_OP_SEND: > > > > + return "SEND"; > > > > + case IORING_OP_RECV: > > > > + return "RECV"; > > > > + case IORING_OP_OPENAT2: > > > > + return "OPENAT2"; > > > > + case IORING_OP_EPOLL_CTL: > > > > + return "EPOLL_CTL"; > > > > + case IORING_OP_SPLICE: > > > > + return "SPLICE"; > > > > + case IORING_OP_PROVIDE_BUFFERS: > > > > + return "PROVIDE_BUFFERS"; > > > > + case IORING_OP_REMOVE_BUFFERS: > > > > + return "REMOVE_BUFFERS"; > > > > + case IORING_OP_TEE: > > > > + return "TEE"; > > > > + case IORING_OP_SHUTDOWN: > > > > + return "SHUTDOWN"; > > > > + case IORING_OP_RENAMEAT: > > > > + return "RENAMEAT"; > > > > + case IORING_OP_UNLINKAT: > > > > + return "UNLINKAT"; > > > > + case IORING_OP_MKDIRAT: > > > > + return "MKDIRAT"; > > > > + case IORING_OP_SYMLINKAT: > > > > + return "SYMLINKAT"; > > > > + case IORING_OP_LINKAT: > > > > + return "LINKAT"; > > > > + case IORING_OP_MSG_RING: > > > > + return "MSG_RING"; > > > > + case IORING_OP_LAST: > > > > + return "LAST"; > > > > + } > > > > + return "UNKNOWN"; > > > > +} > > > > > > My only worry here is that it's another place to touch when > > > adding an > > > opcode. I'm assuming the compiler doesn't warn if you're missing > > > one > > > since it's not strongly typed? > > > > It doesn't complain, but we could strongly type it to get it to? I > > don't think it will break anything (certainly does not locally). > > What > > about something like this: > > I think this would be fine. Would probably be cleaner if you just > make > io_uring_get_opcode() take an enum io_uring_op and just fwd declare > it > in io_uring.h? I cannot do that bit, as there is a separate function for CONFIG_IO_URING=n. That would either require different signatures (u8 vs enum io_uring_op) or including the enum even when not enabled in config. Both of these feel ugly. I'll send a new series giving the enum a type. Dylan ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] io_uring: add io_uring_get_opcode 2022-04-25 14:48 ` Dylan Yudaken @ 2022-04-25 14:53 ` Jens Axboe 0 siblings, 0 replies; 9+ messages in thread From: Jens Axboe @ 2022-04-25 14:53 UTC (permalink / raw) To: Dylan Yudaken, [email protected] Cc: Kernel Team, [email protected] On 4/25/22 8:48 AM, Dylan Yudaken wrote: > On Mon, 2022-04-25 at 08:29 -0600, Jens Axboe wrote: >> On 4/25/22 7:21 AM, Dylan Yudaken wrote: >>> On Mon, 2022-04-25 at 06:38 -0600, Jens Axboe wrote: >>>> On 4/25/22 3:36 AM, Dylan Yudaken wrote: >>>>> In some debug scenarios it is useful to have the text >>>>> representation >>>>> of >>>>> the opcode. Add this function in preparation. >>>>> >>>>> Signed-off-by: Dylan Yudaken <[email protected]> >>>>> --- >>>>> fs/io_uring.c | 91 >>>>> ++++++++++++++++++++++++++++++++++++++++ >>>>> include/linux/io_uring.h | 5 +++ >>>>> 2 files changed, 96 insertions(+) >>>>> >>>>> diff --git a/fs/io_uring.c b/fs/io_uring.c >>>>> index e57d47a23682..326695f74b93 100644 >>>>> --- a/fs/io_uring.c >>>>> +++ b/fs/io_uring.c >>>>> @@ -1255,6 +1255,97 @@ static struct kmem_cache *req_cachep; >>>>> >>>>> static const struct file_operations io_uring_fops; >>>>> >>>>> +const char *io_uring_get_opcode(u8 opcode) >>>>> +{ >>>>> + switch (opcode) { >>>>> + case IORING_OP_NOP: >>>>> + return "NOP"; >>>>> + case IORING_OP_READV: >>>>> + return "READV"; >>>>> + case IORING_OP_WRITEV: >>>>> + return "WRITEV"; >>>>> + case IORING_OP_FSYNC: >>>>> + return "FSYNC"; >>>>> + case IORING_OP_READ_FIXED: >>>>> + return "READ_FIXED"; >>>>> + case IORING_OP_WRITE_FIXED: >>>>> + return "WRITE_FIXED"; >>>>> + case IORING_OP_POLL_ADD: >>>>> + return "POLL_ADD"; >>>>> + case IORING_OP_POLL_REMOVE: >>>>> + return "POLL_REMOVE"; >>>>> + case IORING_OP_SYNC_FILE_RANGE: >>>>> + return "SYNC_FILE_RANGE"; >>>>> + case IORING_OP_SENDMSG: >>>>> + return "SENDMSG"; >>>>> + case IORING_OP_RECVMSG: >>>>> + return "RECVMSG"; >>>>> + case IORING_OP_TIMEOUT: >>>>> + return "TIMEOUT"; >>>>> + case IORING_OP_TIMEOUT_REMOVE: >>>>> + return "TIMEOUT_REMOVE"; >>>>> + case IORING_OP_ACCEPT: >>>>> + return "ACCEPT"; >>>>> + case IORING_OP_ASYNC_CANCEL: >>>>> + return "ASYNC_CANCEL"; >>>>> + case IORING_OP_LINK_TIMEOUT: >>>>> + return "LINK_TIMEOUT"; >>>>> + case IORING_OP_CONNECT: >>>>> + return "CONNECT"; >>>>> + case IORING_OP_FALLOCATE: >>>>> + return "FALLOCATE"; >>>>> + case IORING_OP_OPENAT: >>>>> + return "OPENAT"; >>>>> + case IORING_OP_CLOSE: >>>>> + return "CLOSE"; >>>>> + case IORING_OP_FILES_UPDATE: >>>>> + return "FILES_UPDATE"; >>>>> + case IORING_OP_STATX: >>>>> + return "STATX"; >>>>> + case IORING_OP_READ: >>>>> + return "READ"; >>>>> + case IORING_OP_WRITE: >>>>> + return "WRITE"; >>>>> + case IORING_OP_FADVISE: >>>>> + return "FADVISE"; >>>>> + case IORING_OP_MADVISE: >>>>> + return "MADVISE"; >>>>> + case IORING_OP_SEND: >>>>> + return "SEND"; >>>>> + case IORING_OP_RECV: >>>>> + return "RECV"; >>>>> + case IORING_OP_OPENAT2: >>>>> + return "OPENAT2"; >>>>> + case IORING_OP_EPOLL_CTL: >>>>> + return "EPOLL_CTL"; >>>>> + case IORING_OP_SPLICE: >>>>> + return "SPLICE"; >>>>> + case IORING_OP_PROVIDE_BUFFERS: >>>>> + return "PROVIDE_BUFFERS"; >>>>> + case IORING_OP_REMOVE_BUFFERS: >>>>> + return "REMOVE_BUFFERS"; >>>>> + case IORING_OP_TEE: >>>>> + return "TEE"; >>>>> + case IORING_OP_SHUTDOWN: >>>>> + return "SHUTDOWN"; >>>>> + case IORING_OP_RENAMEAT: >>>>> + return "RENAMEAT"; >>>>> + case IORING_OP_UNLINKAT: >>>>> + return "UNLINKAT"; >>>>> + case IORING_OP_MKDIRAT: >>>>> + return "MKDIRAT"; >>>>> + case IORING_OP_SYMLINKAT: >>>>> + return "SYMLINKAT"; >>>>> + case IORING_OP_LINKAT: >>>>> + return "LINKAT"; >>>>> + case IORING_OP_MSG_RING: >>>>> + return "MSG_RING"; >>>>> + case IORING_OP_LAST: >>>>> + return "LAST"; >>>>> + } >>>>> + return "UNKNOWN"; >>>>> +} >>>> >>>> My only worry here is that it's another place to touch when >>>> adding an >>>> opcode. I'm assuming the compiler doesn't warn if you're missing >>>> one >>>> since it's not strongly typed? >>> >>> It doesn't complain, but we could strongly type it to get it to? I >>> don't think it will break anything (certainly does not locally). >>> What >>> about something like this: >> >> I think this would be fine. Would probably be cleaner if you just >> make >> io_uring_get_opcode() take an enum io_uring_op and just fwd declare >> it >> in io_uring.h? > > I cannot do that bit, as there is a separate function for > CONFIG_IO_URING=n. That would either require different signatures (u8 > vs enum io_uring_op) or including the enum even when not enabled in > config. Both of these feel ugly. > > I'll send a new series giving the enum a type. Agree, let's keep the cast then. -- Jens Axboe ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/3] io_uring: rename op -> opcode 2022-04-25 9:36 [PATCH 0/3] text representation of opcode in trace Dylan Yudaken 2022-04-25 9:36 ` [PATCH 1/3] io_uring: add io_uring_get_opcode Dylan Yudaken @ 2022-04-25 9:36 ` Dylan Yudaken 2022-04-25 9:36 ` [PATCH 3/3] io_uring: use the text representation of ops in trace Dylan Yudaken 2 siblings, 0 replies; 9+ messages in thread From: Dylan Yudaken @ 2022-04-25 9:36 UTC (permalink / raw) To: io-uring; +Cc: axboe, asml.silence, Kernel-team, Dylan Yudaken do this for consistency with the other trace messages Signed-off-by: Dylan Yudaken <[email protected]> --- include/trace/events/io_uring.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/trace/events/io_uring.h b/include/trace/events/io_uring.h index 42534ec2ab9d..c122d2167aa4 100644 --- a/include/trace/events/io_uring.h +++ b/include/trace/events/io_uring.h @@ -530,7 +530,7 @@ TRACE_EVENT(io_uring_req_failed, ), TP_printk("ring %p, req %p, user_data 0x%llx, " - "op %d, flags 0x%x, prio=%d, off=%llu, addr=%llu, " + "opcode %d, flags 0x%x, prio=%d, off=%llu, addr=%llu, " "len=%u, rw_flags=0x%x, buf_index=%d, " "personality=%d, file_index=%d, pad=0x%llx/%llx, error=%d", __entry->ctx, __entry->req, __entry->user_data, -- 2.30.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] io_uring: use the text representation of ops in trace 2022-04-25 9:36 [PATCH 0/3] text representation of opcode in trace Dylan Yudaken 2022-04-25 9:36 ` [PATCH 1/3] io_uring: add io_uring_get_opcode Dylan Yudaken 2022-04-25 9:36 ` [PATCH 2/3] io_uring: rename op -> opcode Dylan Yudaken @ 2022-04-25 9:36 ` Dylan Yudaken 2 siblings, 0 replies; 9+ messages in thread From: Dylan Yudaken @ 2022-04-25 9:36 UTC (permalink / raw) To: io-uring; +Cc: axboe, asml.silence, Kernel-team, Dylan Yudaken It is annoying to translate opcodes to textwhen tracing io_uring. Use the io_uring_get_opcode function instead to use the text representation. A downside here might have been that if the opcode is invalid it will not be obvious, however the opcode is already overridden in these cases to 0 (NOP) in io_init_req(). Therefore this is a non issue. Signed-off-by: Dylan Yudaken <[email protected]> --- include/trace/events/io_uring.h | 42 ++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 17 deletions(-) diff --git a/include/trace/events/io_uring.h b/include/trace/events/io_uring.h index c122d2167aa4..240afbb75302 100644 --- a/include/trace/events/io_uring.h +++ b/include/trace/events/io_uring.h @@ -7,6 +7,7 @@ #include <linux/tracepoint.h> #include <uapi/linux/io_uring.h> +#include <linux/io_uring.h> struct io_wq_work; @@ -87,9 +88,11 @@ TRACE_EVENT(io_uring_register, __entry->ret = ret; ), - TP_printk("ring %p, opcode %d, nr_user_files %d, nr_user_bufs %d, " + TP_printk("ring %p, opcode %s, nr_user_files %d, nr_user_bufs %d, " "ret %ld", - __entry->ctx, __entry->opcode, __entry->nr_files, + __entry->ctx, + io_uring_get_opcode(__entry->opcode), + __entry->nr_files, __entry->nr_bufs, __entry->ret) ); @@ -169,8 +172,9 @@ TRACE_EVENT(io_uring_queue_async_work, __entry->rw = rw; ), - TP_printk("ring %p, request %p, user_data 0x%llx, opcode %d, flags 0x%x, %s queue, work %p", - __entry->ctx, __entry->req, __entry->user_data, __entry->opcode, + TP_printk("ring %p, request %p, user_data 0x%llx, opcode %s, flags 0x%x, %s queue, work %p", + __entry->ctx, __entry->req, __entry->user_data, + io_uring_get_opcode(__entry->opcode), __entry->flags, __entry->rw ? "hashed" : "normal", __entry->work) ); @@ -205,8 +209,9 @@ TRACE_EVENT(io_uring_defer, __entry->opcode = opcode; ), - TP_printk("ring %p, request %p, user_data 0x%llx, opcode %d", - __entry->ctx, __entry->req, __entry->data, __entry->opcode) + TP_printk("ring %p, request %p, user_data 0x%llx, opcode %s", + __entry->ctx, __entry->req, __entry->data, + io_uring_get_opcode(__entry->opcode)) ); /** @@ -305,9 +310,9 @@ TRACE_EVENT(io_uring_fail_link, __entry->link = link; ), - TP_printk("ring %p, request %p, user_data 0x%llx, opcode %d, link %p", - __entry->ctx, __entry->req, __entry->user_data, __entry->opcode, - __entry->link) + TP_printk("ring %p, request %p, user_data 0x%llx, opcode %s, link %p", + __entry->ctx, __entry->req, __entry->user_data, + io_uring_get_opcode(__entry->opcode), __entry->link) ); /** @@ -389,9 +394,9 @@ TRACE_EVENT(io_uring_submit_sqe, __entry->sq_thread = sq_thread; ), - TP_printk("ring %p, req %p, user_data 0x%llx, opcode %d, flags 0x%x, " + TP_printk("ring %p, req %p, user_data 0x%llx, opcode %s, flags 0x%x, " "non block %d, sq_thread %d", __entry->ctx, __entry->req, - __entry->user_data, __entry->opcode, + __entry->user_data, io_uring_get_opcode(__entry->opcode), __entry->flags, __entry->force_nonblock, __entry->sq_thread) ); @@ -433,8 +438,9 @@ TRACE_EVENT(io_uring_poll_arm, __entry->events = events; ), - TP_printk("ring %p, req %p, user_data 0x%llx, opcode %d, mask 0x%x, events 0x%x", - __entry->ctx, __entry->req, __entry->user_data, __entry->opcode, + TP_printk("ring %p, req %p, user_data 0x%llx, opcode %s, mask 0x%x, events 0x%x", + __entry->ctx, __entry->req, __entry->user_data, + io_uring_get_opcode(__entry->opcode), __entry->mask, __entry->events) ); @@ -470,8 +476,9 @@ TRACE_EVENT(io_uring_task_add, __entry->mask = mask; ), - TP_printk("ring %p, req %p, user_data 0x%llx, opcode %d, mask %x", - __entry->ctx, __entry->req, __entry->user_data, __entry->opcode, + TP_printk("ring %p, req %p, user_data 0x%llx, opcode %s, mask %x", + __entry->ctx, __entry->req, __entry->user_data, + io_uring_get_opcode(__entry->opcode), __entry->mask) ); @@ -530,11 +537,12 @@ TRACE_EVENT(io_uring_req_failed, ), TP_printk("ring %p, req %p, user_data 0x%llx, " - "opcode %d, flags 0x%x, prio=%d, off=%llu, addr=%llu, " + "opcode %s, flags 0x%x, prio=%d, off=%llu, addr=%llu, " "len=%u, rw_flags=0x%x, buf_index=%d, " "personality=%d, file_index=%d, pad=0x%llx/%llx, error=%d", __entry->ctx, __entry->req, __entry->user_data, - __entry->opcode, __entry->flags, __entry->ioprio, + io_uring_get_opcode(__entry->opcode), + __entry->flags, __entry->ioprio, (unsigned long long)__entry->off, (unsigned long long) __entry->addr, __entry->len, __entry->op_flags, -- 2.30.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-04-25 14:53 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-04-25 9:36 [PATCH 0/3] text representation of opcode in trace Dylan Yudaken 2022-04-25 9:36 ` [PATCH 1/3] io_uring: add io_uring_get_opcode Dylan Yudaken 2022-04-25 12:38 ` Jens Axboe 2022-04-25 13:21 ` Dylan Yudaken 2022-04-25 14:29 ` Jens Axboe 2022-04-25 14:48 ` Dylan Yudaken 2022-04-25 14:53 ` Jens Axboe 2022-04-25 9:36 ` [PATCH 2/3] io_uring: rename op -> opcode Dylan Yudaken 2022-04-25 9:36 ` [PATCH 3/3] io_uring: use the text representation of ops in trace Dylan Yudaken
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox