Re: [ISSUE] The time cost of IOSQE_IO_LINK

[Peter Zijlstra <peterz@infradead.org>]

On Tue, Feb 18, 2020 at 03:56:46PM +0100, Oleg Nesterov wrote:
> On 02/18, Peter Zijlstra wrote:
> >
> > But this has me wondering about task_work_run(), as it is it will
> > unconditionally take pi_lock,
> 
> because spin_unlock_wait() was removed ;) task_work_run() doesn't
> really need to take pi_lock at all. 

Right.

> > --- a/kernel/task_work.c
> > +++ b/kernel/task_work.c
> > @@ -93,16 +93,20 @@ void task_work_run(void)
> >  	struct callback_head *work, *head, *next;
> >
> >  	for (;;) {
> > +		work = READ_ONCE(task->task_work);
> > +		if (!work)
> > +			break
> 
> This is wrong if PF_EXITING is set, in this case we must set
> task->task_works = work_exited.

Indeed!

> > +
> >  		/*
> >  		 * work->func() can do task_work_add(), do not set
> >  		 * work_exited unless the list is empty.
> >  		 */
> >  		raw_spin_lock_irq(&task->pi_lock);
> >  		do {
> > -			work = READ_ONCE(task->task_works);
> > -			head = !work && (task->flags & PF_EXITING) ?
> > -				&work_exited : NULL;
> > -		} while (cmpxchg(&task->task_works, work, head) != work);
> > +			head = NULL;
> > +			if (unlikely(!work && (task->flags & PF_EXITING)))
> > +				head = &work_exited;
> > +		} while (!try_cmpxchg(&task->task_works, &work, head));
> >  		raw_spin_unlock_irq(&task->pi_lock);
> >
> >  		if (!work)
> 
> otherwise I think this is correct, but how about the patch below?
> Then this code can be changed to use try_cmpxchg().

Works for me. Thanks!