Re: [ISSUE] The time cost of IOSQE_IO_LINK

Peter Zijlstra
2020-02-14T15:32:24Z

On Thu, Feb 13, 2020 at 10:03:54PM -0700, Jens Axboe wrote:

> CC'ing peterz for some cluebat knowledge. Peter, is there a nice way to
> currently do something like this? Only thing I'm currently aware of is
> the preempt in/out notifiers, but they don't quite provide what I need,
> since I need to pass some data (a request) as well.

Whee, nothing quite like this around I think.

> The full detail on what I'm trying here is:
>
> io_uring can have linked requests. One obvious use case for that is to
> queue a POLLIN on a socket, and then link a read/recv to that. When the
> poll completes, we want to run the read/recv. io_uring hooks into the
> waitqueue wakeup handler to finish the poll request, and since we're
> deep in waitqueue wakeup code, it queues the linked read/recv for
> execution via an async thread. This is not optimal, obviously, as it
> relies on a switch to a new thread to perform this read. This hack
> queues a backlog to the task itself, and runs it when it's scheduled in.
> Probably want to do the same for sched out as well, currently I just
> hack that in the io_uring wait part...

I'll definitely need to think more about this, but a few comments on the
below.

> +static void __io_uring_task_handler(struct list_head *list)
> +{
> + struct io_kiocb *req;
> +
> + while (!list_empty(list)) {
> + req = list_first_entry(list, struct io_kiocb, list);
> + list_del(&req->list);
> +
> + __io_queue_sqe(req, NULL);
> + }
> +}
> +
> +void io_uring_task_handler(struct task_struct *tsk)
> +{
> + LIST_HEAD(list);
> +
> + raw_spin_lock_irq(&tsk->uring_lock);
> + if (!list_empty(&tsk->uring_work))
> + list_splice_init(&tsk->uring_work, &list);
> + raw_spin_unlock_irq(&tsk->uring_lock);
> +
> + __io_uring_task_handler(&list);
> +}

> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index fc1dfc007604..b60f081cac17 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -2717,6 +2717,11 @@ static void __sched_fork(unsigned long clone_flags, struct task_struct *p)
> INIT_HLIST_HEAD(&p->preempt_notifiers);
> #endif
>
> +#ifdef CONFIG_IO_URING
> + INIT_LIST_HEAD(&p->uring_work);
> + raw_spin_lock_init(&p->uring_lock);
> +#endif
> +
> #ifdef CONFIG_COMPACTION
> p->capture_control = NULL;
> #endif
> @@ -3069,6 +3074,20 @@ fire_sched_out_preempt_notifiers(struct task_struct *curr,
>
> #endif /* CONFIG_PREEMPT_NOTIFIERS */
>
> +#ifdef CONFIG_IO_URING
> +extern void io_uring_task_handler(struct task_struct *tsk);
> +
> +static inline void io_uring_handler(struct task_struct *tsk)
> +{
> + if (!list_empty(&tsk->uring_work))
> + io_uring_task_handler(tsk);
> +}
> +#else /* !CONFIG_IO_URING */
> +static inline void io_uring_handler(struct task_struct *tsk)
> +{
> +}
> +#endif
> +
> static inline void prepare_task(struct task_struct *next)
> {
> #ifdef CONFIG_SMP
> @@ -3322,6 +3341,8 @@ asmlinkage __visible void schedule_tail(struct task_struct *prev)
> balance_callback(rq);
> preempt_enable();
>
> + io_uring_handler(current);
> +
> if (current->set_child_tid)
> put_user(task_pid_vnr(current), current->set_child_tid);
>

I suspect you meant to put that in finish_task_switch() which is the
tail end of every schedule(), schedule_tail() is the tail end of
clone().

Or maybe you meant to put it in (and rename) sched_update_worker() which
is after every schedule() but in a preemptible context -- much saner
since you don't want to go add an unbounded amount of work in a
non-preemptible context.

At which point you already have your callback: io_wq_worker_running(),
or is this for any random task?