* [bug report] io_uring: refactor io_sq_thread() handling
@ 2020-11-11 11:38 Dan Carpenter
2020-11-11 14:04 ` Xiaoguang Wang
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2020-11-11 11:38 UTC (permalink / raw)
To: xiaoguang.wang; +Cc: io-uring
Hello Xiaoguang Wang,
The patch e0c06f5ab2c5: "io_uring: refactor io_sq_thread() handling"
from Nov 3, 2020, leads to the following static checker warning:
fs/io_uring.c:6939 io_sq_thread()
error: uninitialized symbol 'timeout'.
fs/io_uring.c
6883 static int io_sq_thread(void *data)
6884 {
6885 struct cgroup_subsys_state *cur_css = NULL;
6886 struct files_struct *old_files = current->files;
6887 struct nsproxy *old_nsproxy = current->nsproxy;
6888 struct pid *old_thread_pid = current->thread_pid;
6889 const struct cred *old_cred = NULL;
6890 struct io_sq_data *sqd = data;
6891 struct io_ring_ctx *ctx;
6892 unsigned long timeout;
^^^^^^^^^^^^^^^^^^^^^^
6893 DEFINE_WAIT(wait);
6894
6895 task_lock(current);
6896 current->files = NULL;
6897 current->nsproxy = NULL;
6898 current->thread_pid = NULL;
6899 task_unlock(current);
6900
6901 while (!kthread_should_stop()) {
6902 int ret;
6903 bool cap_entries, sqt_spin, needs_sched;
6904
6905 /*
6906 * Any changes to the sqd lists are synchronized through the
6907 * kthread parking. This synchronizes the thread vs users,
6908 * the users are synchronized on the sqd->ctx_lock.
6909 */
6910 if (kthread_should_park())
6911 kthread_parkme();
6912
6913 if (unlikely(!list_empty(&sqd->ctx_new_list))) {
6914 io_sqd_init_new(sqd);
6915 timeout = jiffies + sqd->sq_thread_idle;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
timeout not set on else path.
6916 }
6917
6918 sqt_spin = false;
6919 cap_entries = !list_is_singular(&sqd->ctx_list);
6920 list_for_each_entry(ctx, &sqd->ctx_list, sqd_list) {
6921 if (current->cred != ctx->creds) {
6922 if (old_cred)
6923 revert_creds(old_cred);
6924 old_cred = override_creds(ctx->creds);
6925 }
6926 io_sq_thread_associate_blkcg(ctx, &cur_css);
6927 #ifdef CONFIG_AUDIT
6928 current->loginuid = ctx->loginuid;
6929 current->sessionid = ctx->sessionid;
6930 #endif
6931
6932 ret = __io_sq_thread(ctx, cap_entries);
6933 if (!sqt_spin && (ret > 0 || !list_empty(&ctx->iopoll_list)))
6934 sqt_spin = true;
6935
6936 io_sq_thread_drop_mm_files();
6937 }
6938
6939 if (sqt_spin || !time_after(jiffies, timeout)) {
^^^^^^^
warning
6940 io_run_task_work();
6941 cond_resched();
6942 if (sqt_spin)
6943 timeout = jiffies + sqd->sq_thread_idle;
6944 continue;
6945 }
6946
6947 if (kthread_should_park())
6948 continue;
6949
6950 needs_sched = true;
6951 prepare_to_wait(&sqd->wait, &wait, TASK_INTERRUPTIBLE);
6952 list_for_each_entry(ctx, &sqd->ctx_list, sqd_list) {
6953 if ((ctx->flags & IORING_SETUP_IOPOLL) &&
6954 !list_empty_careful(&ctx->iopoll_list)) {
6955 needs_sched = false;
6956 break;
6957 }
6958 if (io_sqring_entries(ctx)) {
6959 needs_sched = false;
6960 break;
6961 }
6962 }
6963
6964 if (needs_sched) {
6965 list_for_each_entry(ctx, &sqd->ctx_list, sqd_list)
6966 io_ring_set_wakeup_flag(ctx);
6967
6968 schedule();
6969 list_for_each_entry(ctx, &sqd->ctx_list, sqd_list)
6970 io_ring_clear_wakeup_flag(ctx);
6971 }
6972
6973 finish_wait(&sqd->wait, &wait);
6974 timeout = jiffies + sqd->sq_thread_idle;
6975 }
6976
6977 io_run_task_work();
6978
6979 if (cur_css)
6980 io_sq_thread_unassociate_blkcg();
6981 if (old_cred)
6982 revert_creds(old_cred);
6983
6984 task_lock(current);
6985 current->files = old_files;
6986 current->nsproxy = old_nsproxy;
6987 current->thread_pid = old_thread_pid;
6988 task_unlock(current);
6989
6990 kthread_parkme();
6991
6992 return 0;
6993 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] io_uring: refactor io_sq_thread() handling
2020-11-11 11:38 [bug report] io_uring: refactor io_sq_thread() handling Dan Carpenter
@ 2020-11-11 14:04 ` Xiaoguang Wang
2020-11-11 15:55 ` Jens Axboe
0 siblings, 1 reply; 3+ messages in thread
From: Xiaoguang Wang @ 2020-11-11 14:04 UTC (permalink / raw)
To: Dan Carpenter; +Cc: io-uring
hi,
> Hello Xiaoguang Wang,
>
> The patch e0c06f5ab2c5: "io_uring: refactor io_sq_thread() handling"
> from Nov 3, 2020, leads to the following static checker warning:
>
> fs/io_uring.c:6939 io_sq_thread()
> error: uninitialized symbol 'timeout'.
>
> fs/io_uring.c
> 6883 static int io_sq_thread(void *data)
> 6884 {
> 6885 struct cgroup_subsys_state *cur_css = NULL;
> 6886 struct files_struct *old_files = current->files;
> 6887 struct nsproxy *old_nsproxy = current->nsproxy;
> 6888 struct pid *old_thread_pid = current->thread_pid;
> 6889 const struct cred *old_cred = NULL;
> 6890 struct io_sq_data *sqd = data;
> 6891 struct io_ring_ctx *ctx;
> 6892 unsigned long timeout;
> ^^^^^^^^^^^^^^^^^^^^^^
>
> 6893 DEFINE_WAIT(wait);
> 6894
> 6895 task_lock(current);
> 6896 current->files = NULL;
> 6897 current->nsproxy = NULL;
> 6898 current->thread_pid = NULL;
> 6899 task_unlock(current);
> 6900
> 6901 while (!kthread_should_stop()) {
> 6902 int ret;
> 6903 bool cap_entries, sqt_spin, needs_sched;
> 6904
> 6905 /*
> 6906 * Any changes to the sqd lists are synchronized through the
> 6907 * kthread parking. This synchronizes the thread vs users,
> 6908 * the users are synchronized on the sqd->ctx_lock.
> 6909 */
> 6910 if (kthread_should_park())
> 6911 kthread_parkme();
> 6912
> 6913 if (unlikely(!list_empty(&sqd->ctx_new_list))) {
> 6914 io_sqd_init_new(sqd);
> 6915 timeout = jiffies + sqd->sq_thread_idle;
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> timeout not set on else path.
Thanks for the report, but indeed I think it's not a bug. When io_sq_thread
is created initially, it's not waken up to run, and once it's waken up to run,
it will see that sqd->ctx_new_list is not empty, then timeout always can be
initialized.
Regards,
Xiaoguang Wang
>
> 6916 }
> 6917
> 6918 sqt_spin = false;
> 6919 cap_entries = !list_is_singular(&sqd->ctx_list);
> 6920 list_for_each_entry(ctx, &sqd->ctx_list, sqd_list) {
> 6921 if (current->cred != ctx->creds) {
> 6922 if (old_cred)
> 6923 revert_creds(old_cred);
> 6924 old_cred = override_creds(ctx->creds);
> 6925 }
> 6926 io_sq_thread_associate_blkcg(ctx, &cur_css);
> 6927 #ifdef CONFIG_AUDIT
> 6928 current->loginuid = ctx->loginuid;
> 6929 current->sessionid = ctx->sessionid;
> 6930 #endif
> 6931
> 6932 ret = __io_sq_thread(ctx, cap_entries);
> 6933 if (!sqt_spin && (ret > 0 || !list_empty(&ctx->iopoll_list)))
> 6934 sqt_spin = true;
> 6935
> 6936 io_sq_thread_drop_mm_files();
> 6937 }
> 6938
> 6939 if (sqt_spin || !time_after(jiffies, timeout)) {
> ^^^^^^^
> warning
>
> 6940 io_run_task_work();
> 6941 cond_resched();
> 6942 if (sqt_spin)
> 6943 timeout = jiffies + sqd->sq_thread_idle;
> 6944 continue;
> 6945 }
> 6946
> 6947 if (kthread_should_park())
> 6948 continue;
> 6949
> 6950 needs_sched = true;
> 6951 prepare_to_wait(&sqd->wait, &wait, TASK_INTERRUPTIBLE);
> 6952 list_for_each_entry(ctx, &sqd->ctx_list, sqd_list) {
> 6953 if ((ctx->flags & IORING_SETUP_IOPOLL) &&
> 6954 !list_empty_careful(&ctx->iopoll_list)) {
> 6955 needs_sched = false;
> 6956 break;
> 6957 }
> 6958 if (io_sqring_entries(ctx)) {
> 6959 needs_sched = false;
> 6960 break;
> 6961 }
> 6962 }
> 6963
> 6964 if (needs_sched) {
> 6965 list_for_each_entry(ctx, &sqd->ctx_list, sqd_list)
> 6966 io_ring_set_wakeup_flag(ctx);
> 6967
> 6968 schedule();
> 6969 list_for_each_entry(ctx, &sqd->ctx_list, sqd_list)
> 6970 io_ring_clear_wakeup_flag(ctx);
> 6971 }
> 6972
> 6973 finish_wait(&sqd->wait, &wait);
> 6974 timeout = jiffies + sqd->sq_thread_idle;
> 6975 }
> 6976
> 6977 io_run_task_work();
> 6978
> 6979 if (cur_css)
> 6980 io_sq_thread_unassociate_blkcg();
> 6981 if (old_cred)
> 6982 revert_creds(old_cred);
> 6983
> 6984 task_lock(current);
> 6985 current->files = old_files;
> 6986 current->nsproxy = old_nsproxy;
> 6987 current->thread_pid = old_thread_pid;
> 6988 task_unlock(current);
> 6989
> 6990 kthread_parkme();
> 6991
> 6992 return 0;
> 6993 }
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] io_uring: refactor io_sq_thread() handling
2020-11-11 14:04 ` Xiaoguang Wang
@ 2020-11-11 15:55 ` Jens Axboe
0 siblings, 0 replies; 3+ messages in thread
From: Jens Axboe @ 2020-11-11 15:55 UTC (permalink / raw)
To: Xiaoguang Wang, Dan Carpenter; +Cc: io-uring
On 11/11/20 7:04 AM, Xiaoguang Wang wrote:
> hi,
>
>> Hello Xiaoguang Wang,
>>
>> The patch e0c06f5ab2c5: "io_uring: refactor io_sq_thread() handling"
>> from Nov 3, 2020, leads to the following static checker warning:
>>
>> fs/io_uring.c:6939 io_sq_thread()
>> error: uninitialized symbol 'timeout'.
>>
>> fs/io_uring.c
>> 6883 static int io_sq_thread(void *data)
>> 6884 {
>> 6885 struct cgroup_subsys_state *cur_css = NULL;
>> 6886 struct files_struct *old_files = current->files;
>> 6887 struct nsproxy *old_nsproxy = current->nsproxy;
>> 6888 struct pid *old_thread_pid = current->thread_pid;
>> 6889 const struct cred *old_cred = NULL;
>> 6890 struct io_sq_data *sqd = data;
>> 6891 struct io_ring_ctx *ctx;
>> 6892 unsigned long timeout;
>> ^^^^^^^^^^^^^^^^^^^^^^
>>
>> 6893 DEFINE_WAIT(wait);
>> 6894
>> 6895 task_lock(current);
>> 6896 current->files = NULL;
>> 6897 current->nsproxy = NULL;
>> 6898 current->thread_pid = NULL;
>> 6899 task_unlock(current);
>> 6900
>> 6901 while (!kthread_should_stop()) {
>> 6902 int ret;
>> 6903 bool cap_entries, sqt_spin, needs_sched;
>> 6904
>> 6905 /*
>> 6906 * Any changes to the sqd lists are synchronized through the
>> 6907 * kthread parking. This synchronizes the thread vs users,
>> 6908 * the users are synchronized on the sqd->ctx_lock.
>> 6909 */
>> 6910 if (kthread_should_park())
>> 6911 kthread_parkme();
>> 6912
>> 6913 if (unlikely(!list_empty(&sqd->ctx_new_list))) {
>> 6914 io_sqd_init_new(sqd);
>> 6915 timeout = jiffies + sqd->sq_thread_idle;
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> timeout not set on else path.
> Thanks for the report, but indeed I think it's not a bug. When io_sq_thread
> is created initially, it's not waken up to run, and once it's waken up to run,
> it will see that sqd->ctx_new_list is not empty, then timeout always can be
> initialized.
We should still clean it up and avoid both the checker tripping on on this,
and humans. It's not easy/possible to verify that it is sane.
--
Jens Axboe
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-11-11 15:55 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-11-11 11:38 [bug report] io_uring: refactor io_sq_thread() handling Dan Carpenter
2020-11-11 14:04 ` Xiaoguang Wang
2020-11-11 15:55 ` Jens Axboe
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox