| Message ID | bb184f8b62703ddd3e6e19eae7ab6c67b97e1e10.1722293317.git.olivier@trillion01.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | io_uring: add napi busy settings to the fdinfo output | expand |
On 7/29/24 23:38, Olivier Langlois wrote: > this info may be useful when attempting to debug a problem > involving a ring using the feature. Apart from a comment below, Reviewed-by: Pavel Begunkov <asml.silence@gmail.com> Maybe, Jens would be willing to move the block after the spin_unlock while applying. > Signed-off-by: Olivier Langlois <olivier@trillion01.com> > --- > io_uring/fdinfo.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/io_uring/fdinfo.c b/io_uring/fdinfo.c > index b1e0e0d85349..3ba42e136a40 100644 > --- a/io_uring/fdinfo.c > +++ b/io_uring/fdinfo.c > @@ -221,7 +221,18 @@ __cold void io_uring_show_fdinfo(struct seq_file *m, struct file *file) > cqe->user_data, cqe->res, cqe->flags); > > } > - > +#ifdef CONFIG_NET_RX_BUSY_POLL > + if (ctx->napi_enabled) { > + seq_puts(m, "NAPI:\tenabled\n"); > + seq_printf(m, "napi_busy_poll_dt:\t%llu\n", ctx->napi_busy_poll_dt); > + if (ctx->napi_prefer_busy_poll) > + seq_puts(m, "napi_prefer_busy_poll:\ttrue\n"); > + else > + seq_puts(m, "napi_prefer_busy_poll:\tfalse\n"); > + } else { > + seq_puts(m, "NAPI:\tdisabled\n"); > + } > +#endif > spin_unlock(&ctx->completion_lock); That doesn't need to be under completion_lock, it should move outside of the spin section. > } > #endif
On Mon, 29 Jul 2024 18:38:33 -0400, Olivier Langlois wrote: > this info may be useful when attempting to debug a problem > involving a ring using the feature. > > Here is an example of the output: > ip-172-31-39-89 /proc/772/fdinfo # cat 14 > pos: 0 > flags: 02000002 > mnt_id: 16 > ino: 10243 > SqMask: 0xff > SqHead: 633 > SqTail: 633 > CachedSqHead: 633 > CqMask: 0x3fff > CqHead: 430250 > CqTail: 430250 > CachedCqTail: 430250 > SQEs: 0 > CQEs: 0 > SqThread: 885 > SqThreadCpu: 0 > SqTotalTime: 52793826 > SqWorkTime: 3590465 > UserFiles: 0 > UserBufs: 0 > PollList: > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=6, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=6, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > op=10, task_works=0 > CqOverflowList: > NAPI: enabled > napi_busy_poll_to: 1 > napi_prefer_busy_poll: true > > [...] Applied, thanks! [1/1] io_uring: add napi busy settings to the fdinfo output commit: 0c87670003aabfdf8772e8ee19d8794adab9a7e7 Best regards,
Thank you Pavel for your review! Since I have no indication if Jens did see your comment before applying my patch, I will prepare another one with your comment addressed. just ignore it if it is not needed anymore. On Tue, 2024-07-30 at 12:05 +0100, Pavel Begunkov wrote: > On 7/29/24 23:38, Olivier Langlois wrote: > > this info may be useful when attempting to debug a problem > > involving a ring using the feature. > > Apart from a comment below, > > Reviewed-by: Pavel Begunkov <asml.silence@gmail.com> > > Maybe, Jens would be willing to move the block after the spin_unlock > while applying. > > > > Signed-off-by: Olivier Langlois <olivier@trillion01.com> > > --- > > io_uring/fdinfo.c | 13 ++++++++++++- > > 1 file changed, 12 insertions(+), 1 deletion(-) > > > > diff --git a/io_uring/fdinfo.c b/io_uring/fdinfo.c > > index b1e0e0d85349..3ba42e136a40 100644 > > --- a/io_uring/fdinfo.c > > +++ b/io_uring/fdinfo.c > > @@ -221,7 +221,18 @@ __cold void io_uring_show_fdinfo(struct > > seq_file *m, struct file *file) > > cqe->user_data, cqe->res, cqe->flags); > > > > } > > - > > +#ifdef CONFIG_NET_RX_BUSY_POLL > > + if (ctx->napi_enabled) { > > + seq_puts(m, "NAPI:\tenabled\n"); > > + seq_printf(m, "napi_busy_poll_dt:\t%llu\n", ctx- > > >napi_busy_poll_dt); > > + if (ctx->napi_prefer_busy_poll) > > + seq_puts(m, > > "napi_prefer_busy_poll:\ttrue\n"); > > + else > > + seq_puts(m, > > "napi_prefer_busy_poll:\tfalse\n"); > > + } else { > > + seq_puts(m, "NAPI:\tdisabled\n"); > > + } > > +#endif > > spin_unlock(&ctx->completion_lock); > > That doesn't need to be under completion_lock, it should move outside > of the spin section. > > > > } > > #endif >
On 7/30/24 15:04, Olivier Langlois wrote: > Thank you Pavel for your review! > > Since I have no indication if Jens did see your comment before applying > my patch, I will prepare another one with your comment addressed. Jens saw it https://git.kernel.dk/cgit/linux-block/commit/?h=for-6.12/io_uring
On 7/30/24 8:08 AM, Pavel Begunkov wrote: > On 7/30/24 15:04, Olivier Langlois wrote: >> Thank you Pavel for your review! >> >> Since I have no indication if Jens did see your comment before applying >> my patch, I will prepare another one with your comment addressed. > > Jens saw it > > https://git.kernel.dk/cgit/linux-block/commit/?h=for-6.12/io_uring Yep, I shuffled it below the lock. I queued this one for 6.12 as it's not really 6.11 material, and the other two for 6.11. I did a bit of commit message editing for all of them.
On 7/29/24 23:38, Olivier Langlois wrote: > this info may be useful when attempting to debug a problem > involving a ring using the feature. While on the topic of busy polling, there is a function io_napi_adjust_timeout(), it ensures that we don't busy poll for longer than the passed wait timeout. Do you use it? I have some doubts in regards to its usefulness, and would prefer to try get rid of it if there are no users since it's a hustle. > CqOverflowList: > NAPI: enabled > napi_busy_poll_to: 1 > napi_prefer_busy_poll: true > > Signed-off-by: Olivier Langlois <olivier@trillion01.com> > --- > io_uring/fdinfo.c | 13 ++++++++++++- > 1 file changed, 12 insertions(+), 1 deletion(-) > > diff --git a/io_uring/fdinfo.c b/io_uring/fdinfo.c > index b1e0e0d85349..3ba42e136a40 100644 > --- a/io_uring/fdinfo.c > +++ b/io_uring/fdinfo.c > @@ -221,7 +221,18 @@ __cold void io_uring_show_fdinfo(struct seq_file *m, struct file *file) > cqe->user_data, cqe->res, cqe->flags); > > } > - > +#ifdef CONFIG_NET_RX_BUSY_POLL > + if (ctx->napi_enabled) { > + seq_puts(m, "NAPI:\tenabled\n"); > + seq_printf(m, "napi_busy_poll_dt:\t%llu\n", ctx->napi_busy_poll_dt); > + if (ctx->napi_prefer_busy_poll) > + seq_puts(m, "napi_prefer_busy_poll:\ttrue\n"); > + else > + seq_puts(m, "napi_prefer_busy_poll:\tfalse\n"); > + } else { > + seq_puts(m, "NAPI:\tdisabled\n"); > + } > +#endif > spin_unlock(&ctx->completion_lock); > } > #endif
On 7/30/24 16:33, Pavel Begunkov wrote: > While on the topic of busy polling, there is a function > io_napi_adjust_timeout(), it ensures that we don't busy poll for longer > than the passed wait timeout. > > Do you use it? I have some doubts in regards to its usefulness, and > would prefer to try get rid of it if there are no users since it's a > hustle. And I need to ask Lewis (CC'ed) the same question (see above). Looking at commit 415ce0ea55c5a3afea501a773e002be9ed7149f5 Author: Jens Axboe <axboe@kernel.dk> Date: Mon Jun 3 13:56:53 2024 -0600 io_uring/napi: fix timeout calculation https://git.kernel.dk/cgit/linux-block/commit/?id=415ce0ea55c5a3afea501a773e002be9ed7149f5 It sounds like you might be using it.
On Tue, 2024-07-30 at 16:33 +0100, Pavel Begunkov wrote: > On 7/29/24 23:38, Olivier Langlois wrote: > > this info may be useful when attempting to debug a problem > > involving a ring using the feature. > > While on the topic of busy polling, there is a function > io_napi_adjust_timeout(), it ensures that we don't busy poll for > longer > than the passed wait timeout. > > Do you use it? I have some doubts in regards to its usefulness, and > would prefer to try get rid of it if there are no users since it's a > hustle. > I am not because I use NAPI busy poll exclusively from SQPOLL thread which in this context all that matters is that the feature is enabled or not. For users doing the polling through the io_uring syscall if the precision of the timeout is important for them, it might serve that purpose.
diff --git a/io_uring/fdinfo.c b/io_uring/fdinfo.c index b1e0e0d85349..3ba42e136a40 100644 --- a/io_uring/fdinfo.c +++ b/io_uring/fdinfo.c @@ -221,7 +221,18 @@ __cold void io_uring_show_fdinfo(struct seq_file *m, struct file *file) cqe->user_data, cqe->res, cqe->flags); } - +#ifdef CONFIG_NET_RX_BUSY_POLL + if (ctx->napi_enabled) { + seq_puts(m, "NAPI:\tenabled\n"); + seq_printf(m, "napi_busy_poll_dt:\t%llu\n", ctx->napi_busy_poll_dt); + if (ctx->napi_prefer_busy_poll) + seq_puts(m, "napi_prefer_busy_poll:\ttrue\n"); + else + seq_puts(m, "napi_prefer_busy_poll:\tfalse\n"); + } else { + seq_puts(m, "NAPI:\tdisabled\n"); + } +#endif spin_unlock(&ctx->completion_lock); } #endif
this info may be useful when attempting to debug a problem involving a ring using the feature. Here is an example of the output: ip-172-31-39-89 /proc/772/fdinfo # cat 14 pos: 0 flags: 02000002 mnt_id: 16 ino: 10243 SqMask: 0xff SqHead: 633 SqTail: 633 CachedSqHead: 633 CqMask: 0x3fff CqHead: 430250 CqTail: 430250 CachedCqTail: 430250 SQEs: 0 CQEs: 0 SqThread: 885 SqThreadCpu: 0 SqTotalTime: 52793826 SqWorkTime: 3590465 UserFiles: 0 UserBufs: 0 PollList: op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=6, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=6, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 op=10, task_works=0 CqOverflowList: NAPI: enabled napi_busy_poll_to: 1 napi_prefer_busy_poll: true Signed-off-by: Olivier Langlois <olivier@trillion01.com> --- io_uring/fdinfo.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)