Re: [bug report] io_uring: refactor io_sq_thread() handling

[Jens Axboe <axboe@kernel.dk>]

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